1100738 - Lots of memory allocations under gfxPangoFontGroup::GetBaseFont()

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Nicholas Nethercote [inactive]]

I did some heap profiling of pdf.js opening a very detailed map with *lots* of
tiny text labels on it. The following data came up; it's a measure of
*cumulative* (not live) heap allocations that have the same allocation stack:

> Cumulative {
>   21,474 blocks in heap block record 4 of 11,966
>   351,830,016 bytes (288,782,352 requested / 63,047,664 slop)
>   Individual block sizes: 16,384 x 21,474
>   3.62% of the heap (58.03% cumulative)
>   Allocated at {
>     #01: FcFontSetSort (/usr/lib/x86_64-linux-gnu/libfontconfig.so.1)
>     #02: gfxFcFontSet::SortPreferredFonts(bool&) (/home/njn/moz/mi5/co64dmd/gfx/thebes/../../../gfx/thebes/gfxPangoFonts.cpp:1102)
>     #03: gfxFcFontSet (/home/njn/moz/mi5/co64dmd/gfx/thebes/../../../gfx/thebes/gfxPangoFonts.cpp:707)
>     #04: gfxPangoFontGroup::MakeFontSet(_PangoLanguage*, double, nsAutoRef<_FcPattern>*) (/home/njn/moz/mi5/co64dmd/gfx/thebes/../../../gfx/thebes/gfxPangoFonts.cpp:1368)
>     #05: gfxPangoFontGroup::GetBaseFontSet() (/home/njn/moz/mi5/co64dmd/gfx/thebes/../../../gfx/thebes/gfxPangoFonts.cpp:1853)
>     #06: gfxPangoFontGroup::GetBaseFont() (/home/njn/moz/mi5/co64dmd/gfx/thebes/../../../gfx/thebes/gfxPangoFonts.cpp:1300)
>     #07: void gfxFontGroup::InitScriptRun<char16_t>(gfxContext*, gfxTextRun*, char16_t const*, unsigned int, unsigned int, int) (/home/njn/moz/mi5/co64dmd/gfx/thebes/../../../gfx/thebes/gfxTextRun.cpp:2
> 317)
>     #08: void gfxFontGroup::InitTextRun<char16_t>(gfxContext*, gfxTextRun*, char16_t const*, unsigned int) (/home/njn/moz/mi5/co64dmd/gfx/thebes/../../../gfx/thebes/gfxTextRun.cpp:2227)
>     #09: gfxFontGroup::MakeTextRun(char16_t const*, unsigned int, gfxTextRunFactory::Parameters const*, unsigned int) (/home/njn/moz/mi5/co64dmd/gfx/thebes/../../../gfx/thebes/gfxTextRun.cpp:2120)
>     #10: gfxTextRun* gfxFontGroup::MakeTextRun<char16_t>(char16_t const*, unsigned int, gfxContext*, int, unsigned int) (/home/njn/moz/mi5/co64dmd/layout/mathml/../../dist/include/gfxTextRun.h:791)
>     #11: mozilla::dom::CanvasBidiProcessor::SetText(char16_t const*, int, nsBidiDirection) (/home/njn/moz/mi5/co64dmd/dom/canvas/../../../dom/canvas/CanvasRenderingContext2D.cpp:3255)
>     #12: nsBidiPresUtils::ProcessText(char16_t const*, int, unsigned char, nsPresContext*, nsBidiPresUtils::BidiProcessor&, nsBidiPresUtils::Mode, nsBidiPositionResolve*, int, int*, nsBidi*) (/home/njn/
> moz/mi5/co64dmd/layout/base/../../../layout/base/nsBidiPresUtils.cpp:1937)
>     #13: mozilla::dom::CanvasRenderingContext2D::DrawOrMeasureText(nsAString_internal const&, float, float, mozilla::dom::Optional<double> const&, mozilla::dom::CanvasRenderingContext2D::TextDrawOperati
> on, float*) (/home/njn/moz/mi5/co64dmd/dom/canvas/../../../dom/canvas/CanvasRenderingContext2D.cpp:3629)
>     #14: mozilla::dom::CanvasRenderingContext2D::MeasureText(nsAString_internal const&, mozilla::ErrorResult&) (/home/njn/moz/mi5/co64dmd/dom/canvas/../../../dom/canvas/CanvasRenderingContext2D.cpp:3150
> )   
>     #15: mozilla::dom::CanvasRenderingContext2DBinding::measureText(JSContext*, JS::Handle<JSObject*>, mozilla::dom::CanvasRenderingContext2D*, JSJitMethodCallArgs const&) (/home/njn/moz/mi5/co64dmd/dom
> /bindings/./CanvasRenderingContext2DBinding.cpp:3890)
>     #16: mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) (/home/njn/moz/mi5/co64dmd/dom/bindings/../../../dom/bindings/BindingUtils.cpp:2432)
>     #17: ??? (???:???)
>   }
> } 

