Millions of heap allocations in GetFontMetricsForStyleContext() on Treeherder

RESOLVED FIXED in mozilla37

Status

()

Core
Layout: Text
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: jfkthame)

Tracking

unspecified
mozilla37
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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|.
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.)
(Assignee)

Comment 2

3 years ago
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.
(Assignee)

Comment 3

3 years ago
Created attachment 8537153 [details] [diff] [review]
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.
Attachment #8537153 - Flags: review?(dbaron)
(Assignee)

Updated

3 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
(Assignee)

Comment 4

3 years ago
Tryserver results seem to suggest this may give a measurable Tp5 win:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1f08eb6c8d9d
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?
Attachment #8537153 - Flags: review?(dbaron) → review+
Blocks: 1112352
(Assignee)

Comment 6

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/41a8532f3fc3
Target Milestone: --- → mozilla37
(Reporter)

Comment 7

3 years ago
I have confirmed that this is no longer showing up in the DMD output. Thank you, jkew.
https://hg.mozilla.org/mozilla-central/rev/41a8532f3fc3
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Reporter)

Comment 9

3 years ago
jkew, did you end up seeing any email about Talos improvements?
Flags: needinfo?(jfkthame)
(Assignee)

Comment 10

3 years ago
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 <jkew@mozilla.com> - 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?)
Flags: needinfo?(jfkthame)
(Reporter)

Comment 11

3 years ago
Excellent!

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

Comment 12

3 years ago
Filed that as bug 1113069.
You need to log in before you can comment on or make changes to this bug.