Millions of heap allocations in GetFontMetricsForStyleContext() on Treeherder

RESOLVED FIXED in mozilla37



Layout: Text
3 years ago
3 years ago


(Reporter: njn, Assigned: jfkthame)


Dependency tree / graph

Firefox Tracking Flags

(Not tracked)



(1 attachment)



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

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.)

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.

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)


3 years ago
Assignee: nobody → jfkthame

Comment 4

3 years ago
Tryserver results seem to suggest this may give a measurable Tp5 win:
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

Comment 6

3 years ago
Target Milestone: --- → mozilla37

Comment 7

3 years ago
I have confirmed that this is no longer showing up in the DMD output. Thank you, jkew.
Last Resolved: 3 years ago
Resolution: --- → FIXED

Comment 9

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

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   :

Changeset range:

    : Jonathan Kew <> - Bug 1111879 - Avoid copying an nsFont when we don't need to modify it locally. r=dbaron

  * - 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)

Comment 11

3 years ago

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

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.