Closed Bug 1364224 Opened 3 years ago Closed 3 years ago

Too much refcount churn on gfxFont instances

Categories

(Core :: Graphics: Text, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

Details

Attachments

(1 file, 1 obsolete file)

gfxFont is a refcounted class, which is fine, as we typically have a number of textruns and fontgroups that all need to own references to a given gfxFont instance.

However, current code does far too much AddRef/Release-ing of gfxFont instances during the font-matching/textrun-construction process, where it really just needs a transient weak reference to a font that is already owned by the fontgroup, or where the textrun will hold a strong reference once we've determined which font to use for a given glyph run.

By adding some local instrumentation, I found that simply launching the browser and loading https://html.spec.whatwg.org/, then quitting, results in a little over 25 million(!) calls to to gfxFont::AddRef. (And, obviously, the same number of gfxFont::Release.) That seems.....excessive.

Replacing the use of RefPtr<gfxFont> and already_AddRefed<gfxFont> with plain gfxFont* in key places where a strong reference is not needed reduces the number of AddRef/Release calls from 25M to about 200K. (Yes, that's a 99%+ reduction in refcount churn.)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Comment on attachment 8866975 [details] [diff] [review]
Reduce refcount churn on gfxFont by using raw pointers where no strong ownership is needed

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

I'm confused by this. Returning an already_AddRefed shouldn't cause any calls to AddRef/Release and already_AddRefed should assign to RefPtr without any AddRef/Release. Can you narrow down where the change is coming from?
Right, those things in themselves aren't a problem. The win comes from things like changing RefPtr<gfxFont> to a raw pointer in gfxTextRange (which is OK, because gfxTextRange is just used in a local stack-based array in gfxTextRun::ComputeRanges, and doesn't need to hold strong references; when the fonts go into GlyphRun records in the textrun, -that- still holds strong refs).

The rest flows from there... basically, we want to defer addref'ing the font until we really need to store it somewhere persistent (in a fontgroup or textrun), rather than addref'ing early and then having to release again if it turns out we don't need to hold on. In particular, making gfxFontGroup::FindFontForChar return a non-addref'd pointer is key, because most of the time it returns the same font as it did for the previous character, so we don't need to create a new glyphrun, and so we don't need a new strong reference to the font.
The original patch turned out to have a possible leak in an edge case, as found by tryserver on Win7. Should be fixed in this version, I believe. New try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a057aad4758a461aa591e5274aff2315e6a75e62.
Attachment #8867364 - Flags: review?(jmuizelaar)
Attachment #8866975 - Attachment is obsolete: true
Attachment #8866975 - Flags: review?(jmuizelaar)
Comment on attachment 8867364 [details] [diff] [review]
Reduce refcount churn on gfxFont by using raw pointers where no strong ownership is needed

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

I still don't understand. This patch doesn't seem to change the font in gfxTextRange to not be a RefPtr<gfxFont>. 

Overall I'm pretty hesitant to take this patch without something more concrete showing its advantage or some better kind of mechanism ensuring its correctness. The fact that the first version had a leak already scares me.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> Comment on attachment 8867364 [details] [diff] [review]
> Reduce refcount churn on gfxFont by using raw pointers where no strong
> ownership is needed
> 
> Review of attachment 8867364 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I still don't understand. This patch doesn't seem to change the font in
> gfxTextRange to not be a RefPtr<gfxFont>. 

Yes, that's the piece I had to revert to avoid a leak in the edge case where FindFontForChar returns a newly-instantiated font that has not been used before (so it's not already owned by anyone, there are no strong refs to it), but then we don't end up actually creating a GlyphRun using that font at all.

(That can happen if InitScriptRun ends up creating a modified font for synthetic small-caps or super/subscript rendering, and uses that for all the text it shapes, so the original result from FindFontForChar goes unused. If it was a newly-created font that nobody else owns, it will then leak.)

A possible fix would have been to assign the original matchedFont to a RefPtr at the point where we decide to create a synthetic-style variant, but as there are a couple of places that happens it seems a bit fragile/error-prone. Reverting to a RefPtr in gfxTextRange means that we're guaranteed to take (and eventually release) a strong reference for every font that ComputeRanges chooses; this does increase our refcounting somewhat, but we still get a 98+% reduction compared to current trunk code -- instrumentation shows that it's reduced from 25M to 400K during loading the HTML5 spec.

The key observation here is that currently, gfxFontGroup::FindFontForChar (which is called for every character of text) returns an already_AddRefed pointer, but in the vast majority of cases, all we're going to do is append it to the current glyph run (i.e. we'll keep using the same font), and throw away that new reference. Making FindFontForChar return a raw pointer, and only AddRef'ing it when we record it in a gfxTextRange, means that we only AddRef once per font run instead of once per character.

> Overall I'm pretty hesitant to take this patch without something more
> concrete showing its advantage or some better kind of mechanism ensuring its
> correctness. The fact that the first version had a leak already scares me.

OTOH, the fact that our test suite detected the leak gives me confidence in our checking. Font instances refer to gfxFontEntry records in the platform font list, which own strings (names), etc.; we can't leak these things without automation noticing it. (And we can't double-Release them without hitting assertions, either.)

gfxFont::AddRef/Release aren't likely to show up directly in profiles, as they're tiny (and inlined), but nevertheless they're not entirely free.
I see now how this works. I'm going to think about it a bit more.
<ping> (did you have a chance to think about this again?)
Flags: needinfo?(jmuizelaar)
No, I haven't thought about it through again yet. I will though :) Sorry for the wait.
Flags: needinfo?(jmuizelaar)
Attachment #8867364 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7d6193f31a35c82d54877de472e293112713d67
Bug 1364224 - Reduce refcount churn on gfxFont by using raw pointers where no strong ownership is needed. r=jrmuizel
https://hg.mozilla.org/mozilla-central/rev/d7d6193f31a3
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.