Support using the same vector cache thread and thread pool with several render backends #852

nical · 2017-02-08T15:57:18Z

This PR renames the glyph cache thread into vector cache thread in preparation for the upcoming vector image renderer work, and makes it possible for several render backends to use the same vector cache thread.
This makes it possible to prevent creating 5 threads for each window (Gecko tends to have a bunch of them since each tooltip and popup is its own window), although by default (as in RendererOption::default()), each Renderer creates its own vector cache thread with 4 worker threads.

The main difference is that the vector cache thread doesn't keep a Sender anymore, the latter being passed in the EndFrame message every time.

r? @glennw

This change is


        Rename the glyph cache thread into vector cache thread and support sh…

…aring it with several renderers.

kvark

I think the change, while being positive, is missing a crucial bit in order to:

makes it possible for several render backends to use the same vector cache thread

If multiple render backends would try to simultaneously send BeginFrame/EndFrame messages, the shared vector thread will not handle it. Perhaps, each sender needs to be associated with a namespace, similarly how RenderApi::IdNamespace works?

kvark · 2017-02-08T19:58:13Z

webrender/src/resource_cache.rs

-    (msg_tx, result_rx)
-}
+    msg_tx
+}


nit: missing newline

kvark · 2017-02-08T19:58:13Z

webrender/src/renderer.rs

