2030147 - Don't flush word cache based on size

Status: RESOLVED FIXED

(Core :: Graphics: Text, enhancement)

Description

[Justin Link]

For reasons that are not fully clear, Denis noted here that skipping the code path that uses the word cache for text runs that are long enough.

Denis' original test used a threshold of 1024. I have updated comparisons running right now but it seems like 768 might be a slightly better threshold.

We should also figure out why this change makes an improvement. Both paths break things down word-by-word and fundamentally break down to the same ShapeText() calls, but they get there in different ways.

Comment 1

[Justin Link]

After some investigation, it seems like the benefit of Denis' change to not send long runs of text through the word cache path comes, not because the word cache slows is directly detrimental for long runs of text, but because the word cache has a "kill-switch" of sorts when it reaches a certain size and those long runs of text fill up the word cache quickly and trigger that kill switch. That kill-switch is painful not only because the work involved in flushing the cache takes time (lots of memory walking and de-allocation) but also because the word cache is now no longer able to help.

Skipping the word cache code path for those long runs means that we get the large benefit of not flushing the cache but means that we miss the benefit of using the word cache, so we can probably do better than that.

A quick comparison where we simply remove the flush shows some pretty solid results (look at those improvements in the TipTap subtests!)

Simply not flushing the cache (as done in the above comparison) might be a viable solution (the cache shouldn't grow without bound as words still "age" and get removed based on age, and the cache as a whole is tied to a font which will go away on its own as well) but there are a few more options to consider here as well (such as by using a different cache data structure).

Comment 2

[Justin Link]

This comparison shows pretty clear, positive results on Android too, so I think it's safe to say that this is a win all-around.

Comment 3

[Justin Link]

Attachment: Bug 2030147: Don't flush the word cache just because it reaches a certain size. r=jfkthame

If it's large that means that we're using it and it's providing value.
There are other mechanisms in place to keep the word cache from growing
beyond what's needed (words "age" and can expire, and the word cache as
a whole is also tied to a font which will go away when it's no longer
needed).

Comment 4

[Matthew Gaudet (he/him) [:mgaudet]]

(there are some subtest losses that I wonder if you could recover, but this is quite cool :D )

Comment 5

[Worcester12345]

Please add or allow to add the "PERF" key word. Thanks.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/d58b512d88ee
https://hg.mozilla.org/integration/autoland/rev/d2bd964e7f87
Don't flush the word cache just because it reaches a certain size. r=jfkthame

Comment 7

[Cosmin Sabou [:CosminS]]

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

Comment 8

[Mayank Bansal]

Alert generated: https://treeherder.mozilla.org/perfherder/alerts/alerts?id=49627&hideDwnToInv=0
(legal disclaimer: yet to be validated by perf sheriffs)

Comment 9

[Bas Schouten (:bas.schouten)]

Attachment: Bug 2030147: Add a country field to the pageload domain event that is filled in for some countries on release. r=denispal

This adds country data to our pageload events with domain information. Since this information can be highly fingerprintable for situations with few users in a country/channel combination, we only collect this on release, for users in specific countries where we know we have a good amount of users. This should bring the majority of the value with no risk of accidentally collecting user data.

Comment 10

[Phabricator Automation]

Comment on attachment 9569393 [details]
Bug 2030147: Add a country field to the pageload domain event that is filled in for some countries on release. r=denispal

Revision D293953 was moved to bug 2031308. Setting attachment 9569393 [details] to obsolete.

Comment 11

[Acasandrei Beatrice (needinfo me)]

(In reply to Pulsebot from comment #6)

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/d58b512d88ee
https://hg.mozilla.org/integration/autoland/rev/d2bd964e7f87
Don't flush the word cache just because it reaches a certain size. r=jfkthame

Perfherder has detected a browsertime performance change from push d2bd964e7f87ff29bdf4188d6ec3ff20334966e0.

No action is required from the author; this comment is provided for informational purposes only.

Improvements	Test	Platform	Options	Absolute values [old vs new]	Performance Profiles
36%	netflix fcp (doc)	linux2404-64-shippable	cold fission webrender	459.61 ms -> 293.78 ms	Before/After
33%	netflix FirstVisualChange (doc)	linux2404-64-shippable	cold fission webrender	495.79 ms -> 332.35 ms	Before/After
31%	speedometer3 Editor-TipTap/Highlight/Sync (doc)	macosx1500-aarch64-shippable	fission webrender	19.60 ms -> 13.46 ms	Before/After
29%	speedometer3 Editor-TipTap/Highlight/total (doc)	macosx1500-aarch64-shippable	fission webrender	21.02 ms -> 14.83 ms	Before/After
29%	speedometer3 Editor-TipTap/Highlight/Sync (doc)	macosx1470-64-nightlyasrelease	fission webrender	69.59 ms -> 49.27 ms	Before/After
...	...	...	...	...	...
2%	speedometer3 Editor-TipTap/Highlight/total (doc)	macosx1500-aarch64-shippable	fission webrender	20.98 ms -> 20.56 ms	Before/After

Need Help or Information?

If you have any questions, please reach out to [e-mail]. Alternatively, you can find help on Slack by joining #perf-help, and on Matrix you can find help by joining #perftest.

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.