1355595 - Simplify the default font size calculation for the spellchecker underline

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P1)

Description

[Evelyn Hung]

+++ This bug was initially created as a clone of Bug #1354641 +++

From profile [1] and code [2] it shows we spend some time to calculate the thickness of the spellchecker underline when every time selectedframes is called. We might be able to calculate this value directly from PresContext instead of creating a temporary nsStyleFont object to do a bunch of expensive work.

[1] https://perfht.ml/2p4IE6S
[2] http://searchfox.org/mozilla-central/source/layout/generic/nsTextFrame.cpp#5798-5805

Comment 1

[Evelyn Hung]

Attachment: Bug 1355595 - Simplify the default font size calculation;

When calculating the thickness of the spellchecker underline, we can
directly use aPresContext to get default font size, instead of creating
a temporary nsStyleFont object. nsStyleFont does more work than default
font size calculation, and destructing its member mFont object is
expensive (see bug 1355600), so it's better to avoid it on this hot path.

Review commit: https://reviewboard.mozilla.org/r/129160/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129160/

Comment 2

[Evelyn Hung]

David, I saw you reviewed bug 486735 so set review request to you. Feel free to point to someone else if you are busy. Thanks!

Comment 3

[Evelyn Hung]

profile after applying this patch: https://perfht.ml/2opWylH.

Comment 4

[Evelyn Hung]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=20f035e15d31bf2773f331c1cb6bfe48d3138fce

Comment 5

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

Comment on attachment 8857223 [details]
Bug 1355595 - Simplify the default font size calculation;

https://reviewboard.mozilla.org/r/129160/#review131682

r=dbaron with these three comments:

::: layout/generic/nsTextFrame.cpp:5805
(Diff revision 1)
> -      gfxFloat fontSize = std::min(gfxFloat(defaultFontSize),
> +      int32_t scaledFontSize = aPresContext->AppUnitsToDevPixels(
> +          defaultFontSize * aPresContext->EffectiveTextZoom());

I think it's clearer to continue using the nsStyleFont::ZoomText function here, i.e., by replacing the manual multiplication of "defaultFontSize * aPresContext->EffectiveTextZoom()" with nsStyleFont::ZoomText(aPresContext, defaultFontSize)".  (I think the rounding is different, maybe, depending on what NSToCoordTruncClamped does.  It's also clearer.)

I'd also call the resulting variable zoomedFontSize rather than scaledFontSize, since scaled could mean so many other things.

::: layout/generic/nsTextFrame.cpp:5807
(Diff revision 1)
> +      gfxFloat fontSize = std::min(gfxFloat(scaledFontSize),
>                                   aFontMetrics.emHeight);

While you're here, please line up the indentation of the two arguments to std::min(), or just fit them one one line if they fit within 80 characters.  They were probably misaligned after some previous mass-edit.

Comment 6

[Evelyn Hung]

Comment on attachment 8857223 [details]
Bug 1355595 - Simplify the default font size calculation;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129160/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/abfece312d74
Simplify the default font size calculation; r=dbaron

Comment 8

[Evelyn Hung]

Thanks for the review! David.
commend addressed and trigger auto-land on mozreview.

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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