Make image cache per-document rather than global #16048

ferjm · 2017-03-20T17:04:45Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #15701.
These changes do not require new tests because there should already be WPTs for image loads.

This change is

highfive · 2017-03-20T17:04:49Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/pipeline.rs, components/constellation/constellation.rs
@KiChjang: components/net/lib.rs, components/script/layout_image.rs, components/script/dom/htmlcanvaselement.rs, components/script/dom/window.rs, components/script_traits/lib.rs, components/script_traits/lib.rs, components/net_traits/lib.rs, components/net_traits/lib.rs, components/net_traits/image_cache_thread.rs, components/net_traits/image_cache_thread.rs, components/net_traits/image_cache.rs, components/net_traits/image_cache.rs, components/script/dom/htmlimageelement.rs, components/script_layout_interface/lib.rs, components/script/script_thread.rs, components/script_layout_interface/message.rs, components/net/image_cache_thread.rs, components/script/dom/webglrenderingcontext.rs, components/script/dom/bindings/trace.rs, components/script/dom/canvasrenderingcontext2d.rs, components/net_traits/Cargo.toml, components/net_traits/Cargo.toml
@fitzgen: components/script/layout_image.rs, components/script/dom/htmlcanvaselement.rs, components/script/dom/window.rs, components/script_traits/lib.rs, components/script_traits/lib.rs, components/script/dom/htmlimageelement.rs, components/script_layout_interface/lib.rs, components/script/script_thread.rs, components/script_layout_interface/message.rs, components/script/dom/webglrenderingcontext.rs, components/script/dom/bindings/trace.rs, components/script/dom/canvasrenderingcontext2d.rs
@emilio: components/layout/display_list_builder.rs, components/layout/context.rs, components/layout/fragment.rs, components/script/dom/webglrenderingcontext.rs

highfive · 2017-03-20T17:04:51Z

Warning

These commits modify net, layout, and script code, but no tests are modified. Please consider adding a test!

ferjm · 2017-03-20T17:15:10Z

r? @jdm

jdm · 2017-03-20T17:33:23Z

One thing I see when skimming this PR - by moving image_cache_thread from net to net_traits, we make changing its implementation take longer to compile. We should consider moving the implementation back to net and providing a trait in net_traits for consumers instead.

ferjm · 2017-03-21T17:49:28Z

@bors-servo try

bors-servo · 2017-03-21T17:49:29Z

@ferjm: 🔑 Insufficient privileges: and not in try users

ferjm · 2017-03-21T18:09:53Z

@jdm it seems that I still don't have privileges to trigger try runs. I guess servo/saltfs@09aa211 has not been pushed to production yet. Could you trigger a try run for me, please? I'd like to see if this patch breaks any tests. Thank you!

jdm · 2017-03-21T18:27:26Z

@bors-servo: try

bors-servo · 2017-03-21T18:27:30Z

⌛ Trying commit f9968f7 with merge 54a3309...


        Auto merge of #16048 - ferjm:issue-15701-image-cache, r=<try>

Make image cache per-document rather than global - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15701. - [X] These changes do not require new tests because there should already be WPTs for image loads.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16048)

bors-servo · 2017-03-21T19:04:12Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
State: approved= try=True

jdm

Good start! There are some tricky details that we still need to get right, however.

jdm · 2017-03-21T22:29:09Z

components/net_traits/image_cache.rs


+    // Webrender API instance.
+    webrender_api: Mutex<webrender_traits::RenderApi>,


I'm tempted to recommend that we should avoid storing separate mutexes inside ImageCache, and make consumers wrap it in a single Mutex instead.

May I ask why? :) I would prefer to hide the Mutex complexity to consumers.

Multiple mutexes introduce opportunities for deadlocks. I would prefer to rely on Rust's borrow checker and write methods that accept &mut self arguments, rather than wrapping each member in a separate Mutex.

Ok, thanks for the explanation.

I am a bit blocked trying to address this comment. I think I understand the issue, but I am not sure how to solve it.

So the ImageCacheImpl struct has a webrender_traits::RenderApi member and webrender_traits::RenderApi contains a Cell<webrender_traits::ResourceId>. The thing is that Cell implements Send but not Sync and I was solving this by wrapping the RenderApi member in its own Mutex. But now, removing this Mutex and wrapping the entire ImageCacheImpl in a Mutex makes the compiler unhappy:

