1690986 - 8.88 - 9.56% raptor-tp6-wikipedia-firefox-cold loadtime (windows10-64-shippable) regression on push 78dab59d989b3d5c16ca163b6a6520ca694fb729 (Mon February 1 2021)

Status: RESOLVED WONTFIX

(Core :: Layout: Text and Fonts, defect)

Description

[Florin Strugariu [:Bebe]]

Perfherder has detected a raptor performance regression from push 78dab59d989b3d5c16ca163b6a6520ca694fb729. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
10%	raptor-tp6-wikipedia-firefox-cold	loadtime	windows10-64-shippable	nocondprof	2,008.21 -> 2,200.17
9%	raptor-tp6-wikipedia-firefox-cold	loadtime	windows10-64-shippable	nocondprof	1,979.50 -> 2,155.25

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests. Please follow our guide to handling regression bugs and let us know your plans within 3 business days, or the offending patch(es) will be backed out in accordance with our regression policy.

For more information on performance sheriffing please see our FAQ.

Comment 1

[Jonathan Kew [:jfkthame]]

Because bug 1687426 changed which fallback fonts we will choose in some situations -- particularly for CJK content, where fonts are large and may be slow to load on first use -- it is not surprising we could see a cold-load regression in some cases. A site like wikipedia that includes a huge number of other-language links in their own scripts is particularly likely to hit this.

(It's also possible that some cases will be improved as a result of a changed font choice, but it's really hard to predict as it would depend on what's already loaded in caches, etc.)

Given that this will not tend to be an issue except on cold load (once font data caches are warm, there should be little difference in performance depending on which fallback is chosen), and the revised behavior is more correct / gives a better user experience, I think we should just accept this regression.

Masatoshi, Makoto, WDYT?

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1687426

Comment 3

[Masatoshi Kimura [:emk]]

I agree. We will have to accept the regression in this particular case.

Comment 4

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/mozilla-central/rev/78dab59d989b#l2.34

+ gfxPlatformFontList::PrefFontList* families = nullptr;
+ if (mFirstGeneric != StyleGenericFontFamily::None) {
+ families = pfl->GetPrefFontsLangGroup(mFirstGeneric, currentLang);
+ }
+ if (!families) {
+ StyleGenericFontFamily defaultGeneric =
+ pfl->GetDefaultGeneric(currentLang);
+ families = pfl->GetPrefFontsLangGroup(defaultGeneric, currentLang);
+ }

    StyleGenericFontFamily generic = mFirstGeneric;
    if (generic == StyleGenericFontFamily::None) {
      generic = pfl->GetDefaultGeneric(currentLang);
    }
    gfxPlatformFontList::PrefFontList* families = pfl->GetPrefFontsLangGroup(mFirstGeneric, currentLang);

may improve performance a bit because the current code will build two font family lists if mFirstGeneric and defaultGeneric do not agree. (But I didn't measure.)

Comment 5

[Makoto Kato [:m_kato]]

Fetching more fonts may cause performance regression. This test (wikipedia) only uses font-family, so I make sense this alert.
So It is acceptable issue for me if this is cold start only.

Comment 6

[Jonathan Kew [:jfkthame]]

(In reply to Masatoshi Kimura [:emk] from comment #4)

https://hg.mozilla.org/mozilla-central/rev/78dab59d989b#l2.34

+ gfxPlatformFontList::PrefFontList* families = nullptr;
+ if (mFirstGeneric != StyleGenericFontFamily::None) {
+ families = pfl->GetPrefFontsLangGroup(mFirstGeneric, currentLang);
+ }
+ if (!families) {
+ StyleGenericFontFamily defaultGeneric =
+ pfl->GetDefaultGeneric(currentLang);
+ families = pfl->GetPrefFontsLangGroup(defaultGeneric, currentLang);
+ }

    StyleGenericFontFamily generic = mFirstGeneric;
    if (generic == StyleGenericFontFamily::None) {
      generic = pfl->GetDefaultGeneric(currentLang);
    }
    gfxPlatformFontList::PrefFontList* families = pfl->GetPrefFontsLangGroup(mFirstGeneric, currentLang);

may improve performance a bit because the current code will build two font family lists if mFirstGeneric and defaultGeneric do not agree. (But I didn't measure.)

I don't think this is an issue. Calling GetPrefFontsLangGroup should never return NULL, so if mFirstGeneric is not None, we'll get a family list pointer there and so we won't look at the default generic at all.

Rearranging things as you suggest would be clearer, I think, so we should probably do it. But it won't affect performance.

Comment 7

[Jonathan Kew [:jfkthame]]

Closing this as WONTFIX per comment 1, with Masatoshi and Makoto's agreement.