1915384 - 2.29% speedometer3 TodoMVC-Backbone/CompletingAllItems/Async (OSX) regression on Thu August 22 2024

Status: RESOLVED WONTFIX

(Core :: Layout: Block and Inline, defect)

Description

[Treeherder Bug Filer]

Perfherder has detected a browsertime performance regression from push 95661209edef8d7767329c1f1759d679d2692092. 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)	Performance Profiles
2%	speedometer3 TodoMVC-Backbone/CompletingAllItems/Async	macosx1400-64-shippable-qr	fission webrender	1.82 -> 1.86	Before/After

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 patch(es) may 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.

You can run these tests on try with ./mach try perf --alert 1769

For more information on performance sheriffing please see our FAQ.

Comment 1

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

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

Comment 2

[Jonathan Kew [:jfkthame]]

The regressing change (bug 727831) did make us update the line cursor more often when reflowing blocks, so it makes sense this could have caused a slight regression in some cases (though looking at the graph, it's only just noticeable compared to the overall noise of the test).

However, it also greatly improved behavior in cases that previously hit pathologically-poor performance (e.g. see bug 1405989, bug 1911451). As such, I think the tradeoff is well worth it.

One possible mitigation might be to store the line cursor directly in the frame instead of attaching it as a frame property. I tried this locally and couldn't see any clear benefit, but I'll push a comparison to tryserver and see if it moves the needle at all.

Comment 3

[Jonathan Kew [:jfkthame]]

Try runs in progress (noted here so we can easily find the comparison when it's ready):

Base revision's try run: https://treeherder.mozilla.org/jobs?repo=try&revision=d8922870805489458f9c44f8a62a8b1449336770
Local revision's try run: https://treeherder.mozilla.org/jobs?repo=try&revision=418cf839914003ac4fbde2cc70b324aa265a8f54

You'll be able to find a performance comparison here once the tests are complete (ensure you select the right framework):
https://perf.compare/compare-results?baseRev=d8922870805489458f9c44f8a62a8b1449336770&newRev=418cf839914003ac4fbde2cc70b324aa265a8f54&baseRepo=try&newRepo=try

The old comparison tool is still available at this URL:
https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=d8922870805489458f9c44f8a62a8b1449336770&newProject=try&newRevision=418cf839914003ac4fbde2cc70b324aa265a8f54

[this failed thanks to a bad rebase.... will try again]

Comment 4

[Jonathan Kew [:jfkthame]]

Fixed try run:

Base revision's try run: https://treeherder.mozilla.org/jobs?repo=try&revision=d8922870805489458f9c44f8a62a8b1449336770
Local revision's try run: https://treeherder.mozilla.org/jobs?repo=try&revision=5d9193a6bc836b7d340debb947591e5b39817211

https://perf.compare/compare-results?baseRev=d8922870805489458f9c44f8a62a8b1449336770&newRev=5d9193a6bc836b7d340debb947591e5b39817211&baseRepo=try&newRepo=try

This does show a small improvement on macOS 14, including for the subtest mentioned here (1.85ms -> 1.83ms). However, it also shows fairly similar-sized regressions on various other platforms/subtests, including several that are significant enough to be individually flagged as regressions in perf-compare: https://perf.compare/subtests-compare-results?baseRev=d8922870805489458f9c44f8a62a8b1449336770&baseRepo=try&newRev=5d9193a6bc836b7d340debb947591e5b39817211&newRepo=try&framework=13&baseParentSignature=4569403&newParentSignature=4569403

On balance, then, I don't think this really gains us anything, and we should accept the small regression here as a reasonable trade-off for the large improvement in other situations, per comment 2.

Comment 5

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

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

Comment 6

[Dianna Smith [:diannaS]]

:jfkthame is there a secondary decision maker on accepting the regression or can I close this as WONTFIX?

Comment 7

[Jonathan Kew [:jfkthame]]

Let's just close this. The regression is minor compared to the gains of bug 727831, and this doesn't look actionable at this point.