1843274 - 10.36 - 4.73% twitter fcp / twitter LastVisualChange + 2 more (Linux) regression on Wed June 28 2023

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect, P2)

Description

[Alex Finder|PTO, back on November 6th]

Perfherder has detected a browsertime performance regression from push 93a3284935bda1cf45173e0c3feaf663ad7da2f3. 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
10%	twitter fcp	linux1804-64-shippable-qr	bytecode-cached cold fission webrender	214.18 -> 236.38	Before/After
5%	twitter ContentfulSpeedIndex	linux1804-64-shippable-qr	bytecode-cached cold fission webrender	1,469.43 -> 1,546.22	Before/After
5%	twitter SpeedIndex	linux1804-64-shippable-qr	bytecode-cached cold fission webrender	1,392.17 -> 1,461.35	Before/After
5%	twitter LastVisualChange	linux1804-64-shippable-qr	bytecode-cached cold fission webrender	1,872.03 -> 1,960.59	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 offending 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.

For more information on performance sheriffing please see our FAQ.

Comment 1

[BugBot [:suhaib / :marco]]

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

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Bug 1843274 - Remove MOZ_UNLIKELY annotation that may not hold r?sfink

The only annotation I can see in changeset
93a3284935bda1cf45173e0c3feaf663ad7da2f3 that is possibly responsible for this
change is this one which concerns whether it's more common to allocate at a new
alloc site or at a previously used site. I assumed it would be the latter but
it's possible that that doesn't hold for a large application starting up.

I tried to test this with perfherder, with mixed results. This patch does
improve results for the tests mentioned in the bug - but only on macOS, not
Linux.

Comment 3

[Jon Coppeard (:jonco)]

We'll see what effect this has.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a3382c455833
Remove MOZ_UNLIKELY annotation that may not hold r=sfink

Comment 5

[Natalia Csoregi [:nataliaCs]]

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

Comment 6

[Dianna Smith [:diannaS]]

:jonco not sure how to confirm the effect.. can you take a look? if its better we might want to uplift this to beta as there are only two betas left this cycle

Comment 7

[Jon Coppeard (:jonco)]

I was waiting to see if perfherder reported any changes.

This doesn't appear to have made any difference from looking at the graphs, but I couldn't see any long term change in them to start with. I'm starting to think this isn't an actual regression at all.

Alex, how confident are you that this is a real change? The graphs are noisy but I can't really see any change on the 28th.

Comment 8

[BugBot [:suhaib / :marco]]

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

Comment 9

[Alex Finder|PTO, back on November 6th]

(In reply to Jon Coppeard (:jonco) from comment #7)

I was waiting to see if perfherder reported any changes.

This doesn't appear to have made any difference from looking at the graphs, but I couldn't see any long term change in them to start with. I'm starting to think this isn't an actual regression at all.

Alex, how confident are you that this is a real change? The graphs are noisy but I can't really see any change on the 28th.

Zooming in on the graph on this area shows the change a bit better. There is also the following side-by-side job that shows a video comparison between the two revisions, for more reference. Please need info me if you have additional questions or if you think the regression is invalid.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Looking at the various Twitter graphs, it appears they're all running at lower values now than they were when this first regressed. Should we go ahead and close this out?

Comment 11

[Jon Coppeard (:jonco)]

Yes, let's close this.