1509167 - Reduce time spent in nsLayoutUtils::GetFontMetricsForComputedStyle

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect, P3)

Description

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Jonathan can you elaborate on what work is happening here and if it's necessary to happen when we call DrawText?

Comment 2

[Jonathan Kew [:jfkthame]]

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.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: Cache an nsFontMetrics reference in nsTextFrame so that we don't have to re-create it on every DrawText call

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?

Comment 4

[Jeff Muizelaar [:jrmuizel]]

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 5

[Jonathan Kew [:jfkthame]]

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.

Comment 6

[Jonathan Watt [:jwatt]]

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.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

(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 8

[Jonathan Watt [:jwatt]]

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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/8a7a03bc253d

Comment 11

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 12

[Jonathan Watt [:jwatt]]

Thanks, Jeff!