Closed Bug 980639 Opened 6 years ago Closed 6 years ago

9.8% CART regression on OS X found Mar 6

Categories

(Firefox :: Toolbars and Customization, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED WONTFIX
Firefox 30
Tracking Status
firefox29 --- wontfix

People

(Reporter: mconley, Assigned: mconley)

References

(Blocks 2 open bugs)

Details

(Keywords: perf, regression, Whiteboard: [talos_regression][Australis:P2])

Attachments

(1 obsolete file)

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
Setting P- until further analysis.
Whiteboard: [talos_regression] → [talos_regression][Australis:P-]
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.
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.
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.
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.
Attached patch 980639 (obsolete) — Splinter Review
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.
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Attachment #8388771 - Flags: review?(gijskruitbosch+bugs)
Attachment #8388771 - Flags: review?(gijskruitbosch+bugs) → review+
remote:   https://hg.mozilla.org/integration/fx-team/rev/ba806a1ff84e
Whiteboard: [talos_regression][Australis:P-] → [talos_regression][Australis:P2][fixed-in-fx-team]
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.
Backed out as https://hg.mozilla.org/integration/fx-team/rev/e905debc9d6a
Whiteboard: [talos_regression][Australis:P2][fixed-in-fx-team] → [talos_regression][Australis:P2]
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.
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?
(In reply to Mike Conley (:mconley) from comment #11)
> I, personally, think we should leave it be. Any thoughts?

+1
https://hg.mozilla.org/mozilla-central/rev/ba806a1ff84e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Hrp - the original appears to have landed on central, but not the backout. This bug is still open.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [talos_regression][Australis:P2] → [talos_regression][Australis:P2][leave open]
I just spoke to gavin, and he says we should take the smoother frames for a small delay.

We're done here, folks.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → WONTFIX
Whiteboard: [talos_regression][Australis:P2][leave open] → [talos_regression][Australis:P2]
You need to log in before you can comment on or make changes to this bug.