I don't know this code at all, but I'd guess that stack frames #07..#15 are
about creating a text run, and #01..#06 are some font organisation stuff that
looks like it should only be done once, rather than once per text run.

Comment 1

[Nicholas Nethercote [inactive]]

Here are a two other, similar ones from the same profile.

> Cumulative {
>   ~32,964 blocks in heap block record 8 of 11,966
>   ~134,921,652 bytes (~134,921,652 requested / ~0 slop)
>   Individual block sizes: ~4,093 x 32,964
>   1.39% of the heap (65.44% cumulative)
>   Allocated at {
>     #01: __GI___strdup (/build/buildd/eglibc-2.19/string/strdup.c:44)
>     #02: FcValueSave (/usr/lib/x86_64-linux-gnu/libfontconfig.so.1)
>     #03: FcConfigAddRule (/usr/lib/x86_64-linux-gnu/libfontconfig.so.1)
>     #04: FcConfigSubstituteWithPat (/usr/lib/x86_64-linux-gnu/libfontconfig.so.1)
>     #05: PrepareSortPattern(_FcPattern*, double, double, bool) (/home/njn/moz/mi5/co64dmd/gfx/thebes/../../../gfx/thebes/gfxPangoFonts.cpp:1208)
>     #06: gfxPangoFontGroup::MakeFontSet(_PangoLanguage*, double, nsAutoRef<_FcPattern>*) (/home/njn/moz/mi5/co64dmd/gfx/thebes/../../../gfx/thebes/gfxPangoFonts.cpp:1366)
>     #07: gfxPangoFontGroup::GetBaseFontSet() (/home/njn/moz/mi5/co64dmd/gfx/thebes/../../../gfx/thebes/gfxPangoFonts.cpp:1853)
>     #08: gfxPangoFontGroup::GetBaseFont() (/home/njn/moz/mi5/co64dmd/gfx/thebes/../../../gfx/thebes/gfxPangoFonts.cpp:1300)
>     #09: void gfxFontGroup::InitScriptRun<char16_t>(gfxContext*, gfxTextRun*, char16_t const*, unsigned int, unsigned int, int) (/home/njn/moz/mi5/co64dmd/gfx/thebes/../../../gfx/thebes/gfxTextRun.cpp:2317)
>     #10: void gfxFontGroup::InitTextRun<char16_t>(gfxContext*, gfxTextRun*, char16_t const*, unsigned int) (/home/njn/moz/mi5/co64dmd/gfx/thebes/../../../gfx/thebes/gfxTextRun.cpp:2227)
>     #11: gfxFontGroup::MakeTextRun(char16_t const*, unsigned int, gfxTextRunFactory::Parameters const*, unsigned int) (/home/njn/moz/mi5/co64dmd/gfx/thebes/../../../gfx/thebes/gfxTextRun.cpp:2120)
>     #12: gfxTextRun* gfxFontGroup::MakeTextRun<char16_t>(char16_t const*, unsigned int, gfxContext*, int, unsigned int) (/home/njn/moz/mi5/co64dmd/layout/mathml/../../dist/include/gfxTextRun.h:791)
>     #13: mozilla::dom::CanvasBidiProcessor::SetText(char16_t const*, int, nsBidiDirection) (/home/njn/moz/mi5/co64dmd/dom/canvas/../../../dom/canvas/CanvasRenderingContext2D.cpp:3255)
>     #14: nsBidiPresUtils::ProcessText(char16_t const*, int, unsigned char, nsPresContext*, nsBidiPresUtils::BidiProcessor&, nsBidiPresUtils::Mode, nsBidiPositionResolve*, int, int*, nsBidi*) (/home/njn/moz/mi5/co64dmd/layout/base/../../../layout/base/nsBidiPresUtils.cpp:1937)
>     #15: mozilla::dom::CanvasRenderingContext2D::DrawOrMeasureText(nsAString_internal const&, float, float, mozilla::dom::Optional<double> const&, mozilla::dom::CanvasRenderingContext2D::TextDrawOperation, float*) (/home/njn/moz/mi5/co64dmd/dom/canvas/../../../dom/canvas/CanvasRenderingContext2D.cpp:3629)
>     #16: mozilla::dom::CanvasRenderingContext2D::MeasureText(nsAString_internal const&, mozilla::ErrorResult&) (/home/njn/moz/mi5/co64dmd/dom/canvas/../../../dom/canvas/CanvasRenderingContext2D.cpp:3150)
>     #17: mozilla::dom::CanvasRenderingContext2DBinding::measureText(JSContext*, JS::Handle<JSObject*>, mozilla::dom::CanvasRenderingContext2D*, JSJitMethodCallArgs const&) (/home/njn/moz/mi5/co64dmd/dom/bindings/./CanvasRenderingContext2DBinding.cpp:3890)
>     #18: mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) (/home/njn/moz/mi5/co64dmd/dom/bindings/../../../dom/bindings/BindingUtils.cpp:2432)
>     #19: ??? (???:???)
>   }
> }
> 
> Live {
>   2,378 blocks in heap block record 11 of 11,966
>   58,441,728 bytes (51,155,536 requested / 7,286,192 slop)
>   Individual block sizes: 24,576 x 2,378
>   0.60% of the heap (67.67% cumulative)
>   Allocated at {
>     #01: FcFontSetSort (/usr/lib/x86_64-linux-gnu/libfontconfig.so.1)
>     #02: gfxFcFontSet::SortPreferredFonts(bool&) (/home/njn/moz/mi5/co64dmd/gfx/thebes/../../../gfx/thebes/gfxPangoFonts.cpp:1102)
>     #03: gfxFcFontSet (/home/njn/moz/mi5/co64dmd/gfx/thebes/../../../gfx/thebes/gfxPangoFonts.cpp:707)
>     #04: gfxPangoFontGroup::MakeFontSet(_PangoLanguage*, double, nsAutoRef<_FcPattern>*) (/home/njn/moz/mi5/co64dmd/gfx/thebes/../../../gfx/thebes/gfxPangoFonts.cpp:1368)
>     #05: gfxPangoFontGroup::GetBaseFontSet() (/home/njn/moz/mi5/co64dmd/gfx/thebes/../../../gfx/thebes/gfxPangoFonts.cpp:1853)
>     #06: gfxPangoFontGroup::GetBaseFont() (/home/njn/moz/mi5/co64dmd/gfx/thebes/../../../gfx/thebes/gfxPangoFonts.cpp:1300)
>     #07: void gfxFontGroup::InitScriptRun<char16_t>(gfxContext*, gfxTextRun*, char16_t const*, unsigned int, unsigned int, int) (/home/njn/moz/mi5/co64dmd/gfx/thebes/../../../gfx/thebes/gfxTextRun.cpp:2317)
>     #08: void gfxFontGroup::InitTextRun<char16_t>(gfxContext*, gfxTextRun*, char16_t const*, unsigned int) (/home/njn/moz/mi5/co64dmd/gfx/thebes/../../../gfx/thebes/gfxTextRun.cpp:2227)
>     #09: gfxFontGroup::MakeTextRun(char16_t const*, unsigned int, gfxTextRunFactory::Parameters const*, unsigned int) (/home/njn/moz/mi5/co64dmd/gfx/thebes/../../../gfx/thebes/gfxTextRun.cpp:2120)
>     #10: gfxTextRun* gfxFontGroup::MakeTextRun<char16_t>(char16_t const*, unsigned int, gfxContext*, int, unsigned int) (/home/njn/moz/mi5/co64dmd/layout/mathml/../../dist/include/gfxTextRun.h:791)
>     #11: mozilla::dom::CanvasBidiProcessor::SetText(char16_t const*, int, nsBidiDirection) (/home/njn/moz/mi5/co64dmd/dom/canvas/../../../dom/canvas/CanvasRenderingContext2D.cpp:3255)
>     #12: nsBidiPresUtils::ProcessText(char16_t const*, int, unsigned char, nsPresContext*, nsBidiPresUtils::BidiProcessor&, nsBidiPresUtils::Mode, nsBidiPositionResolve*, int, int*, nsBidi*) (/home/njn/moz/mi5/co64dmd/layout/base/../../../layout/base/nsBidiPresUtils.cpp:1937)
>     #13: mozilla::dom::CanvasRenderingContext2D::DrawOrMeasureText(nsAString_internal const&, float, float, mozilla::dom::Optional<double> const&, mozilla::dom::CanvasRenderingContext2D::TextDrawOperation, float*) (/home/njn/moz/mi5/co64dmd/dom/canvas/../../../dom/canvas/CanvasRenderingContext2D.cpp:3629)
>     #14: mozilla::dom::CanvasRenderingContext2D::FillText(nsAString_internal const&, double, double, mozilla::dom::Optional<double> const&, mozilla::ErrorResult&) (/home/njn/moz/mi5/co64dmd/dom/canvas/../../../dom/canvas/CanvasRenderingContext2D.cpp:3132)
>     #15: mozilla::dom::CanvasRenderingContext2DBinding::fillText(JSContext*, JS::Handle<JSObject*>, mozilla::dom::CanvasRenderingContext2D*, JSJitMethodCallArgs const&) (/home/njn/moz/mi5/co64dmd/dom/bindings/./CanvasRenderingContext2DBinding.cpp:3788)
>     #16: ??? (???:???)
>   }
> }

