1756123 - 4.39 - 0.14% tart / tart (Windows) regression on Fri February 11 2022

Status: RESOLVED WONTFIX

(Testing :: Performance, defect, P3)

Description

[Alex Finder]

Perfherder has detected a talos performance regression from push b247e781ba93444c892e2e45f919600b82af8891. 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%	tart	windows10-64-shippable-qr	e10s fission stylo webrender-sw	2.36 -> 2.47
0.14%	tart	windows10-64-shippable-qr	e10s fission stylo webrender-sw	2.48 -> 2.48

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

[Alex Finder]

Hi Randell, could you please help with any updates on this regression?

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Mike, Nika - any idea how this patch would cause this? Nika indicated in the reviews that Unsound_NumQueuedMessages always returned 0, and so it's removed in the patch. Perhaps this isn't the case? (Mike, could this impact the tart test this way if NumQueuedMessages isn't returning 0?)

None of the subtests are above "low" confidence, but the final value is high confidence... also, close to half the subtests are improvements (though perhaps slightly less large than average regressions), and noise is down a lot.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

I can't imagine that this regression has anything to do with Unsound_NumQueuedMessages. There should genuinely be no way that the value returned there would be changed, and the performance of doing a couple fewer virtual calls for it should have no impact.

The only differences I'm seeing here are that we now acquire the MessageChannel's mutex when asking IsClosed() (but I wouldn't expect us to be collecting memory reports at all during this particular test, and there should be no other callers of that method), and that we no longer update the atomic output_queue_length_ member of IPC::Channel when queueing/dequeueing messages (though I would expect that to only lead to performance improvements, and extremely slight ones at that).

Interestingly, looking at the graph it appears that the regression is for a similar-ish amount to the recent improvement reported in bug 1753436, although I have no idea how they could be related. Perhaps there's some optimizer threshold which we happen to be very near right now, and is causing small swings on this particular test?

Comment 5

[Mike Conley (:mconley) (:⚙️)]

I'm no data scientist, but looking at the subtests and the noise metric, I suspect what's happening here is that we're simply reducing the noise and that's causing a "truer" sense of the score here to emerge, and that truer sense is at a slightly higher score.

The actual absolute value of the regression in milliseconds is a fraction of a millisecond. I don't think it's worth the engineering time to really dig into what happened here. We should take the win on the noise metric, and take the apparent regression, and close this as WONTFIX.