980032 - 5.67% cart regression on osx 10.6 found on fx-team from march 4th.

Status: RESOLVED WONTFIX

(Firefox :: General, defect)

Description

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

found on dev.tree-management:
https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/q23uBLDfvU0

here is a graph which shows the regression:
http://graphs.mozilla.org/graph.html#tests=[[309,64,21]]&sel=none&displayrange=7&datatype=running

here is the changeset:
http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=6622c62be1d5&tochange=aefca2214dd2

this is only seen on osx 10.6 right now.

Comment 1

[:Gijs (out for now; he/him)]

(In reply to Joel Maher (:jmaher) from comment #0)
> found on dev.tree-management:
> https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/
> q23uBLDfvU0
> 
> here is a graph which shows the regression:
> http://graphs.mozilla.org/graph.html#tests=[[309,64,
> 21]]&sel=none&displayrange=7&datatype=running
> 
> here is the changeset:
> http://hg.mozilla.org/integration/fx-team/
> pushloghtml?fromchange=6622c62be1d5&tochange=aefca2214dd2
> 
> this is only seen on osx 10.6 right now.

Mike and I just looked at this and think it's just lying. Looking at datazilla, that push is well after the change in perf.

Comment 2

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

fyi, this has make it to inbound:
https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/0S3GRDX5rQI

Comment 3

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

oh, let me look on datazilla- it is quite possible that it has picked the wrong changeset.

Comment 4

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

Looking at fx-team specifically, I see this:
https://datazilla.mozilla.org/?start=1393446824&stop=1394051624&product=Firefox&repository=Fx-Team&os=mac&os_version=OS%20X%2010.6.8&test=cart&graph_search=a781d8486ebc&tr_id=4698577&graph=1-customize-enter.all.TART&project=talos

that points at: https://hg.mozilla.org/integration/fx-team/rev/a781d8486ebc, which regresses:
* 1-customize-enter.all.TART
* 2-customize-exit.half.TART (although this is partially fixed by 092b3b1a185c)

and this revision https://hg.mozilla.org/integration/fx-team/rev/d67d16611935, regresses:
* 3-customize-enter-css.half.TART
* 3-customize-enter-css.all.TART
 
hmm, this is looking to be a dup of 980029, the same revisions are showing up in datazilla regressing the same things on osx 10.6 and osx 10.8.

oh, to get off graph server alerts.

Comment 6

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

Also appearing on 10.8:

1) https://hg.mozilla.org/integration/fx-team/rev/a781d8486ebc
* 1-customize-enter.all.TART
* 2-customize-exit.half.TART

2) https://hg.mozilla.org/integration/fx-team/rev/d67d16611935
* 3-customize-enter-css.half.TART

Comment 7

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

Those changesets are from bug 979326 and bug 979031. Both are plausible causes of this possible regression.

I'll push some patches to try to see what it shows.

Comment 8

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

Baseline: https://tbpl.mozilla.org/?tree=Try&rev=bf8919c4f791
Backout bug 979326: https://tbpl.mozilla.org/?tree=Try&rev=050b29e74923
Backout bug 979031: https://tbpl.mozilla.org/?tree=Try&rev=9873bd646acb

Compare talos baseline vs backout bug 979326: http://compare-talos.mattn.ca/?oldRevs=bf8919c4f791&newRev=050b29e74923&server=graphs.mozilla.org&submit=true

Compare talos baseline vs backout bug 979031: http://compare-talos.mattn.ca/?oldRevs=bf8919c4f791&newRev=9873bd646acb&server=graphs.mozilla.org&submit=true

Comment 9

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

P-'ing at this point until further notice.

Comment 10

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

It's not ~2%, but I am seeing somewhat of an improvement when backing out bug 979031, so let's pin it on that one for now.

Comment 11

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

Er, and by "It's not ~2%", I meant, "It's only ~2%".

Comment 12

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

thanks for looking into this!

Comment 13

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

So I took a quick look, and ran some local tests.

I seem to be able to reproduce the regression locally. Setting the visibility to hidden, or setting display: none on the customize tab does not help the numbers, so it looks like it's not a layout or painting regression on drawing more tab more of the time.

I also looked at tabbrowser.xml, and got rid of the call to _handleNewTab on a setTimeout of 0 after a non-animating tab opens. Still no dice - I really thought that'd be it.

I need to look at some more pressing bugs, but I'll come back to this.

Comment 14

[:Gijs (out for now; he/him)]

(In reply to Mike Conley (:mconley) from comment #10)
> It's not ~2%, but I am seeing somewhat of an improvement when backing out
> bug 979031, so let's pin it on that one for now.

Was that still a win on other platforms, just not OS X, or something? I'm confused why what was aiming to be an improvement is now a regression, and in that case, why we can't back it out. :-)