Comment 2

[Karl Tomlinson (:karlt)]

(In reply to Nicholas Nethercote [:njn] from comment #0)
> I don't know this code at all, but I'd guess that stack frames #07..#15 are
> about creating a text run, and #01..#06 are some font organisation stuff that
> looks like it should only be done once, rather than once per text run.

Yes, the gfxFontGroup is usually reused to avoid repeating those steps.  Every
other consumer of gfxPlatform::CreateFontGroup() does that with a small cache
through nsDeviceContext::GetMetricsFor().

Part of the problem may be related to the comment at the top of
http://hg.mozilla.org/mozilla-central/annotate/7913c9392c5f/dom/canvas/CanvasRenderingContext2D.cpp#l2956
but AFAIK nsDeviceContext::GetMetricsFor() could be used here.

A similar issue was fixed in
http://hg.mozilla.org/mozilla-central/rev/165df837579e

Comment 3

[Nicholas Nethercote [inactive]]

I redid this measurement but with sampling off, which means I have exact data about small heap allocations... and things are *much* worse than they first appear. libfontconfig accounts for more than two-thirds of all heap allocations done by the child process! That's an incredible number. Most of them
are small -- 8 or 16 or 32 bytes -- but the contention for the global heap lock will be extreme.

Here's a sample of the output from |perf|:

> 3.80%      Web Content  libc-2.19.so                   [.] __strchr_sse2
> 2.47%      Web Content  libfontconfig.so.1.8.0         [.] FcConfigSubstituteWithPat
> 1.95%      Web Content  libxul.so                      [.] _ZN12PLDHashTable11SearchTableEPKvj15PLDHashOperator
> 1.60%      Web Content  libfreetype.so.6.11.1          [.] TT_RunIns
> 1.21%      Web Content  libpthread-2.19.so             [.] pthread_mutex_lock
> 1.19%      Web Content  libpthread-2.19.so             [.] __lll_lock_elision
> 1.13%      Web Content  libxul.so                      [.] _cairo_bentley_ottmann_tessellate_polygon
> 1.07%      Web Content  libpthread-2.19.so             [.] pthread_mutex_unlock
> 1.04%      Web Content  libxul.so                      [.] _ZN20nsDisplayListBuilder31ResetMarkedFramesForDisplayListEv
> 0.90%      Web Content  plugin-container               [.] arena_dalloc
> 0.89%       DOM Worker  libxul.so                      [.] _ZN2js7baseops11GetPropertyEP9JSContextN2JS6HandleIPNS_12NativeObjectEEENS4_IP8JSObjectEENS4_I4jsidEENS3_13MutableHandleINS3_5ValueEEE
> 0.83%      Web Content  libxul.so                      [.] _ZN7mozilla18FramePropertyTable3GetEPK8nsIFramePKNS_23FramePropertyDescriptorEPb
> 0.83%       DOM Worker  libxul.so                      [.] _ZN2js5Shape6searchEPNS_16ExclusiveContextEPS0_4jsidPPS3_b
> 0.78%      Web Content  plugin-container               [.] arena_malloc
> 0.67%      Web Content  libxul.so                      [.] _ZN8nsIFrame24GetOverflowAreasPropertyEv
> 0.66%      Web Content  libxul.so                      [.] _ZN8nsIFrame24BuildDisplayListForChildEP20nsDisplayListBuilderPS_RK6nsRectRK16nsDisplayListSetj
> 0.66%      Web Content  libxul.so                      [.] sse2_blt
> 0.63%      Web Content  libxul.so                      [.] _ZNK11nsFrameList9GetLengthEv
> 0.62%      Web Content  libxul.so                      [.] _ZNK8nsIFrame15GetOverflowRectE14nsOverflowType
> 0.61%      Web Content  libfontconfig.so.1.8.0         [.] 0x000000000001e619
> 0.60%      Web Content  libfontconfig.so.1.8.0         [.] FcStrCmpIgnoreCase
> 0.58%      Web Content  libfontconfig.so.1.8.0         [.] 0x000000000001e603
> 0.58%       DOM Worker  libxul.so                      [.] _ZL15fun_hasInstanceP9JSContextN2JS6HandleIP8JSObjectEENS1_13MutableHandleINS1_5ValueEEEPb
> 0.57%      Web Content  libc-2.19.so                   [.] __memcpy_sse2_unaligned
> 0.55%      Web Content  libfontconfig.so.1.8.0         [.] FcStrStrIgnoreCase
> 0.51%      Web Content  libxul.so                      [.] _cairo_uint64_divrem
> 0.51%      Web Content  libfontconfig.so.1.8.0         [.] 0x000000000001e5b0
> 0.50%      Web Content  libxul.so                      [.] _ZNK8nsIFrame13IsTransformedEv
> 0.48%      Web Content  [unknown]                      [k] 0xffffffff813a7757
> 0.46%      Web Content  libxul.so                      [.] _ZN7mozilla18FramePropertyTable6RemoveEP8nsIFramePKNS_23FramePropertyDescriptorEPb
> 0.45%      Web Content  libfontconfig.so.1.8.0         [.] 0x000000000001e5fa
> 0.44%      Web Content  libxul.so                      [.] _cairo_scaled_font_keys_equal
> 0.44%      Web Content  libfontconfig.so.1.8.0         [.] 0x000000000001e5cb
> 0.43%      Web Content  libfontconfig.so.1.8.0         [.] 0x000000000001e5b9
> 0.42%       Compositor  [unknown]                      [k] 0xffffffff813a7b25
> 0.42%      Web Content  libfontconfig.so.1.8.0         [.] 0x000000000001e60e
> 0.42%      Web Content  libfontconfig.so.1.8.0         [.] 0x000000000001e5c0
> 0.41%      Web Content  libc-2.19.so                   [.] strlen
> 0.41%      Web Content  libxul.so                      [.] _ZN2js10ShapeTable6searchE4jsidb
> 0.39%      Web Content  libxul.so                      [.] _cairo_hash_table_lookup
> 0.38%      Web Content  libfontconfig.so.1.8.0         [.] 0x000000000001e61f
> 0.38%      Web Content  libxul.so                      [.] _cairo_int32x32_64_mul
> 0.37%      Web Content  libfontconfig.so.1.8.0         [.] 0x0000000000009960
> 0.35%      Web Content  libfontconfig.so.1.8.0         [.] 0x000000000001e625

libfontconfig appears repeatedly.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: DMD output with no sampling

Oh, I forgot to attach the new DMD output.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (DRAFT) - Use GetMetricsFor() to avoid many Pango font allocations

Here's an attempt to use GetMetricsFor(). I'm quite unsure about a couple of
the parameters, as the comments indicate.

This makes a huge difference on the detailed PDF map I've been profiling:

- Without this patch: rendering the visual-layer + text-layer takes 12 + 20 =
  32 seconds, and 32M cumulative heap allocations taking up 31 GiB are done.

- With this patch: it takes 10 + 6 = 16 seconds, and 10M cumulative heap
  allocations taking up 20 GiB are done.

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 8543195 [details] [diff] [review]
(DRAFT) - Use GetMetricsFor() to avoid many Pango font allocations

>+  nsRefPtr<nsFontMetrics> metrics;
>+  c->DeviceContext()->GetMetricsFor(fontStyle->mFont,       // njn: ?

This should be good apart from the quirk about ignoring the minimum font size.

http://hg.mozilla.org/mozilla-central/annotate/636498d041b5/dom/canvas/CanvasRenderingContext2D.cpp#l3009

I think the best way to keep current behavior is to make a copy of the nsFont and set the size field of the copy to fontStyle->mSize.

>+                                    fontStyle->mLanguage,
>+                                    fontStyle->mExplicitLanguage,
>+                                    gfxFont::eHorizontal,   // njn: ??!

aOrientation is unused for GetThebesFontGroup(), except for selecting
nsFontMetrics from the cache, so yes, eHorizontal should be good.

>+                                    c->GetUserFontSet(),
>+                                    c->GetTextPerfMetrics(),
>+                                    *getter_AddRefs(metrics));

>   CurrentState().fontGroup->SetTextPerfMetrics(c->GetTextPerfMetrics());

SetTextPerfMetrics can be removed now, as it is done in the nsFontMetrics.
There is also some other code above that can be removed.

Comment 7

[Nicholas Nethercote [inactive]]

> I think the best way to keep current behavior is to make a copy of the
> nsFont and set the size field of the copy to fontStyle->mSize.

One problem with that is that copying nsFont isn't cheap -- see bug 1113069. But I just checked and for the PDF I've been measuring those two sizes are always the same, so I can avoid the copy in that case.

> SetTextPerfMetrics can be removed now, as it is done in the nsFontMetrics.
> There is also some other code above that can be removed.

Sorry, which code above can be removed?

Comment 8

[Karl Tomlinson (:karlt)]

(In reply to Nicholas Nethercote [:njn] from comment #7)
> > I think the best way to keep current behavior is to make a copy of the
> > nsFont and set the size field of the copy to fontStyle->mSize.
> 
> One problem with that is that copying nsFont isn't cheap -- see bug 1113069.

Ah.

> But I just checked and for the PDF I've been measuring those two sizes are
> always the same, so I can avoid the copy in that case.

Sounds good.

However, I now see there is also a difference between
CanvasRenderingContext2D::SetFont() and nsFontMetrics::Init() in that the
former uses CSSPixels while the later uses devicePixels.
Maintaining existing behavior and using nsFontMetrics::Init() requires an
adjustment in the font size.

(I expect the gfxFontStyle would ideally contain the size in device pixels but
that would require an adjustment elsewhere, which we don't have right now.)

Even when a copy is needed, I expect copying the nsFont will (or at least can)
be much cheaper than constructing a new gfxFontGroup.

> > There is also some other code above that can be removed.
> 
> Sorry, which code above can be removed?

printerFont and style are unused now.

Comment 9

[Nicholas Nethercote [inactive]]

> However, I now see there is also a difference between
> CanvasRenderingContext2D::SetFont() and nsFontMetrics::Init() in that the
> former uses CSSPixels while the later uses devicePixels.
> Maintaining existing behavior and using nsFontMetrics::Init() requires an
> adjustment in the font size.

How do I make that adjustment? Should I scale resizedFont.size by |AppUnitsPerDevPixel() / AppUnitsPerCSSPixel()|?

> Even when a copy is needed, I expect copying the nsFont will (or at least can)
> be much cheaper than constructing a new gfxFontGroup.

True. I still get 16 seconds for the map rendering even with the nsFont copying.

> printerFont and style are unused now.

|language|, too... although, we now pass |fontStyle->mLanguage| to GetMetricsFor()... should we instead pass |language|?

Comment 10

[Karl Tomlinson (:karlt)]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> > However, I now see there is also a difference between
> > CanvasRenderingContext2D::SetFont() and nsFontMetrics::Init() in that the
> > former uses CSSPixels while the later uses devicePixels.
> > Maintaining existing behavior and using nsFontMetrics::Init() requires an
> > adjustment in the font size.
> 
> How do I make that adjustment? Should I scale resizedFont.size by
> |AppUnitsPerDevPixel() / AppUnitsPerCSSPixel()|?

Yes, that looks right.
These are integers, so multiply before dividing, I guess.

> |language|, too... although, we now pass |fontStyle->mLanguage| to
> GetMetricsFor()... should we instead pass |language|?

In nsStyleFont::Init(), it looks like mLanguage already includes the
default from aPresContext->GetLanguageFromCharset(), so I don't think anything
extra is required there.

https://hg.mozilla.org/mozilla-central/annotate/d480b3542cc2/layout/style/nsStyleStruct.cpp#l152

Comment 11

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (back 19 Jan :karlt) from comment #10)
> (In reply to Nicholas Nethercote [:njn] from comment #9)
> > |language|, too... although, we now pass |fontStyle->mLanguage| to
> > GetMetricsFor()... should we instead pass |language|?
> 
> In nsStyleFont::Init(), it looks like mLanguage already includes the
> default from aPresContext->GetLanguageFromCharset(), so I don't think
> anything extra is required there.
> 
> https://hg.mozilla.org/mozilla-central/annotate/d480b3542cc2/layout/style/
> nsStyleStruct.cpp#l152

Jonathan, do you know whether there is still any need to have this fallback to
GetLanguageFromCharset()?

https://hg.mozilla.org/mozilla-central/annotate/d480b3542cc2/dom/canvas/CanvasRenderingContext2D.cpp#l2999

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: Use GetMetricsFor() to avoid many Pango font allocations

I'm still a bit nervous about the css vs. dev pixels stuff, including the
removal of the "use CSS pixels instead of dev pixels to avoid being affected by
page zoom" comment.

But try is looking good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d34bf390015

Comment 13

[Jonathan Watt [:jwatt]]

(In reply to Karl Tomlinson (back 19 Jan :karlt) from comment #11)
> Jonathan, do you know whether there is still any need to have this fallback
> to GetLanguageFromCharset()?

I'm afraid I don't, no, although it does look like you're correct (it looks unnecessary).

Comment 14

[Karl Tomlinson (:karlt)]

Comment on attachment 8546412 [details] [diff] [review]
Use GetMetricsFor() to avoid many Pango font allocations

>Bug 1100738 - Use GetMetricsFor() to avoid many Pango font allocations.

We don't really use Pango any longer and things are a bit misnamed.  You could
say Fontconfig allocations, or gfxFontGroup constructions, or both.

(In reply to Nicholas Nethercote [:njn] from comment #12)
> I'm still a bit nervous about the css vs. dev pixels stuff, including the
> removal of the "use CSS pixels instead of dev pixels to avoid being affected
> by page zoom" comment.

>-  // use CSS pixels instead of dev pixels to avoid being affected by page zoom

>-                     NSAppUnitsToFloatPixels(fontStyle->mSize, float(aupcp)),

>+  // Convert size from css pixels to device pixels, which is what
>+  // GetMetricsFor() needs.
>+  resizedFont.size =
>+    (fontStyle->mSize * c->AppUnitsPerDevPixel()) / c->AppUnitsPerCSSPixel();

The size is in app units so its not really a conversion from CSS pixels to dev
pixels.  The math is equivalent to the inverse conversion.

I suggest something like this comment:

"Create a font group working in units of CSS pixels instead of the usual
device pixels, to avoid being affected by page zoom.  nsFontMetrics will
convert nsFont size in app units to device pixels for the font group, so
here we first apply to the size the equivalent of a conversion from device
pixels to CSS pixels, to adjust for the difference in expectations from other
nsFontMetrics clients."

Perhaps a canvas person should review this, but I think my late response has
probably given opportunity to comment.

Comment 15

[Nicholas Nethercote [inactive]]

I did some more measurements on complex PDF maps. All timings were with a stopwatch(!)

- wuppertal (from https://github.com/mozilla/pdf.js/issues/2541): 31.7s --> 15.8s

- prorail (from https://github.com/mozilla/pdf.js/issues/2813): 45.9s --> 17.9s

- mta (from bug 835380): 12.5s --> 5.8s

- paris (from https://github.com/mozilla/pdf.js/issues/2541): 5.6s --> 4.4s

There were also five others; all of them took less than 10s to begin with and may have had a small (<1s) improvement but it's hard to say given the low precision of the stopwatch.

Comment 16

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f391c76bb094

Thank you for all the hand-holding, Karl; much appreciated. The results in comment 15 are pleasing :)

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f391c76bb094

Comment 18

[Nicholas Nethercote [inactive]]

Improvement: Mozilla-Inbound-Non-PGO - CanvasMark - Ubuntu HW 12.04 - 12.5% increase
------------------------------------------------------------------------------------
    Previous: avg 6232.083 stddev 224.934 of 12 runs up to revision 1ef39d9f57c5
    New     : avg 7008.083 stddev 65.457 of 12 runs since revision f391c76bb094
    Change  : +776.000 (12.5% / z=3.450)
    Graph   : http://mzl.la/1undG53

Improvement: Mozilla-Inbound-Non-PGO - CanvasMark - Ubuntu HW 12.04 x64 - 12.8% increase
----------------------------------------------------------------------------------------
    Previous: avg 6564.333 stddev 104.169 of 12 runs up to revision 1ef39d9f57c5
    New     : avg 7403.792 stddev 73.841 of 12 runs since revision f391c76bb094
    Change  : +839.458 (12.8% / z=8.059)
    Graph   : http://mzl.la/1undC5h

\o/