Reduce time spent in nsLayoutUtils::GetFontMetricsForComputedStyle

RESOLVED FIXED in Firefox 65

Status

()

P3
normal
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: jrmuizel, Assigned: jfkthame)

Tracking

(Blocks: 1 bug)

unspecified
mozilla65
Points:
---

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 months ago
GetFontMetricsForComputedStyle accounts for ~20% of the nsTextFrame::DrawText with WebRender. It isn't exactly clear to me yet what work is happening here but it seems like it should be possible to do better.
(Reporter)

Updated

4 months ago
Blocks: 1509158
(Reporter)

Comment 1

4 months ago
Jonathan can you elaborate on what work is happening here and if it's necessary to happen when we call DrawText?
Flags: needinfo?(jfkthame)
Priority: -- → P3
We need the nsFontMetrics (and gfxFontGroup which it references) for the frame's style whenever we draw, in order to get things like the extents for highlighting, or to determine whether there's a pending user font which means we should be temporarily hiding the text.

But AFAICS, each time we draw (or measure) we instantiate a PropertyProvider, which in turn calls nsLayoutUtils to get the nsFontMetrics (and hence gfxFontGroup) that corresponds to the current style. That seems inefficient: we ought to be able to cache this in the textframe and avoid doing the work to re-create it (or find it in the device context's cache) every time.
Flags: needinfo?(jfkthame)
Something like this should basically remove GetFontMetricsForComputedStyle from the DrawText code path, AFAICS. (My local profile wasn't showing anything like 20% of the time there -- more like 1-2% -- but with this patch it no longer appears at all.) Could you try this with your testcase and see if it eliminates the issue?
Attachment #9027074 - Flags: feedback?(jmuizelaar)
(Reporter)

Comment 4

4 months ago
I don't remember what page I was profiling this on originally but on https://jrmuizel.github.io/webrender/facebook-refresh.html GetFontMetricsForComputedStyle was 7% nsTextFrame::DrawText and I was saw it go away completely with this patch.
Comment on attachment 9027074 [details] [diff] [review]
Cache an nsFontMetrics reference in nsTextFrame so that we don't have to re-create it on every DrawText call

Good enough; that sounds like what I'd expect to see.

:jwatt, WDYT? The main cost of this patch is that it adds an extra RefPtr to nsTextFrame, so every text frame will be that bit larger, but that seems worthwhile for the sake of a pretty significant boost to DrawText, IMO.

The idea here is to hold a reference to an nsFontMetrics that matches the frame's textrun (the inflated one, which is what we mostly care about measuring and drawing) in the textframe, so we can pass that to the PropertyProvider we create when drawing, instead of forcing it to go and find/create an nsFontMetrics all over again from the style properties.

I've made it drop the nsFontMetrics reference wherever we clear the textRun, in case something may have changed that would invalidate it; not sure if this is strictly necessary in all cases, but it seemed the safe option.
Attachment #9027074 - Flags: feedback?(jmuizelaar) → review?(jwatt)
Component: Graphics: Text → Layout: Text and Fonts
It would be good to know why this is such a large percentage only when we're using WR.

At first glance this seems like a good trade-off but I'm too tired to do a proper review this late in the evening. I'll get to it on Monday, if not tomorrow.
(Reporter)

Comment 7

4 months ago
(In reply to Jonathan Watt [:jwatt] from comment #6)
> It would be good to know why this is such a large percentage only when we're
> using WR.

An uneducated guess is that the we're only recording the glyphs during paint so the other parts of the process end up showing up as a higher percentage. That being said, the same should be true of OMTP, so I'll profile this on Monday to get a better answer.
Comment on attachment 9027074 [details] [diff] [review]
Cache an nsFontMetrics reference in nsTextFrame so that we don't have to re-create it on every DrawText call

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

Looks good, although it would still be good hear what Jeff's profiling indicates is the cause for the higher percentage.
Attachment #9027074 - Flags: review?(jwatt) → review+

Comment 9

4 months ago
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a7a03bc253d
Cache an nsFontMetrics reference in nsTextFrame so that we don't have to re-create it on every DrawText call. r=jwatt

Comment 10

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8a7a03bc253d
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Assignee: nobody → jfkthame
(Reporter)

Comment 11

4 months ago
Just to close the loop here. I was able to reproduce an appreciable amount of time in nsTextFrame::DrawText being spent in GetFontMetricsForComputedStyle on https://jrmuizel.github.io/webrender/facebook-refresh.html with and without WebRender.
Thanks, Jeff!
You need to log in before you can comment on or make changes to this bug.