rror[E0277]: the trait bound `std::cell::Cell<webrender_traits::ResourceId>: std::marker::Sync` is not satisfied in `image_cache::ImageCacheImpl` --> /Volumes/mozilladev/servo/components/net/image_cache.rs:406:6 | 406 | impl ImageCache for ImageCacheImpl { | ^^^^^^^^^^ within `image_cache::ImageCacheImpl`, the trait `std::marker::Sync` is not implemented for `std::cell::Cell<webrender_traits::ResourceId>` | = note: `std::cell::Cell<webrender_traits::ResourceId>` cannot be shared between threads safely = note: required because it appears within the type `webrender_traits::RenderApi` = note: required because it appears within the type `image_cache::ImageCacheImpl` = note: required by `net_traits::image_cache::ImageCache` error[E0277]: the trait bound `std::cell::Cell<()>: std::marker::Sync` is not satisfied in `image_cache::ImageCacheImpl` --> /Volumes/mozilladev/servo/components/net/image_cache.rs:406:6 | 406 | impl ImageCache for ImageCacheImpl { | ^^^^^^^^^^ within `image_cache::ImageCacheImpl`, the trait `std::marker::Sync` is not implemented for `std::cell::Cell<()>` | = note: `std::cell::Cell<()>` cannot be shared between threads safely = note: required because it appears within the type `std::marker::PhantomData<std::cell::Cell<()>>` = note: required because it appears within the type `ipc_channel::platform::OsIpcSender` = note: required because it appears within the type `ipc_channel::ipc::IpcSender<webrender_traits::ApiMsg>` = note: required because it appears within the type `webrender_traits::RenderApi` = note: required because it appears within the type `image_cache::ImageCacheImpl` = note: required by `net_traits::image_cache::ImageCache`

@jdm should I keep the Mutex for the webrender_traits::RenderApi member? Or is there a better way to solve this?

Nevermind. I think I fixed it.

jdm · 2017-03-21T22:29:09Z

components/net_traits/image_cache.rs

+
+    // Public API
+
+    pub fn new(webrender_api: webrender_traits::RenderApi) -> Arc<ImageCache> {


Let's extract the public API into a trait that lives in net_traits/image_cache.rs, and move the implementation back into net_traits/image_cache.rs.

Yes, this was done in the second commit.

jdm · 2017-03-21T22:29:09Z

components/net_traits/image_cache.rs

-    DecoderMsg {
-        key: key,
-        image: image
+                        if let Some(msg) = rx.recv().ok() {


Synchronously waiting on the result here makes spawning a thread redundant. We should call handle_decoder from inside the thread instead, but that suddenly makes this code more complicated to write. Maybe notify_pending_response needs to accept an additional argument which is an Arc for self? The other option is to given the image cache an interface it can use to enqueue an async operation to call handle_decoder on the original thread; this would mean that there is no risk of an image being decoded halfway through a layout operation, which sounds like a good idea.

Fixed in the 3rd commit.

jdm · 2017-03-21T22:29:09Z

components/layout_traits/lib.rs

@@ -48,4 +48,4 @@ pub trait LayoutThreadFactory {
              content_process_shutdown_chan: Option<IpcSender<()>>,
              webrender_api_sender: webrender_traits::RenderApiSender,
              layout_threads: usize);
-}
+    }


Unintentional?

ferjm · 2017-03-22T20:31:35Z

@jdm my last commit is not exactly what you were asking for, but I think it's a good middle ground solution where we have a single Mutex for the ImageCache data as you suggested but we don't require consumers to explicitly wrap with a Mutex and request the locks. I also removed the synchronous wait after spawning the image decoding thread. Thanks for your feedback!

jdm · 2017-03-27T13:34:46Z

components/net/image_cache.rs

@@ -405,14 +401,39 @@ impl ImageCacheImpl {
    }
 }

+struct ImageDecoderRunnable {


I don't see a reason for this separate structure rather than having the code inline in the caller of run.

jdm · 2017-03-27T13:34:46Z

components/net/image_cache.rs

+    }
+}
+
+pub struct ImageCacheImpl {


All the code using this now puts it in an extra Arc, right? Can we fix that by making this structure Clone and storing ImageCacheImpl (perhaps renaming it to ImageCacheHandle).

@jdm if I am not wrong, this means that I need to use net::image_cache::ImageCacheHandle instead of net_traits::image_cache::ImageCache in all consumers, is this ok?

Bleah, you're right. Let's leave it the way it is.


        Make image cache per-document rather than global


        Move image cache implementation to the net crate


        Make ImageCacheImpl have a single Mutex<ImageCacheStore> and use Imag…

…eDecoderRunnable


        Remove useless ImageDecoderRunnable struct

jdm · 2017-03-27T19:40:38Z

@bors-servo: r+

bors-servo · 2017-03-27T19:40:39Z

📌 Commit fa433a7 has been approved by jdm

bors-servo · 2017-03-27T20:50:48Z

⌛ Testing commit fa433a7 with merge c85a02b...


        Auto merge of #16048 - ferjm:issue-15701-image-cache, r=jdm

Make image cache per-document rather than global - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15701. - [X] These changes do not require new tests because there should already be WPTs for image loads.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16048)

bors-servo · 2017-03-27T21:15:57Z

💔 Test failed - mac-rel-wpt1

jdm · 2017-03-27T21:49:44Z

@bors-servo: retry

#14323

bors-servo · 2017-03-27T21:54:18Z

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt2, windows-msvc-dev are reusable. Rebuilding only mac-rel-wpt1...

bors-servo · 2017-03-27T22:18:24Z

💔 Test failed - mac-rel-wpt1

jdm · 2017-03-27T22:27:06Z

@bors-servo: retry

#14323

bors-servo · 2017-03-27T22:57:34Z

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt2, windows-msvc-dev are reusable. Rebuilding only mac-rel-wpt1...

bors-servo · 2017-03-27T23:08:06Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
Approved by: jdm
Pushing c85a02b to master...

highfive assigned Manishearth Mar 20, 2017

highfive added the S-awaiting-review label Mar 20, 2017

highfive assigned jdm and unassigned Manishearth Mar 20, 2017

ferjm force-pushed the ferjm:issue-15701-image-cache branch 2 times, most recently from 2c6ff8f to f9968f7 Mar 20, 2017

jdm requested changes Mar 21, 2017

View changes

ferjm force-pushed the ferjm:issue-15701-image-cache branch from f9968f7 to d9bb5d0 Mar 22, 2017

ferjm requested a review from jdm Mar 27, 2017

jdm requested changes Mar 27, 2017

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Mar 27, 2017

ferjm added 4 commits Mar 16, 2017

Make image cache per-document rather than global

72d7ee6

Move image cache implementation to the net crate

166c4e8

Make ImageCacheImpl have a single Mutex<ImageCacheStore> and use Imag…

f4da165

…eDecoderRunnable

Remove useless ImageDecoderRunnable struct

Loading status checks…

fa433a7

ferjm force-pushed the ferjm:issue-15701-image-cache branch from d9bb5d0 to fa433a7 Mar 27, 2017

highfive removed the S-needs-code-changes label Mar 27, 2017

highfive added the S-awaiting-review label Mar 27, 2017

ferjm requested a review from jdm Mar 27, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Mar 27, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Mar 27, 2017

highfive added the S-tests-failed label Mar 27, 2017

bors-servo merged commit fa433a7 into servo:master Mar 27, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

This was referenced Mar 27, 2017

WIP : Using the entry settings object in CanvasRenderingContext2d::is_origin_clean #15887

Closed

Redirect document loads manually #15354

Merged

ferjm deleted the ferjm:issue-15701-image-cache branch Mar 28, 2017


		// Webrender API instance.
		webrender_api: Mutex<webrender_traits::RenderApi>,


		// Public API

		pub fn new(webrender_api: webrender_traits::RenderApi) -> Arc<ImageCache> {

servo / servo

Join GitHub today

Make image cache per-document rather than global #16048

Make image cache per-document rather than global #16048

Conversation

ferjm commented Mar 20, 2017 • edited

highfive commented Mar 20, 2017

highfive commented Mar 20, 2017

ferjm commented Mar 20, 2017

jdm commented Mar 20, 2017

ferjm commented Mar 21, 2017

bors-servo commented Mar 21, 2017

ferjm commented Mar 21, 2017

jdm commented Mar 21, 2017

bors-servo commented Mar 21, 2017

bors-servo commented Mar 21, 2017

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ferjm commented Mar 22, 2017 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Mar 27, 2017

bors-servo commented Mar 27, 2017

bors-servo commented Mar 27, 2017

bors-servo commented Mar 27, 2017

jdm commented Mar 27, 2017

bors-servo commented Mar 27, 2017

bors-servo commented Mar 27, 2017

jdm commented Mar 27, 2017

bors-servo commented Mar 27, 2017

bors-servo commented Mar 27, 2017

ferjm commented Mar 20, 2017 •

edited

ferjm commented Mar 22, 2017 •

edited