Closed
Bug 1100738
Opened 10 years ago
Closed 10 years ago
Lots of memory allocations under gfxPangoFontGroup::GetBaseFont()
Categories
(Core :: Graphics: Canvas2D, defect)
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(2 files, 1 obsolete file)
47.73 KB,
text/plain
|
Details | |
3.77 KB,
patch
|
karlt
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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•10 years ago
|
||
(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
Component: Graphics: Text → Canvas: 2D
Assignee | ||
Comment 3•10 years ago
|
||
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.
Assignee | ||
Comment 4•10 years ago
|
||
Oh, I forgot to attach the new DMD output.
Assignee | ||
Comment 5•10 years ago
|
||
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.
Attachment #8543195 -
Flags: feedback?(karlt)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Comment 6•10 years ago
|
||
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.
Attachment #8543195 -
Flags: feedback?(karlt) → feedback+
Assignee | ||
Comment 7•10 years ago
|
||
> 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•10 years ago
|
||
(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.
Assignee | ||
Comment 9•10 years ago
|
||
> 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|?
Flags: needinfo?(karlt)
Comment 10•10 years ago
|
||
(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
Flags: needinfo?(karlt)
Comment 11•10 years ago
|
||
(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
Flags: needinfo?(jwatt)
Assignee | ||
Comment 12•10 years ago
|
||
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
Attachment #8546412 -
Flags: review?(karlt)
Assignee | ||
Updated•10 years ago
|
Attachment #8543195 -
Attachment is obsolete: true
Comment 13•10 years ago
|
||
(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).
Flags: needinfo?(jwatt)
Comment 14•10 years ago
|
||
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.
Attachment #8546412 -
Flags: review?(karlt) → review+
Assignee | ||
Comment 15•10 years ago
|
||
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.
Assignee | ||
Comment 16•10 years ago
|
||
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•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Assignee | ||
Comment 18•10 years ago
|
||
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/
You need to log in
before you can comment on or make changes to this bug.
Description
•