Closed Bug 1771791 Opened 2 years ago Closed 2 years ago

22.66 - 15.79% a11yr dhtml.html / a11yr dhtml.html + 3 more (Linux, Windows) regression on Thu May 26 2022

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
103 Branch
Tracking Flags:
Tracking Status
firefox-esr91 --- unaffected
firefox101 --- unaffected
firefox102 --- fixed
firefox103 --- fixed

People

(Reporter: aglavic, Assigned: emilio)

References

(Regression)

Details

(4 keywords)

Attachments

(1 file)

Bug 1771791 - Use regular ComputeIndexOf in ComputeFlatTreeIndexOf if there's no Shadow DOM involved. r=mrobinson
48 bytes, text/x-phabricator-request
pascalc
: approval-mozilla-beta+
Details | Review

Perfherder has detected a talos performance regression from push e0e15fc6dbf02a00e0c65316b4487782d9b3bc89. 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)
23% a11yr dhtml.html windows10-64-shippable-qr e10s fission stylo webrender 624.87 -> 766.46
22% a11yr dhtml.html windows10-64-shippable-qr e10s fission stylo webrender-sw 628.47 -> 765.97
18% a11yr dhtml.html windows10-64-shippable-qr e10s fission stylo webrender 657.70 -> 773.27
16% a11yr dhtml.html linux1804-64-shippable-qr e10s fission stylo webrender 790.07 -> 917.30
16% a11yr dhtml.html linux1804-64-shippable-qr e10s fission stylo webrender-sw 781.05 -> 904.35

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.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

For more information on performance sheriffing please see our FAQ.

Flags: needinfo?(mrobinson)

I've investigated this performance regression. The issue seems to stem from the switch from ComputeIndexOf to ComputeFlatTreeIndexOf. ComputeIndexOf is highly optimized. Not only does it cache results, it can use the cache to find subsequent indices faster. To fix this issue, I think we'd need to try:

  1. Adding a cache for ComputeFlatTreeIndexOf
  2. Implement a version of GetNextSibling and GetPreviousSibling for the flat tree that are fast enough to improve performance.

I've confirmed that the second part is key, but I'm not sure if it's possible.

Flags: needinfo?(mrobinson)

(In reply to Martin Robinson (email:mrobinson@igalia.com) from comment #1)

I've investigated this performance regression. The issue seems to stem from the switch from ComputeIndexOf to ComputeFlatTreeIndexOf. ComputeIndexOf is highly optimized. Not only does it cache results, it can use the cache to find subsequent indices faster. To fix this issue, I think we'd need to try:

  1. Adding a cache for ComputeFlatTreeIndexOf
  2. Implement a version of GetNextSibling and GetPreviousSibling for the flat tree that are fast enough to improve performance.

We should be able at the very least be able to optimize the cases where shadow DOM isn't involved, right?

Assignee: nobody → emilio
Status: NEW → ASSIGNED

Martin, any chance you could check if comment 3 (or a similar version of it) addresses the issue?

Flags: needinfo?(mrobinson)

Sure thing, I'll test this now.

Flags: needinfo?(mrobinson)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/292465d818d0
Use regular ComputeIndexOf in ComputeFlatTreeIndexOf if there's no Shadow DOM involved. r=mrobinson

https://hg.mozilla.org/mozilla-central/rev/292465d818d0

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox103: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 103 Branch

== Change summary for alert #34277 (as of Thu, 02 Jun 2022 13:49:33 GMT) ==

Regressions:

Ratio Test Platform Options Absolute values (old vs new)
9% wikipedia FirstVisualChange macosx1015-64-shippable-qr fission warm webrender 320.00 -> 350.00
9% wikipedia PerceptualSpeedIndex macosx1015-64-shippable-qr fission warm webrender 322.04 -> 351.80
9% wikipedia ContentfulSpeedIndex macosx1015-64-shippable-qr fission warm webrender 321.71 -> 351.15
9% wikipedia SpeedIndex macosx1015-64-shippable-qr fission warm webrender 321.78 -> 350.68
6% amazon ContentfulSpeedIndex macosx1015-64-shippable-qr cold fission webrender 431.58 -> 457.08
5% wikipedia loadtime macosx1015-64-shippable-qr fission warm webrender 378.40 -> 398.08
5% wikipedia loadtime macosx1015-64-shippable-qr fission warm webrender 379.08 -> 397.83
3% wikipedia fcp macosx1015-64-shippable-qr cold fission webrender 487.50 -> 503.42
3% wikipedia fcp macosx1015-64-shippable-qr cold fission webrender 489.83 -> 502.21
2% wikipedia loadtime macosx1015-64-shippable-qr cold fission webrender 655.50 -> 668.76

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

To be clear these are regressions from the original patch, not from comment 9, right? The fix here should address those as well.

Flags: needinfo?(aionescu)

Yes, but I can't reassign browsertime alert to talos.

Flags: needinfo?(aionescu)

The patch landed in nightly and beta is affected.
:emilio, 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?(emilio)

Comment on attachment 9279096 [details]
Bug 1771791 - Use regular ComputeIndexOf in ComputeFlatTreeIndexOf if there's no Shadow DOM involved. r=mrobinson

Beta/Release Uplift Approval Request

  • User impact if declined: Simple performance fix for large-ish perf regressions, see comment 0 and comment 10
  • 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): relatively straight-forward fix.
  • String changes made/needed: none
  • Is Android affected?: Yes
Flags: needinfo?(emilio)
Attachment #9279096 - Flags: approval-mozilla-beta?

== Change summary for alert #34322 (as of Tue, 07 Jun 2022 00:45:03 GMT) ==

Improvements:

Ratio Test Platform Options Absolute values (old vs new)
20% a11yr dhtml.html macosx1015-64-shippable-qr e10s fission stylo webrender-sw 875.56 -> 699.28
18% a11yr dhtml.html windows10-64-shippable-qr e10s fission stylo webrender-sw 764.62 -> 626.35
18% a11yr dhtml.html windows10-64-shippable-qr e10s fission stylo webrender 767.93 -> 630.97
17% a11yr dhtml.html macosx1015-64-shippable-qr e10s fission stylo webrender 880.16 -> 729.50
17% a11yr dhtml.html macosx1015-64-shippable-qr e10s fission stylo webrender 876.60 -> 727.05
... ... ... ... ...
6% a11yr dhtml.html macosx1015-64-shippable-qr e10s fission stylo webrender 735.04 -> 690.77

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

Comment on attachment 9279096 [details]
Bug 1771791 - Use regular ComputeIndexOf in ComputeFlatTreeIndexOf if there's no Shadow DOM involved. r=mrobinson

Approved for 102 beta 5, thanks.

Attachment #9279096 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

== Change summary for alert #34398 (as of Fri, 10 Jun 2022 19:39:41 GMT) ==

Improvements:

Ratio Test Platform Options Absolute values (old vs new)
19% a11yr dhtml.html macosx1015-64-shippable-qr e10s fission stylo webrender-sw 855.68 -> 691.07
19% a11yr dhtml.html windows10-64-shippable-qr e10s fission stylo webrender-sw 764.51 -> 617.68
19% a11yr dhtml.html windows10-64-shippable-qr e10s fission stylo webrender 761.95 -> 619.29
16% a11yr dhtml.html macosx1015-64-shippable-qr e10s fission stylo webrender 832.81 -> 701.50
14% a11yr dhtml.html linux1804-64-shippable-qr e10s fission stylo webrender 908.34 -> 781.79
13% a11yr dhtml.html linux1804-64-shippable-qr e10s fission stylo webrender-sw 890.81 -> 774.33

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

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: