973258 - 83a3ef9b2144 m-c to fx-team merge caused ~1.7% ts_paint regression

Status: RESOLVED WONTFIX

(Testing :: Talos, defect)

Description

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from bug 967766 comment #35)
> (In reply to :Gijs Kruitbosch from 967766 comment #31)
> > Open questions for this bug:
> 
> 4) Was there something that regressed shortly before the sessionstore
> changes?
> 
> > just before sessionstore: https://tbpl.mozilla.org/?tree=Try&rev=692abed8b78f
> > (http://hg.mozilla.org/integration/fx-team/rev/bf25e29dc677)
> 
> <snip>
> 
> > But before bf25e29dc677, there was also mayhem. So I've pushed some ones
> > before the baseline listed above:
> > 
> > feb 2, middle of the day, before an m-c merge:
> > https://tbpl.mozilla.org/?tree=Try&rev=87109379ade5
> >  (https://hg.mozilla.org/integration/fx-team/rev/b6cc3c35d419)
> > 
> > feb 2, very early, before another merge:
> > https://tbpl.mozilla.org/?tree=Try&rev=64a30014824c
> > (https://hg.mozilla.org/integration/fx-team/rev/463bae14bef3)
> 
> 
> Yes/maybe:
> 
> http://compare-talos.mattn.ca/
> ?oldRevs=64a30014824c&newRev=87109379ade5&server=graphs.mozilla.
> org&submit=true
> 
> This regression is bigger than the 6ms ones seen between jan 31 and feb 02,
> but not by that much, and the noise is still crazy. Anyway.
> 
> Regression range:
> 
> http://hg.mozilla.org/integration/fx-team/
> pushloghtml?fromchange=463bae14bef3&tochange=b6cc3c35d419
> 
> Considering the other csets are string/css-only changes with no realistic
> performance impact, and a metro change, I suspect the m-c to fx-team merge.
> 
> Try push for the merge: https://tbpl.mozilla.org/?tree=Try&rev=4733b02b1b97
> 
> prospective graph link:
> 
> http://compare-talos.mattn.ca/
> ?oldRevs=64a30014824c&newRev=4733b02b1b97&server=graphs.mozilla.
> org&submit=true

This shows a 1.73% regression on ts_paint.

Comment 1

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

:Gijs, is there any action we can take here?  This has now moved to Aurora?

Comment 2

[:Gijs (he/him)]

(In reply to Joel Maher (:jmaher) from comment #1)
> :Gijs, is there any action we can take here?  This has now moved to Aurora?

We could try to try-bisect the talos impacts of the merge if we think that's valuable. I don't have cycles to do that in the forthcoming weeks, however...

Comment 3

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

this might be a lost cause, but I wanted to set a tracking flag and make sure we get to a decision on this.

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

We're past the 29 release now, do you have a spare cycle to try your idea in comment 2 in the coming week or two?

Comment 5

[:Gijs (he/him)]

Sooo.... merge pushlog on m-c:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=463bae14bef3&tochange=83a3ef9b2144

I think we can safely exclude the b-i merge. So I bisected the inbound merge:

Baseline (2918a9e625b4): remote:   https://tbpl.mozilla.org/?tree=Try&rev=b065a14c7bae
With m-i merge (5f88d54c28e0): remote:   https://tbpl.mozilla.org/?tree=Try&rev=70a3f5e7bf86
With everything up to (ee15873640b6): remote:   https://tbpl.mozilla.org/?tree=Try&rev=2b597c8659ce
With everything up to (7e2d6d56c282): remote:   https://tbpl.mozilla.org/?tree=Try&rev=4943f88ea3c9
With everything up to (2b6516c8713e): remote:   https://tbpl.mozilla.org/?tree=Try&rev=700a743ac13f
With everything up to (2682af062a4b): remote:   https://tbpl.mozilla.org/?tree=Try&rev=aa0505242c17
With everything up to (b973195f0aaf): remote:   https://tbpl.mozilla.org/?tree=Try&rev=1156c355aac2

Comment 6

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #5)
> https://tbpl.mozilla.org/?tree=Try&rev=b065a14c7bae
> With m-i merge (5f88d54c28e0): remote:  
> https://tbpl.mozilla.org/?tree=Try&rev=70a3f5e7bf86
http://perf.snarkfest.net/compare-talos/index.html?oldRevs=b065a14c7bae&newRev=70a3f5e7bf86&submit=true
+1.12%

So that's what the entire push caused. Let's try to figure out in which part this was:



> With everything up to (ee15873640b6): remote:  
> https://tbpl.mozilla.org/?tree=Try&rev=2b597c8659ce
http://perf.snarkfest.net/compare-talos/index.html?oldRevs=b065a14c7bae&newRev=2b597c8659ce&submit=true
+0.44%

> With everything up to (7e2d6d56c282): remote:  
> https://tbpl.mozilla.org/?tree=Try&rev=4943f88ea3c9
http://perf.snarkfest.net/compare-talos/index.html?oldRevs=b065a14c7bae&newRev=4943f88ea3c9&submit=true
-0.2% (!)

> With everything up to (2b6516c8713e): remote:  
> https://tbpl.mozilla.org/?tree=Try&rev=700a743ac13f
http://perf.snarkfest.net/compare-talos/index.html?oldRevs=b065a14c7bae&newRev=700a743ac13f&submit=true
+0.99%

> With everything up to (2682af062a4b): remote:  
> https://tbpl.mozilla.org/?tree=Try&rev=aa0505242c17
http://perf.snarkfest.net/compare-talos/index.html?oldRevs=b065a14c7bae&newRev=aa0505242c17&submit=true
+1.15%

> With everything up to (b973195f0aaf): remote:  
> https://tbpl.mozilla.org/?tree=Try&rev=1156c355aac2
http://perf.snarkfest.net/compare-talos/index.html?oldRevs=b065a14c7bae&newRev=1156c355aac2&submit=true
+0.7%

So tentatively, that means:

http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7e2d6d56c282&tochange=2b6516c8713e

and because of backouts and because there's a GCC build arch change in there, it's really down to:

http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7e2d6d56c282&tochange=6199eb8d2f3e

I could imagine it being any of these, quite frankly. :-\

Comment 7

[:Gijs (he/him)]

Although http://hg.mozilla.org/integration/mozilla-inbound/rev/de11fdd37b20 seems most suspicious to me in terms of affecting startup, but not tpaint. Aaron, can you look into this?

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

My patches are only enabled on the Nightly channel, so if this regression rode the train then it can't be my stuff.(In reply to Joel Maher (:jmaher) from comment #1)
> :Gijs, is there any action we can take here?  This has now moved to Aurora?

Joel, am I reading this right, this regression is riding the trains?

(In reply to :Gijs Kruitbosch from comment #7)
> Although http://hg.mozilla.org/integration/mozilla-inbound/rev/de11fdd37b20
> seems most suspicious to me in terms of affecting startup, but not tpaint.
> Aaron, can you look into this?

This patch is only enabled on the nightly channel and debug builds, so if the regression is riding the train than I highly doubt that this patch is the cause.

Comment 9

[:Gijs (he/him)]

(In reply to Aaron Klotz [:aklotz] from comment #8)
> My patches are only enabled on the Nightly channel, so if this regression
> rode the train then it can't be my stuff.(In reply to Joel Maher (:jmaher)
> from comment #1)
> > :Gijs, is there any action we can take here?  This has now moved to Aurora?
> 
> Joel, am I reading this right, this regression is riding the trains?
> 
> (In reply to :Gijs Kruitbosch from comment #7)
> > Although http://hg.mozilla.org/integration/mozilla-inbound/rev/de11fdd37b20
> > seems most suspicious to me in terms of affecting startup, but not tpaint.
> > Aaron, can you look into this?
> 
> This patch is only enabled on the nightly channel and debug builds, so if
> the regression is riding the train than I highly doubt that this patch is
> the cause.

I don't think that it would be possible to isolate this particular regression from the overall bump that happens when we merge. :-(

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

I'll look further into this. Since we already have main thread I/O coverage in Talos via xperf, I might end up pushing the initialization of some of these observers to the end of the startup stage.

Comment 11

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

this is currently in the beta branch, so it is alive and well.  We have this growth in general on all our branches (>20% growth in the last year).  Despite that, this is a small regression and I would argue we need to just focus on ts, paint as a single benchmark and find a way to drop it 10% on trunk.

Comment 12

[:Gijs (he/him)]

(In reply to Aaron Klotz [:aklotz] from comment #10)
> I'll look further into this. Since we already have main thread I/O coverage
> in Talos via xperf, I might end up pushing the initialization of some of
> these observers to the end of the startup stage.

This sounds promising as a ts_paint win in general, although I suppose only for nightly. However, please be aware that I am not 100% certain, from the data in comment #6, that it's your patch. It just seems the most likely.

(In reply to Joel Maher (:jmaher) from comment #11)
> this is currently in the beta branch, so it is alive and well.  We have this
> growth in general on all our branches (>20% growth in the last year). 
> Despite that, this is a small regression and I would argue we need to just
> focus on ts, paint as a single benchmark and find a way to drop it 10% on
> trunk.

So should we wontfix this bug, file a followup on what Aaron just said, and a tracker bug for your suggestion? That might be more productive than investigating this individual 1-1.7% loss...

Comment 13

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

going to track in bug 1008163