1979049 - Testcase generating (and loading?) N dummy WOFF2 font is 50% slower to generate, and takes infinite time in cleanup.

Status: RESOLVED FIXED

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

Description

[Mayank Bansal]

Attachment: Dummy WOFF2 Fonts.HTML

Open testcase
Click on start
Let the page generate and load the dummy fonts
Reload the page to do cleanup.

Firefox: https://share.firefox.dev/40ymNIm (12s to generate, infinite time in cleanup)
Chrome: https://share.firefox.dev/4omjEpx (8s to generate, does not do cleanup)

Very artificial. But maybe something easy to improve?

Comment 1

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

The severity field is not set for this bug.
:tlouw, could you have a look please?

For more information, please visit BugBot documentation.

Comment 2

[Mayank Bansal]

Testcase triggers long style/flushing thingy on theme cange: https://share.firefox.dev/3LYUkqD (90s+ in mozilla::dom::FontFaceSetDocumentImpl::InsertRuleFontFace)

Is that expected that theme change (which incluides change to the border color etc.) trigger such long restyle, when there is no element and the only thing added to the page is fonts?

This is the same scenario but with N=50000 : https://share.firefox.dev/4oceElI

Comment 3

[Jonathan Kew [:jfkthame]]

When we refresh the font set, FontFaceSetDocumentImpl::UpdateRules moves the existing mRuleFaces to a temporary oldRules array, and then builds the new mRuleFaces one by one, moving individual records across from oldRules as they're found. The problem arises if the array is large because we will generally be handling the rules in the same order as previously, which means we'll be pulling entries out of oldRules from the beginning of the array. That's expensive with a large array because all the following entries get shifted down.

A simple improvement, therefore, is to reverse the oldRules array, so that we'll be pulling entries from the end instead of the beginning. This avoids lots of array-element shifting as we're rebuilding the set.

(In theory, I guess an author could still hit the quadratically-bad case, but they'd have to not only have a very large number of rules, but then explicitly reverse their order, which seems a pretty unlikely scenario.)

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1979049 - Reverse the array of FontFaceRecords in oldRecords, so that we will most commonly remove them from the end rather than the beginning of the array.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/30a753a79987
https://hg.mozilla.org/integration/autoland/rev/060a0a7be5c9
Reverse the array of FontFaceRecords in oldRecords, so that we will most commonly remove them from the end rather than the beginning of the array. r=layout-reviewers,firefox-style-system-reviewers,emilio

Comment 6

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/060a0a7be5c9

Comment 7

[Mayank Bansal]

profile for generation: https://share.firefox.dev/4igxZ3Y (12s)
profile for theme change triggered restlye: https://share.firefox.dev/3KnNniq (2s) --> So this has definitely improved atleast 45x.
profile of cleanup: https://share.firefox.dev/3MlypKl (lots of time)