@@ -757,11 +758,15 @@ impl Renderer {
        let render_target_debug = options.render_target_debug;
        let payload_tx_for_backend = payload_tx.clone();
        let recorder = options.recorder;
+        let vector_cache_tx = mem::replace(&mut options.vector_cache_sender, None).unwrap_or_else(||{


using mem::replace here is an overkill, just use Option::take(): https://doc.rust-lang.org/stable/std/option/enum.Option.html#method.take

kvark · 2017-02-08T19:58:13Z

webrender/src/resource_cache.rs

+}
+
+pub fn spawn_vector_cache_thread(worker_count: usize) -> Sender<VectorCacheMsg> {
+    let worker_count = if worker_count > 0 { worker_count } else { 1 };


I assume we'll have something like const DEFAULT_WORKER_COUNT: usize = 1 eventually

glennw · 2017-02-08T21:08:34Z

I haven't read this is detail yet - but it sounds like the GlyphCache thread is shared as well as the worker thread pool?

I was thinking of sharing the thread pool only, and still having one GlyphCache thread per instance. This would be a lot simpler and fix any issues with BeginFrame/EndFrame namespacing etc.

How does that sound?

kvark · 2017-02-08T21:57:32Z

@glennw

I was thinking of sharing the thread pool only, and still having one GlyphCache thread per instance. This would be a lot simpler and fix any issues with BeginFrame/EndFrame namespacing etc.

Citing a person all of us have met: "That would be glorious!"

nical · 2017-02-08T23:08:06Z

Sharing only the thread pool was what I was going for initially, but I ended up going for sharing the vector cache as well. The reason is that it itches me that when a user hovers his cursor over something and a little tooltip appears, two threads have to be created (one for the render backend and one for the vector cache (and previously 4 more threads due to the thread pool). There is also the vague hope that several windows could share some resources like font data and rasterized glyphs.
In gecko windows add up a lot quicker than in servo (which I think has only one).

I only superficially looked at the actual logic in the thread and indeed after a closer look it wouldn't work out of the box when sharing the vector cache thread.

So I'm a bit undecided about this. I guess we have more important things to do so I'll stick with sharing only the thread pool for now since it is dead simple. Some time later it would be worth coming back to this and think about how we can make this all play optimally with many windows (including not creating a render backend thread for every tooltip). In the mean time we can rely on gecko only using WebRender for the main windows and using the old BasicLayerManager for tooltips and popups.

glennw · 2017-02-08T23:12:07Z

@nical Sounds good, thanks! I agree that we should revisit this later with a view to making multiple instances share resources / threads better. In the meantime, sharing the thread pool is a nice easy win :)

kvark · 2017-02-08T23:35:32Z

@nical

The reason is that it itches me that when a user hovers his cursor over something and a little tooltip appears, two threads have to be created

What's going on on the Gecko side in this case? Is the new window created for a tooltip? Does it have its own GL context, and if so - can it share resources with the main one?
Asking to probe if Gecko can re-cycle WR instances instead of re-creating them.

nical · 2017-02-09T10:09:07Z

In gecko, things like tooltips, drop-down menus, the awesome-bar's autocomplete thing, popups, and the likes all have their own windows. I wouldn't be surprised that we recycle tooltip windows because we typically only ever have one or none present at a time. For the rest I don't think we do. Each window has its own GL context which it is tightly coupled to, so I don't think we can recycle a renderer and put it on a different window (unless we can replace the gl context of an existing renderer, but then we add complexity in a place where it is less useful).

For now it's fair to assume that WebRender is only for big/complex windows and not use it with smaller popup type things. In a distant and bright future, it would make sense to be able to turn some knobs and make it lighter-weight for small/simple windows (and not have to use the legacy rendering code for these).
That and also leverage the probability that several windows are using the same resources like fonts.


        Auto merge of #855 - nical:thread-pool, r=kvark

@kvark

Enable several WebRender instances to share a thread pool. This is a simplified version of PR #852 which only enables sharing the thread pool. It would be worth looking into why ThreadPool isn't Sync and see if we can avoid the mutex. r? @kvark  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/855)


        Auto merge of #855 - nical:thread-pool, r=kvark

@kvark

Enable several WebRender instances to share a thread pool. This is a simplified version of PR #852 which only enables sharing the thread pool. It would be worth looking into why ThreadPool isn't Sync and see if we can avoid the mutex. r? @kvark  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/855)

glennw · 2017-02-09T22:37:36Z

Closing in favour of #855


        Auto merge of #855 - nical:thread-pool, r=kvark

@kvark

Enable several WebRender instances to share a thread pool. This is a simplified version of PR #852 which only enables sharing the thread pool. It would be worth looking into why ThreadPool isn't Sync and see if we can avoid the mutex. r? @kvark  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/855)


        Auto merge of #855 - nical:thread-pool, r=kvark

@kvark

Enable several WebRender instances to share a thread pool. This is a simplified version of PR #852 which only enables sharing the thread pool. It would be worth looking into why ThreadPool isn't Sync and see if we can avoid the mutex. r? @kvark  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/855)

Rename the glyph cache thread into vector cache thread and support sh…

Loading status checks…

f891bf3

…aring it with several renderers.

staktrace mentioned this pull request Feb 8, 2017

Allow WR to be initialized with an optional shared threadpool for glyph cache threads. #819

Closed

kvark mentioned this pull request Feb 8, 2017

Add simple SVG primitives #402

Open

kvark reviewed Feb 8, 2017

View changes

nical mentioned this pull request Feb 9, 2017

Enable several WebRender instances to share a thread pool. #855

Merged

glennw closed this Feb 9, 2017

servo / webrender

Support using the same vector cache thread and thread pool with several render backends #852

Support using the same vector cache thread and thread pool with several render backends #852

nical commented Feb 8, 2017 •

edited by larsbergstrom

kvark left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

glennw commented Feb 8, 2017

kvark commented Feb 8, 2017

nical commented Feb 8, 2017

glennw commented Feb 8, 2017

kvark commented Feb 8, 2017

nical commented Feb 9, 2017

glennw commented Feb 9, 2017

servo / webrender

Join GitHub today

Support using the same vector cache thread and thread pool with several render backends #852

Support using the same vector cache thread and thread pool with several render backends #852

Conversation

nical commented Feb 8, 2017 • edited by larsbergstrom

kvark left a comment

This comment has been minimized.

kvark Feb 8, 2017

This comment has been minimized.

kvark Feb 8, 2017

This comment has been minimized.

kvark Feb 8, 2017

glennw commented Feb 8, 2017

kvark commented Feb 8, 2017

nical commented Feb 8, 2017

glennw commented Feb 8, 2017

kvark commented Feb 8, 2017

nical commented Feb 9, 2017

glennw commented Feb 9, 2017

nical commented Feb 8, 2017 •

edited by larsbergstrom