1111879 - Millions of heap allocations in GetFontMetricsForStyleContext() on Treeherder

Status: RESOLVED FIXED

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

Description

[Nicholas Nethercote [inactive]]

I used DMD to profile Firefox while viewing Treeherder -- just scrolling up and
down for 20 or 30 seconds. GetFontMetricsForStyleContext() is causing a huge
number of small heap allocations:

> Cumulative {
>   2,486,791 blocks in heap block record 1 of 15,540
>   278,201,552 bytes (258,339,128 requested / 19,862,424 slop)
>   Individual block sizes: 112 x 2,482,803; 32 x 3,988
>   7.24% of the heap (7.24% cumulative)
>   Allocated at {
>     #01: nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::EnsureCapacity(unsigned long, unsigned long) (/home/njn/moz/mi4/o64dmd/media/libstagefright/../../dist/include/nsTArray-inl.h:135)
>     #02: mozilla::FontFamilyName* nsTArray_Impl<mozilla::FontFamilyName, nsTArrayInfallibleAllocator>::AppendElements<mozilla::FontFamilyName>(mozilla::FontFamilyName const*, unsigned long) (/home/njn/moz/mi4/o64dmd/gfx/src/../../dist/include/nsTArray.h:1313)
>     #03: FontFamilyList (/home/njn/moz/mi4/o64dmd/gfx/src/../../dist/include/gfxFontFamilyList.h:197)
>     #04: nsFont (/home/njn/moz/mi4/o64dmd/gfx/src/../../../gfx/src/nsFont.cpp:66)
>     #05: nsLayoutUtils::GetFontMetricsForStyleContext(nsStyleContext*, nsFontMetrics**, float) (/home/njn/moz/mi4/o64dmd/layout/base/../../../layout/base/nsLayoutUtils.cpp:3758)
>   }
> }

Almost 2.5 million allocations, which is 17% of all allocations in the child
process!

The relevant line is this:

> nsFont font = aStyleContext->StyleFont()->mFont;

which takes us into nsFont's copy constructor, which initializes |fontList| and
so takes us into FontFamilyList's copy constructor, which initializes
|mFontList|, which takes us into nsTArray<FontFamilyName>'s copy constructor,
which does the allocation.

This looks like the kind of thing that could be avoidable with a small change.
|font| is a copy; its |size| member is modified if |aInflation != 1| (which
never happens on my machine); it is only used briefly -- it's passed to
GetMetricsFor() -- and it looks like its only used in a font equality
comparison. Maybe the copy could be avoided, or a shallow copy could be done
instead when |aInflation != 1|.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We could fix the simple case just within this function.

But I wonder if we should instead reference-count the family array within the nsFont (and maybe the other arrays too).  (FontFamilyList could have a DEBUG-only mImmutable that we make true once it's set up, so we can assert we don't mutate shared ones.)

Comment 2

[Jonathan Kew [:jfkthame]]

To some extent, I guess this is a regression from bug 280443; prior to that, the font family list was stored as an nsString, which would have been cheaper to copy. (It had other drawbacks, though, so I'm not suggesting we want to go back there.)

Given that this is a pretty hot piece of code, I think it's worth optimizing locally for inflation==1.0; in that case, we can simply avoid making a local nsFont at all.

Refcounting the arrays within nsFont might also be worthwhile; in particular, it would benefit the case where inflation!=1.0, which is likely to be common on mobile.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: Avoid copying an nsFont when we don't need to modify it locally

This should optimize the common case on desktop. Even if we decide to go down the refcounted-arrays path in nsFont for a more general solution, I think it'll be worth having this change, as it avoids constructing the local variable altogether here.

Comment 4

[Jonathan Kew [:jfkthame]]

Tryserver results seem to suggest this may give a measurable Tp5 win:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1f08eb6c8d9d

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8537153 [details] [diff] [review]
Avoid copying an nsFont when we don't need to modify it locally

>-  // We need to not run font.size through floats when it's large since
>-  // doing so would be lossy.  Fortunately, in such cases, aInflation is
>-  // guaranteed to be 1.0f.

Please preserve this comment.

r=dbaron with that.

Maybe also worth filing a followup bug on the reference-counting?

Comment 6

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/41a8532f3fc3

Comment 7

[Nicholas Nethercote [inactive]]

I have confirmed that this is no longer showing up in the DMD output. Thank you, jkew.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/41a8532f3fc3

Comment 9

[Nicholas Nethercote [inactive]]

jkew, did you end up seeing any email about Talos improvements?

Comment 10

[Jonathan Kew [:jfkthame]]

Yes, for example:

Improvement: Mozilla-Inbound - Tp5 Optimized - MacOSX 10.6 (rev4) - 2.21% decrease
----------------------------------------------------------------------------------
    Previous: avg 325.888 stddev 1.912 of 12 runs up to revision 1853501bdde1
    New     : avg 318.695 stddev 0.960 of 12 runs since revision 41a8532f3fc3
    Change  : -7.193 (2.21% / z=3.761)
    Graph   : http://mzl.la/1wFr7S9

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1853501bdde1&tochange=41a8532f3fc3

Changesets:
  * http://hg.mozilla.org/integration/mozilla-inbound/rev/41a8532f3fc3
    : Jonathan Kew <[e-mail]> - Bug 1111879 - Avoid copying an nsFont when we don't need to modify it locally. r=dbaron
    : http://bugzilla.mozilla.org/show_bug.cgi?id=1111879

Bugs:
  * http://bugzilla.mozilla.org/show_bug.cgi?id=1111879 - Millions of heap allocations in GetFontMetricsForStyleContext() on Treeherder


And similar reports from a couple of other platforms/tests, e.g.

Improvement: Mozilla-Inbound-Non-PGO - a11y Row Major MozAfterPaint - WINNT 6.2 x64 - 4.48% decrease
Improvement: Mozilla-Inbound-Non-PGO - a11y Row Major MozAfterPaint - Ubuntu HW 12.04 x64 - 6.07% decrease
Improvement: Mozilla-Inbound-Non-PGO - a11y Row Major MozAfterPaint - Ubuntu HW 12.04 - 5.72% decrease

(Interesting how the a11y test seems to be especially sensitive to this. Fwiw, this was the same test that showed a regression in bug 1104905, when no other test reported anything. I guess it exercises layout more intensively than any of our other tests?)

Comment 11

[Nicholas Nethercote [inactive]]

Excellent!

Can you file the follow-up that dbaron requested? Thank you.

Comment 12

[Jonathan Kew [:jfkthame]]

Filed that as bug 1113069.