1739152 - 4.05% google-docs LastVisualChange (OSX) regression on Tue October 26 2021

Status: RESOLVED FIXED

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

Description

[Alexandru Ionescu (needinfo me) [:alexandrui]]

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

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)
4%	google-docs LastVisualChange	macosx1014-64-shippable-qr	warm webrender	3,790.00 -> 3,943.33

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]]

There could be a couple of reasons for this, I think: maybe it's the overhead of checking whether we need to start the expiration timer, each time we cache a word; or maybe it's because we shortened the timeouts so that cache entries expire more quickly.

I suggest we try reverting the timeout values to what they were before bug 1736868, so that the only change is the check for empty caches (and starting/stopping the expiration timer accordingly), and see what effect that has.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1739152 - Revert changes to font cache timeouts from bug 1736868, to see effect on perf tests. r?lsalzman

Comment 3

[Florian Quèze [:florian]]

(In reply to Jonathan Kew (:jfkthame) from comment #1)

I suggest we try reverting the timeout values to what they were before bug 1736868, so that the only change is the check for empty caches (and starting/stopping the expiration timer accordingly), and see what effect that has.

Is this something that can be easily verified on try without landing changes?

Comment 4

[Jonathan Kew [:jfkthame]]

We can give it a shot, I guess, if the results are consistent enough to show through the noise. I'll try pushing some jobs...

Comment 5

[Jonathan Kew [:jfkthame]]

Try seems to think this does help:
https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=0b9388e620776f70429ca39858b2f8ffa5c6f852&newProject=try&newRevision=f1c6ecbf868f7f8c413c4840a0cce7604cde181c&framework=13&page=1&showOnlyImportant=1

Comment 6

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

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/13120fb0093f
Revert changes to font cache timeouts from bug 1736868, to see effect on perf tests. r=lsalzman

Comment 8

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/13120fb0093f

Comment 9

[Acasandrei Beatrice (needinfo me)]

(In reply to Pulsebot from comment #7)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/13120fb0093f
Revert changes to font cache timeouts from bug 1736868, to see effect on
perf tests. r=lsalzman

== Change summary for alert #32318 (as of Tue, 09 Nov 2021 06:17:27 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
5%	google-docs LastVisualChange	macosx1014-64-shippable-qr	warm webrender	3,963.33 -> 3,770.00
3%	google-docs LastVisualChange	macosx1014-64-shippable-qr	warm webrender	3,931.67 -> 3,796.67
3%	wikipedia loadtime	linux1804-64-shippable-qr	warm webrender	418.50 -> 406.75

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=32318

Comment 10

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

The patch landed in nightly and beta is affected.
:jfkthame, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 11

[Alexandru Ionescu (needinfo me) [:alexandrui]]

== Change summary for alert #32277 (as of Sat, 06 Nov 2021 05:09:13 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
6%	imdb dcf	windows10-64-shippable-qr	warm webrender	411.83 -> 386.04

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=32277

Comment 12

[Jonathan Kew [:jfkthame]]

Comment on attachment 9249105 [details]
Bug 1739152 - Revert changes to font cache timeouts from bug 1736868, to see effect on perf tests. r?lsalzman

Beta/Release Uplift Approval Request

• User impact if declined: Minor perf regression on a few individual browsertime tests. (I suspect the overall impact is pretty negligible for real-world users.)
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Just restores previous (longer) cache-expiration timeouts. One possible risk is that this means we're a bit slower to release some memory, but this is just reverting to earlier behavior (and was done on central a couple weeks ago) so unlikely to be a serious concern.
• String changes made/needed:

Comment 13

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9249105 [details]
Bug 1739152 - Revert changes to font cache timeouts from bug 1736868, to see effect on perf tests. r?lsalzman

Has tests, was verified on nightly and baked on nightly for 3 weeks, looks safe enough for our last beta, approved for 95 beta 12, thanks.

Comment 14

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-beta/rev/67b59c815e82