Comment 15

[:Gijs (out for now; he/him)]

Oh, so per comment #8 this is now about a TART regression rather than a CART regression? In which case, can you update the summary? :-)

Comment 16

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

(In reply to :Gijs Kruitbosch from comment #15)
> Oh, so per comment #8 this is now about a TART regression rather than a CART
> regression? In which case, can you update the summary? :-)

Nope, this bug summary is still very much about a CART (customization) regression. What made you think it'd be for the tabstrip?

Comment 17

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

(In reply to :Gijs Kruitbosch from comment #14)
> (In reply to Mike Conley (:mconley) from comment #10)
> > It's not ~2%, but I am seeing somewhat of an improvement when backing out
> > bug 979031, so let's pin it on that one for now.
> 
> Was that still a win on other platforms, just not OS X, or something? I'm
> confused why what was aiming to be an improvement is now a regression, and
> in that case, why we can't back it out. :-)

This was a subjective improvement. UX wanted less movement in the tabstrip during the customize mode transition. Forcing the customization mode tab to not animate achieved the effect they wanted, and I agreed that it looked smoother / less noisy.

So it was a subjective improvement that UX approved of. Whether or not it was a win on other platforms, I don't know just yet, but I'll tell you soon. I'm spending today picking through talos regressions, I guess.

Comment 18

[:Gijs (out for now; he/him)]

(In reply to Mike Conley (:mconley) from comment #16)
> (In reply to :Gijs Kruitbosch from comment #15)
> > Oh, so per comment #8 this is now about a TART regression rather than a CART
> > regression? In which case, can you update the summary? :-)
> 
> Nope, this bug summary is still very much about a CART (customization)
> regression. What made you think it'd be for the tabstrip?

Comment 4 and comment 6 all say "TART". It seems they also say "customize-..." so it's just confusing. :-|

Comment 19

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

(In reply to :Gijs Kruitbosch from comment #18)
> (In reply to Mike Conley (:mconley) from comment #16)
> > (In reply to :Gijs Kruitbosch from comment #15)
> > > Oh, so per comment #8 this is now about a TART regression rather than a CART
> > > regression? In which case, can you update the summary? :-)
> > 
> > Nope, this bug summary is still very much about a CART (customization)
> > regression. What made you think it'd be for the tabstrip?
> 
> Comment 4 and comment 6 all say "TART". It seems they also say
> "customize-..." so it's just confusing. :-|

Ah, right.

So, it _is_ confusing. We've shoe-horned CART into TART's framework, which is why it has the .TART suffix - however, let it be known that this bug is indeed about customization transition performance.

Comment 20

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

I _think_ I'm starting to narrow down on this one. It looks like when we animate the tab strip, we cause another frame to be painted somehow, and it's a cheaper frame than all of the other ones, which brings down the average for the entering and exiting transition measurements.

Here are some frame measurements I just got from my talos clone:


Baseline (not animating about:customizing)

3-customize-enter-css.raw.TART: 7.5 72.9 29.8 45.7 31.6 30.6 29.9
3-customize-enter-css.half.TART: 34.5
3-customize-enter-css.all.TART: 35.4
3-customize-enter-css.error.TART: 52.2

3-customize-enter-css.raw.TART: 7.8 72.7 30.5 45.7 32.1 30.6 29.8
3-customize-enter-css.half.TART: 34.5
3-customize-enter-css.all.TART: 35.6
3-customize-enter-css.error.TART: 52.9


Animating about:customizing (what we were doing before bug 979031 landed)

3-customize-enter-css.raw.TART: 10.0 72.4 30.8 44.4 32.7 30.1 30.4 6.3
3-customize-enter-css.half.TART: 28.8
3-customize-enter-css.all.TART: 32.1
3-customize-enter-css.error.TART: 57.8


3-customize-enter-css.raw.TART: 9.9 72.3 28.1 48.1 31.4 29.5 29.4 4.3
3-customize-enter-css.half.TART: 28.5
3-customize-enter-css.all.TART: 31.6
3-customize-enter-css.error.TART: 62.6



Check out that cheap frame at the end of the animating ones (the second set). 6.3 and 4.3 ms to draw a single frame each, both of which are crazy outliers to the rest of the measurements.

I don't know where that cheap frame is coming from, but because it's so cheap, I have to conclude it's not coming from the customization mode transition.

So, conclusion:

This is not a regression in the normal sense. We haven't made anything worse - in fact, we've probably made things better because we don't have to animate the new tab. The only reason this is showing up as bad on CART is because what we had before was accidentally measuring a thing it wasn't supposed to. That thing brought down the average of some measurements.

When that thing went away, the averages went up, and it looked like a regression.

Comment 21

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

On the argument above, I'm going to WONTFIX this one unless there are any serious arguments against.