Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upIntroduce a glyph cache thread. #565
Conversation
|
r? @pcwalton |
|
Does rayon not have a task API (like a job queuing kind of thing)? I thought it did. |
|
Niko says we do offer that, but it requires |
|
We use that in servo and stylo, so presumably that's ok for WR too. |
nikomatsakis
commented
Nov 17, 2016
•
|
You would just do this: rayon::scope(|s| {
s.spawn(|s| /* parallel task */)
});Each task gets a |
|
I'm not sure that will work for the future use case described in the commit, since from what I understand the thread creating that scope will be blocked until all those job(s) complete? I'll have a play with it today, maybe I'm misunderstanding how the API works. |
|
|
|
@pcwalton @nikomatsakis I don't really see how that API fits the design of this PR. I have a thread that handles the event loop (glyph requests etc) and I want that to be able to fire and forget about jobs, and then go back to the event loop and be able to do other work in the meantime. The idea is that at some point later in the frame, the event loop gets a message to block until those jobs are complete, but until that time its free to do other work. But with the rayon scope, that event loop thread would be blocked until all child jobs complete, right? Is there an easy way to express that behaviour with rayon? |
glennw
force-pushed the
glennw:atc3
branch
2 times, most recently
from
085d3ab
to
ab0bd9d
nikomatsakis
commented
Nov 18, 2016
|
The scope API doesn't seem like exactly what you want, I agree. That said, it would be pretty easy to add an API that lets you "fork" a job outside of a scope. You could then have this job send a signal to a port or something when it is done (one could easily build a future etc atop this, if you wanted). Another option would be to run the event loop itself as the scope, but the current APIs aren't really designed for that. I've been wanting to explore integrating as an "executor" in the futures library -- and then it might be a better fit -- but I haven't had time for that yet. (Similarly, I've been wanting to explore things like TBB's 'graph' abstraction, though I think there is a lot of overlap between that and futures.) Anyway, if you would like me to add a "fork" API like the one I described I could do that quite readily. All the machinery is there, it's just not wrapped in a public, safe API. Caveat: I've not read this current PR. =) (I've also been wanting to explore integrating Rayon with the futures library, but haven't had time to do that.) |
nikomatsakis
commented
Nov 18, 2016
|
I should add: one advantage of using Rayon uniformly across Servo is that you have the option all sharing a global thread pool, which tends to be a win, since you don't have multiple threadpools competing for resources. (Though it could cause longer latency, so you may not want that.) |
nikomatsakis
commented
Nov 18, 2016
|
OK, I skimmed the PR. It seems like the API I proposed would act more-or-less precisely the same as the |
|
@nikomatsakis Yep, if we had a rayon API that could work that way, it'd be great to be able to switch this back to rayon and make use of the shared thread pool. |
|
|
nikomatsakis
commented
Nov 18, 2016
|
@glennw can definitely do that... |
|
@nikomatsakis Would it make sense to land this PR now and switch it over to use rayon once that PR lands? (I guess what I'm really asking is how long will it be until that API is available in rayon for use?). |
nikomatsakis
commented
Nov 21, 2016
|
@glennw Either way really. I'd probably be happy to land that patch as is, though I would expect to update the API slightly in the future (which might or might not be a breaking change for you). But if you wanted to land this PR and then compare a rayon-based impl when it becomes available, that's fine too. I'm curious, is it easy for you to do some benchmarks now? (Or would it be, if I landed that PR?) |
|
@nikomatsakis My preference would probably be to get this reviewed and landed, and then switch it to rayon as a follow up (then you won't need to rush to merge your PR either). Benchmarking between the two implementations should be easy to do. Sound ok, @pcwalton ? |
|
Yup, sounds good! |
|
Rebased and pushed. r? @pcwalton |
|
There seem to be quite a few thread barriers on the glyph way to rasterization: RenderApi -> RenderBackend -> GlyphCacheThread -> WorkerThread. We might want to look (at some point later on) into reducing this depth. |
|
r? @mrobinson and @kvark |
| enum GlyphCacheMsg { | ||
| /// Begin the frame - pass ownership of the glyph cache to the thread. | ||
| BeginFrame(FrameId, GlyphCache), | ||
| /// Add a new font |
This comment has been minimized.
This comment has been minimized.
mrobinson
Nov 24, 2016
Member
Super-super-nit: this comment doesn't have a period, but all the others do.
|
This looks good to me as far as I understand it. |
|
@mrobinson Thanks, fixed up the nit and rebased. |
|
@bors-servo: r+ |
|
|
|
|
|
|
This is the first step towards making the glyph cache more efficient and also run asynchronously to allow rendering to continue if the glyph rasterizing becomes a bottleneck (e.g. during animated zooms). Previously, we collected the list of glyphs that were required for a frame, and ran a rayon loop to rasterize any that weren't in the glyph cache. However, this meant that the glyph rasterizion could not start until we knew the list of all required glyphs. Now, all requests for text run glyphs are sent immediately to the glyph cache thread. The glyph cache thread uses a worker thread pool to immediately begin rasterizing any uncached glyphs as soon as they are requested. In the future, the idea is that instead of waiting at the block() method for all pending glyphs to be rasterized, the backend thread will instead receive small numbers of glyphs as they are rasterized. This will allow the backend thread to make a decision that glyph rasterizing is taking too long for this frame, and that we should just draw a frame with the currently cached glyph sizes available. When this occurs, the glyph cache thread will signal that a new frame should be rendered when it completes rasterizing any pending glyphs. This will allow us to keep rendering at 60fps during zoom, even if the glyph rasterizing threads can't keep up (at a quality tradeoff of using lower resolution glyphs).
|
@bors-servo r=pcwalton |
|
|
Introduce a glyph cache thread. This is the first step towards making the glyph cache more efficient and also run asynchronously to allow rendering to continue if the glyph rasterizing becomes a bottleneck (e.g. during animated zooms). Previously, we collected the list of glyphs that were required for a frame, and ran a rayon loop to rasterize any that weren't in the glyph cache. However, this meant that the glyph rasterizion could not start until we knew the list of all required glyphs. Now, all requests for text run glyphs are sent immediately to the glyph cache thread. The glyph cache thread uses a worker thread pool to immediately begin rasterizing any uncached glyphs as soon as they are requested. In the future, the idea is that instead of waiting at the block() method for all pending glyphs to be rasterized, the backend thread will instead receive small numbers of glyphs as they are rasterized. This will allow the backend thread to make a decision that glyph rasterizing is taking too long for this frame, and that we should just draw a frame with the currently cached glyph sizes available. When this occurs, the glyph cache thread will signal that a new frame should be rendered when it completes rasterizing any pending glyphs. This will allow us to keep rendering at 60fps during zoom, even if the glyph rasterizing threads can't keep up (at a quality tradeoff of using lower resolution glyphs). <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/565) <!-- Reviewable:end -->
|
|
glennw commentedNov 17, 2016
•
edited by larsbergstrom
This is the first step towards making the glyph cache more efficient
and also run asynchronously to allow rendering to continue if the
glyph rasterizing becomes a bottleneck (e.g. during animated zooms).
Previously, we collected the list of glyphs that were required for
a frame, and ran a rayon loop to rasterize any that weren't in the
glyph cache. However, this meant that the glyph rasterizion could
not start until we knew the list of all required glyphs.
Now, all requests for text run glyphs are sent immediately to the
glyph cache thread. The glyph cache thread uses a worker thread
pool to immediately begin rasterizing any uncached glyphs as soon
as they are requested.
In the future, the idea is that instead of waiting at the block()
method for all pending glyphs to be rasterized, the backend thread
will instead receive small numbers of glyphs as they are rasterized.
This will allow the backend thread to make a decision that glyph
rasterizing is taking too long for this frame, and that we should
just draw a frame with the currently cached glyph sizes available.
When this occurs, the glyph cache thread will signal that a new frame
should be rendered when it completes rasterizing any pending glyphs.
This will allow us to keep rendering at 60fps during zoom, even
if the glyph rasterizing threads can't keep up (at a quality
tradeoff of using lower resolution glyphs).
This change is