Closed Bug 1369526 Opened 3 years ago Closed 3 years ago

Provide a fast-path to accelerate font selection in gfxFontGroup::ComputeRanges for the most common case

Categories

(Core :: Graphics: Text, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame, NeedInfo)

Details

Attachments

(1 file)

gfxFontGroup::FindFontForChar tries to optimize the most common cases of font selection, and return as quickly as it can. However, we can do even better for the most common case (the first font in CSS font-family supports the current character, and the character is not a joiner or diacritic etc that requires special treatment) by processing inline within ComputeRanges and avoiding the function call altogether.
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Comment on attachment 8873606 [details] [diff] [review]
Add a fast-path in gfxFontGroup::ComputeRanges to avoid calling FindFontForChar when possible

Review of attachment 8873606 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxTextRun.cpp
@@ +3167,5 @@
> +                    && ch != NARROW_NO_BREAK_SPACE
> +                    && !gfxFontUtils::IsJoinControl(ch)
> +                    && !gfxFontUtils::IsJoinCauser(prevCh)
> +                    && !gfxFontUtils::IsVarSelector(ch)))) {
> +            matchType = gfxTextRange::kFontGroup;

Can this code be refactored so these conditions are not duplicated with FindFontForChar? i.e. Can we early exit from FindFontForChar or make it a two stage process?
Flags: needinfo?(jfkthame)
(In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> Comment on attachment 8873606 [details] [diff] [review]
> Add a fast-path in gfxFontGroup::ComputeRanges to avoid calling
> FindFontForChar when possible
> 
> Review of attachment 8873606 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxTextRun.cpp
> @@ +3167,5 @@
> > +                    && ch != NARROW_NO_BREAK_SPACE
> > +                    && !gfxFontUtils::IsJoinControl(ch)
> > +                    && !gfxFontUtils::IsJoinCauser(prevCh)
> > +                    && !gfxFontUtils::IsVarSelector(ch)))) {
> > +            matchType = gfxTextRange::kFontGroup;
> 
> Can this code be refactored so these conditions are not duplicated with
> FindFontForChar? i.e. Can we early exit from FindFontForChar or make it a
> two stage process?

I don't see a good way to do that... FindFontForChar does try to exit as early as it can, but here I'm attempting to avoid calling it at all for the most common case, but we need to check these conditions to make sure it's a valid shortcut.

We could avoid the potential duplication of these tests if we moved this inside FindFontForChar as its first block, but I think that's inferior overall for two reasons: first, it means we're always calling the (large) FindFontForChar method, which is too big to comfortably inline and so we'll probably get worse instruction cache locality, and second, FindFontForChar will always have to do multiple tests here, whereas by inlining this in ComputeRanges, they'll be optimized away from the 8-bit instantiation of the method. To get that benefit within FindFontForChar, we'd need to either make it yet another template method, leading to code bloat, or pass in (and then test) an extra parameter `aTextIs8Bit`.

So at this point, I think this is as good a balance as I can see. Profiling indicated that it does measurably reduce the total time we spend in ComputeRanges during textrun construction.
Flags: needinfo?(jfkthame) → needinfo?(jmuizelaar)
Here are profiles from my local build, loading the HTML5 spec (from a local file). Filtering each profile for ComputeRanges shows the gains in this area, first from bug 1364224 which eliminates a lot of redundant refcount twiddling, and then from adding this patch on top:

  Current trunk code: http://bit.ly/2tp3tfv (ComputeRanges: 380ms)
  With bug 1364224:   http://bit.ly/2sPGxsS (ComputeRanges: 304ms)
  Plus this patch:    http://bit.ly/2tplrhW (ComputeRanges: 204ms)

(These are profiles of loading the page in a newly-launched browser, but in each case they represent "best" timings achieved after running the test several times to warm up any relevant OS-level caches etc.)
Attachment #8873606 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca5f7819cada183c3f2b0393238b3d04ce837971
Bug 1369526 - Add a fast-path in gfxFontGroup::ComputeRanges to avoid calling FindFontForChar when possible. r=jrmuizel
https://hg.mozilla.org/mozilla-central/rev/ca5f7819cada
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.