Details
- Reviewers
nical - Commits
- rMOZILLACENTRAL29584a35d69a: Bug 1595767 - Don't use rayon for glyph rasterization unless there is a lot of…
- Bugzilla Bug ID
- 1595767
Diff Detail
- Repository
- rMOZILLACENTRAL mozilla-central
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
| gfx/wr/webrender/src/glyph_rasterizer/mod.rs | ||
|---|---|---|
| 171–172 | If there is a chance this blocks, we might still want to self.workers.spawn this part. | |
| 184 | This 8 is a rough guess from looking at a histogram of the number of glyphs request on a couple of test pages. | |
We also need a font context on the render backend thread to register/unregister fonts with. see add_font, etc.
Thanks for reviewing.
Regarding the extra context, wouldn't that be the existing shared_context ? (code link)
// This worker should be accessed by threads that don't belong to the thread pool // (in theory that's only the render backend thread so no contention expected either). shared_context: Mutex<FontContext>,
It seems like async_for_each includes the shared_context in the list of contexts that it works on, so add_font etc. already do the right thing.
process_glyph calls font_contexts.lock_current_context:
pub fn lock_current_context(&self) -> MutexGuard<FontContext> {
let id = self.current_worker_id();
self.lock_context(id)
}current_worker_id is a wrapper around Rayon::current_thread_index which returns None when not actually called from a worker thread, ie. when inlining.
And lock_context will return the shared_context in that case:
None => self.shared_context.lock().unwrap(),
... so TLDR I think everything is already working without contention?
But I'm not 100% sure and also I just tested with some casual browsing :) Please let me know what I've missed! Cheers.