Closed Bug 1739152 Opened 7 months ago Closed 7 months ago

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

Categories

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

Firefox 96
defect

Tracking

()

RESOLVED FIXED
96 Branch
Tracking Status
firefox-esr91 --- unaffected
firefox94 --- unaffected
firefox95 --- fixed
firefox96 --- fixed

People

(Reporter: alexandrui, Assigned: jfkthame)

References

(Regression)

Details

(Keywords: perf, perf-alert, regression)

Attachments

(1 file)

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.

Flags: needinfo?(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.

Flags: needinfo?(jfkthame)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED

(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?

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

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

Pushed by jkew@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → 96 Branch

(In reply to Pulsebot from comment #7)

Pushed by jkew@mozilla.com:
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

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.

Flags: needinfo?(jfkthame)

== 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 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:
Flags: needinfo?(jfkthame)
Attachment #9249105 - Flags: approval-mozilla-beta?

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.

Attachment #9249105 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.