Page MenuHomePhabricator

Bug 1595767 - Don't use rayon for glyph rasterization unless there is a lot of glyphs to rasterize
ClosedPublic

Authored by Bert on Dec 12 2019, 9:46 PM.

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

Bert created this revision.Dec 12 2019, 9:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 12 2019, 9:46 PM
Bert added inline comments.Dec 12 2019, 9:48 PM
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.
Final value will depend on measuring the overhead of Rayon versus time spent in raster-glyph scope.

Bert requested review of this revision.Dec 12 2019, 9:49 PM
phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".Dec 12 2019, 9:50 PM
phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".
phab-bot removed a project: Restricted Project.
nical requested changes to this revision.Dec 13 2019, 9:09 AM

We also need a font context on the render backend thread to register/unregister fonts with. see add_font, etc.

This revision now requires changes to proceed.Dec 13 2019, 9:09 AM
Bert added a comment.EditedDec 13 2019, 6:09 PM

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.

nical accepted this revision.Jan 7 2020, 5:30 PM

Indeed! I forgot there was a shared context.

This revision is now accepted and ready to land.Jan 7 2020, 5:30 PM