980639 - 9.8% CART regression on OS X found Mar 6

Status: RESOLVED WONTFIX

(Firefox :: Toolbars and Customization, defect)

Description

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

Pretty clear from the graphs and datazilla that this is from bug 909349.

The funny thing, is that for the most part - bug 909349 is a CART win. According to Datazilla[1], the average time to draw all frames during exiting went down a bit, and the other non-error measures are unchanged.

So the question is - why did the error values go up? Why, if the frames are not taking longer to paint on exiting customize mode, is it taking even longer than expected to exit the mode?

Wtf?

[1]: https://datazilla.mozilla.org/?start=1393545375&stop=1394150175&product=Firefox&repository=Fx-Team&os=mac&os_version=OS%20X%2010.6.8&test=cart&graph_search=ccbe695b5f40&project=talos

Comment 1

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

Setting P- until further analysis.

Comment 2

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

Ok, I have a theory on this one - the one difference to the customize transition that occurs with bug 909349 landed is the "open" state of the menu button is different. Before, we just swapped in a different sprite (the blue-ish menu icon).

Now we style the button differently, with box-shadows, and rounded corners and some shading. We already know that some of these things are expensive, and it may account for the regression here.

If I can reproduce locally (I'm in the midst of seeing if I can reproduce the regression on my OS X 10.6 talos clone), I'll see if not setting the open state of the button reverses the regression. If so, we've caught the culprit.

Comment 3

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

Good news - I seem to have reproduced the regression locally. \o/

Now going to test my theory with the depressed state of the menu button.

Comment 4

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

If not toggling its open state removes the regression, then I suspect we might be able to workaround the regression by bypassing the transition on the button decorations when the open state is removed.

Comment 5

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

Ok, I think we can improve on what we've got by moving the instruction where we unset the open state of the menu button to _after_ the transition ends. I think this looks more consistent too - the depressed state goes away once the panel is no longer visible.

Comment 6

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

Attachment: 980639

This doesn't remove the entirety of the regression, but it should take out the majority of it.

The rest of the regression we might have to eat unless we want to somehow make drawing the menu button cheaper during the transition (I experimented with removing the border-radius during the transition, but it didn't give us any improvement). I worry that this will result in too much movement during the beginnings and ends of the transition - thus, my suggestion that we take this win and run.

Comment 7

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

remote:   https://hg.mozilla.org/integration/fx-team/rev/ba806a1ff84e

Comment 8

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

Hm. Looking at [1], I think this backfired a little.

While this patch certainly took down the customize-exit.error measure, the customize-exit.all measure went up - meaning we traded in some lag, and we're getting a higher likelihood for choppier frames.

The similar effect can be seen on Windows and Linux.

I think I'm going to back this out and try something else.

Comment 9

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

Backed out as https://hg.mozilla.org/integration/fx-team/rev/e905debc9d6a

Comment 10

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

So, another idea is to try to bypass the transition on the menu panel button on customize-exit so that the open styling is immediately removed. I'll try that next.

Comment 11

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

So I've toyed with it a little - including trying a new CSS rule to bypass the transition on the button if we're in customize mode. No real improvement there.

So here's what we know:

1) When bug 909349, it added an "open" style to the menu button that it didn't have before (a "depressed" look). Before, it was just a shift along a sprite sheet to a blue icon.
2) Bug 909349 actually made it cheaper to paint frames while exiting customize mode - customize-exit.all went from an average of around ~20.20ms to ~15.3ms. That's a good thing!
3) The error measures for entering and exiting bumped up a little. My hypothesis is that the cost to compute the style of the button to appear open is being factored in here. That's new - we didn't have that before.
4) OS X was unique in that the menu button didn't have a depressed appearance in customize mode. Linux and Windows had this.

So what we have here is a textbook tradeoff:

We leave everything be. We eat what looks to be about a 40ms hit on customize exit (meaning a ~40ms delay before the transition will begin), but have a higher probability of smoother frames on exiting.

OR

We do something about the open state of the button - like not setting it, ever. We lose that bit of polish / consistency (where the button is "open" when the panel is shown), and we _also_ lose the win we get on customize-exit.all. But we don't have to pay the ~40ms delay at the start of exiting.

I, personally, think we should leave it be. Any thoughts?

Comment 12

[Mike de Boer [:mikedeboer]]

(In reply to Mike Conley (:mconley) from comment #11)
> I, personally, think we should leave it be. Any thoughts?

+1

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

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

Hrp - the original appears to have landed on central, but not the backout. This bug is still open.

Comment 15

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

I just spoke to gavin, and he says we should take the smoother frames for a small delay.

We're done here, folks.