1418389 - ~5% speedometer regression around Nov 15

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Description

[Kevin Brosnan [Ex-Mozilla]]

Attachment: Perfherder spedometer graph

There is a 2 point change in the speedometer score on the 15th. Both Perfherder and arwefastyet show this regression. Need some help figuring out what regressed this. It is not a sharp line. https://treeherder.mozilla.org/perf.html#/graphs?timerange=604800&series=mozilla-inbound,1572499,1,1&series=autoland,1569047,1,1&series=mozilla-central,1571695,1,1

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Is this reproducable locally?  If so, you could run a mozregression.

Comment 2

[Andrew Overholt [:overholt]]

It doesn't look like arewefastyet reflects this dip: https://arewefastyet.com/#machine=36&view=single&suite=speedometer-misc&subtest=score

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrew Overholt [:overholt] from comment #2)
> It doesn't look like arewefastyet reflects this dip:
> https://arewefastyet.com/#machine=36&view=single&suite=speedometer-
> misc&subtest=score

You have to zoom in by selecting a range on the graph, but it does show our score noticeably worsening around the same time.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

the 2-3% regression on November 15th is from bug 1417234, :glandium- I see you wrote the patch in that bug, can you look into this regression?

Comment 5

[Mike Hommey [:glandium] OoO until Dec 30]

Attachment: Bug 1418389 - Partially revert bug 1417234.

Bug 1417234 replaced all uses of CRITICAL_SECTION with SRWLocks in the
allocator on Windows, and this seems to have incurred performance
regressions on speedometer.

OTOH, there is a real benefit from not having to manually initialize the
allocator.

So we restore the use of CRITICAL_SECTIONs for Mutexes in the allocator,
except for the initialization lock, which is remaining as a SRWLock.

Talos indicates this solves the regression in large part, but is not
definitive as whether it has the same effect as a pure backout of bug
1417234. We'll see how things go over time.

Review commit: https://reviewboard.mozilla.org/r/202280/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202280/

Comment 6

[Mike Hommey [:glandium] OoO until Dec 30]

(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #4)
> the 2-3% regression on November 15th

So is it 2-3% or 5% as the bug summary says?

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 8931193 [details]
Bug 1418389 - Partially revert bug 1417234.

https://reviewboard.mozilla.org/r/202280/#review207688

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f8b98354732
Partially revert bug 1417234. r=njn

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Partially revert bug 1417234: https://hg.mozilla.org/mozilla-central/rev/4f8b98354732

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

it is 2-3%, we see an overall 5% regression, but the issue on the 15th is 2-3% depending on the config.  Also when there is some noise, it is hard to know the exact regression- but I would safely say 2-3% it is :)

Comment 11

[(Away)]

Is the remainder of the regression clear enough to attribute to specific change(s)? If we had a 5% regression, and won back 2-3%, I don't think we should stop there.

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

there is no clear change that we can see, the one issue on Nov 15th is the single point where we can easily see a change.

Comment 13

[(Away)]

(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #12)
> there is no clear change that we can see, the one issue on Nov 15th is the
> single point where we can easily see a change.

That's unfortunate. Given how hard it is to squeeze even 1% out of Speedometer these days, it's much easier to improve our speed by un-regressing something than it is to make new gains.

If Talos can't tell us, then maybe the profiler can? Jan/Mike, do you still dig into profiles these days? It might be worth taking a fresh look to see if the past weeks have introduced any new low-hanging fruit?

Comment 14

[Jan de Mooij [:jandem]]

Bug 1392841 probably also regressed Speedometer :/

Comment 15

[Mike Conley (:mconley) (:⚙️) (PTO Dec 22 - Jan 2)]

Bug 1392841 does seem like something that would contribute to this - one of the things blocking it from landing for 57 was a Speedometer regression that, as far as I can tell, was never resolved.

jduell suggested a backout in bug 1392841. Perhaps we should back out RCWN on try and see if the numbers recover.

In the meantime, I've done retriggers on the Speedometer jobs on inbound on the push just before and just after RWCN was enabled.

There's a lot of noise in these graphs, which makes it hard to pin the issue on any one commit.

(In reply to David Major [:dmajor] from comment #13)
> If Talos can't tell us, then maybe the profiler can? Jan/Mike, do you still
> dig into profiles these days? It might be worth taking a fresh look to see
> if the past weeks have introduced any new low-hanging fruit?

I've personally not had much luck using the Profiler to debug Speedometer regressions this small - I attempted, and failed, to do so in bug 1392841.

Comment 16

[Mike Conley (:mconley) (:⚙️) (PTO Dec 22 - Jan 2)]

The retriggers suggest that RCWN (bug 1392841) did indeed introduce some amount of Speedometer regression here:

https://treeherder.mozilla.org/perf.html#/graphs?timerange=5184000&series=mozilla-central,1571695,0,1&series=autoland,1569047,0,1&series=mozilla-inbound,1572499,1,1&zoom=1510988340357.7505,1511009942229.3516,38.29972316929254,41.345132406404545

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Clearly ni on Naveed as he no longer works at Mozilla.