Closed Bug 916946 Opened 6 years ago Closed 6 years ago

Stop animating the back-button when enabling or disabling it

Categories

(Firefox :: Tabbed Browser, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: mconley, Assigned: mconley)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P1][Australis:M9])

Attachments

(1 file, 5 obsolete files)

I noticed another blip on our backprocessed TART performance data on the UX branch - on the July 18th nightly, the simple-open, simple-close, icon-open/close and newtab-open tests were, on average, 5.22% *better* than m-c.

Then, on July 19th, that regressed to 15.77% *worse* than m-c on average.

As usual, the details paint a more interesting story:

http://compare-talos.mattn.ca/breakdown.html?oldTestIds=331&newTestIds=333&testName=tart&osName=Windows%205.1%20%28ix%29&server=graphs.mattn.ca

So something happened in these changesets that hurt TART on the UX branch for XP pretty deeply:

http://hg.mozilla.org/projects/ux/pushloghtml?fromchange=955e76aa16e7&tochange=0b6c881ba74c
Avi and I have discussed this, and our current theory is that this is our best bet for a regression that could put XP back in the black.

I've carpet-bombed Try with every changeset from 955e76aa16e7 to 0b6c881ba74c, so hopefully in the next 24 hours or so, we'll see if and where a regression happened.

955e76aa16e7: https://tbpl.mozilla.org/?tree=Try&rev=681a04310dd3
09fd1ae57a2e: https://tbpl.mozilla.org/?tree=Try&rev=924ed72235db
3a2eda2fae3f: https://tbpl.mozilla.org/?tree=Try&rev=a2b1add9af75
1227fcd5b021: https://tbpl.mozilla.org/?tree=Try&rev=24fd1a627228
9b41e04e51a8: https://tbpl.mozilla.org/?tree=Try&rev=3dc4c9062219
f9ec5709d992: https://tbpl.mozilla.org/?tree=Try&rev=6e16b578cabe
4883dc0017a1: https://tbpl.mozilla.org/?tree=Try&rev=9aa09ecd2009
0b6c881ba74c: https://tbpl.mozilla.org/?tree=Try&rev=f3da800bed49
Blocks: 755598
Summary: Find out why UX branch regressed on TART between changeset 955e76aa16e7 and 0b6c881ba74c → Fix tab-close TART regression introduced by merging URL bar items
Blocks: 911431
Attached patch Backout bug 755598 from Windows (obsolete) — Splinter Review
I'm curious to know how much bug 755598 is costing us across each platform. I've started a backout patch that I'm going to push to try. I'm also going to measure any effect this has on ts_paint and tpaint.

This patch only backs out on Windows, so I'm checkpointing here while I work on the OS X and Linux GTK backout.
Attached patch Backout bug 755598 from Windows (obsolete) — Splinter Review
It would probably help if the patch contained something.
Attachment #809400 - Attachment is obsolete: true
Linux next, and then try push.
Attachment #809402 - Attachment is obsolete: true
I'll note that this also backs out bug 899587 and bug 895724.
Attached patch Backout bug 755598 (obsolete) — Splinter Review
Ok, this does all three.
Attachment #809425 - Attachment is obsolete: true
Baseline: https://tbpl.mozilla.org/?tree=Try&rev=618b2baa290b
Backout patch: https://tbpl.mozilla.org/?tree=Try&rev=e1d7279b6cbf

I'm doing both TART and ts_paint to see what effect the merge had on ts_paint times (although I'll wager it's very little if anything). I'm also measuring perf impact on all three platforms.
Here's another interesting piece of data - I pushed a patch to try last night that reverts a few of the box-shadow rules on the back and forward buttons. Nothing major (see the diff here: https://hg.mozilla.org/try/rev/576879c7c830).

I pushed that patch, and a baseline. Here's the compare-talos:

http://compare-talos.mattn.ca/?oldRevs=666a0cc6143d&newRev=576879c7c830&server=graphs.mozilla.org&submit=true

Apparently, something about those box-shadows is costing us quite a bit on XP, especially on tab-close. Not sure why just yet.

I'll be interested in comparing this compare-talos with the one from the full backout when it's done.
So I took the build that I referred to in comment 11 (the one that reverts some box-shadow stuff), and I ran it on my local XP box with paint flashing. I then compared that with UX Nightly's paint flashing.

Two things stood out:

1) The full navigation bar repaint I reported in bug 915352 is gone. Not sure where that went.
2) The back button gets repainted a bunch when the tab is closing!

Here's a screencast of me opening some tabs with UX Nightly. Pay attention to the back button on tab close:

http://screencast.com/t/sEVSzgFRog

And here's the build from comment 11:

http://screencast.com/t/H2NkSvDu11B

No repaints in the second. So why is the first repainting so much, and can we stop it?
According to jrmuizel, the back-button repainting is really, really expensive, and we can expect a nice win if we stop doing that.

Some investigation has revealed what's happening - we're animating the box-shadow (which is the stroke of the back-button) when the back-button goes from the disabled to the enabled state. We should stop doing that.  Patch coming, and then I'll push to try to get an estimate of perf win.
I guess this little rule just needed an !important like its siblings. This seems to fix the issue locally. I'll push this to try shortly.
Note that this would very much explain bug 911431, as the back button would not be affected during the fade tests.
So can we not backout the patch from bug 755598 and just land the patch that fixes the transition on the back button box-shadow?
(In reply to Jared Wein [:jaws] from comment #17)
> So can we not backout the patch from bug 755598 and just land the patch that
> fixes the transition on the back button box-shadow?

Yep, that's my plan. I'm testing the backout patch just to see what other things that the URL bar might be doing to TART (but I really don't think it's doing anything else - I think the transition is the big win).

Once we confirm that, I'll request review, and we can land this puppy.
Here's the compare-talos with the back-button transition patch:

http://compare-talos.mattn.ca/?oldRevs=618b2baa290b&newRev=0f1b2b7edcdd&server=graphs.mozilla.org&submit=true

That's a pretty nice win.

Here's the compare-talos with UX if we whole-sale backout bug 755598:

http://compare-talos.mattn.ca/breakdown.html?oldTestIds=29824711&newTestIds=29824721&testName=tart&osName=Windows%20XP&server=graphs.mozilla.org

I don't see any win with the back-out over the transition patch, so I have to conclude that the back-button transition solely caused the regression here.
Attachment #809429 - Attachment is obsolete: true
Comment on attachment 809486 [details] [diff] [review]
Don't animate the border-color for the back-button - v1

Review of attachment 809486 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, but the transition-duration line below it is most likely ignored due to the higher precedence of the "#nav-bar .toolbarbutton-1 > .toolbarbutton-icon," rule that also sets these properties (hence the !important). Since we've lived this long with the transition-duration at 150ms, let's keep it that way and remove this unused transition-duration of 250ms.
Attachment #809486 - Flags: review?(jaws) → review+
Summary: Fix tab-close TART regression introduced by merging URL bar items → Stop animating the back-button when enabling or disabling it
(In reply to Jared Wein [:jaws] from comment #20)
> Comment on attachment 809486 [details] [diff] [review]
> Don't animate the border-color for the back-button - v1
> 
> Review of attachment 809486 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Since we've lived this long with the transition-duration at
> 150ms, let's keep it that way and remove this unused transition-duration of
> 250ms.

Sounds good.
Whiteboard: [Australis:P1][Australis:M9][fixed-in-ux]
Great investigation and results!

(In reply to Mike Conley (:mconley) from comment #11)
> ... that reverts a few of the box-shadow rules on the back and forward
> buttons.
> http://compare-talos.mattn.ca/?oldRevs=666a0cc6143d&newRev=576879c7c830&server=graphs.mozilla.org&submit=true
> 
> Apparently, something about those box-shadows is costing us quite a bit on
> XP, especially on tab-close. Not sure why just yet.

Actually, win7 gains very similar to xp from this according to this link. Win8 also has similar gains on tab close but apparently meaningfully regresses several tab-open tests.


(In reply to Mike Conley (:mconley) from comment #19)
> Here's the compare-talos with the back-button transition patch:
> 
> http://compare-talos.mattn.ca/?oldRevs=618b2baa290b&newRev=0f1b2b7edcdd&server=graphs.mozilla.org&submit=true
> 
> That's a pretty nice win.

That's an understatement - it's a great win!

Also interesting here is that xp and win7 have good wins on tab-close with mostly unaffected tab-open results (the change in .error is negligible despite its apparent big % regression), however, win8 has great wins on both tab-close _and_ tab-open.

Can we explain the win8 "abnormal" results? specifically, the fact that backing out the box-shadow rules regressed win8 tab-open (and didn't affect tab-open on win xp/7), and that the back-button transition patch improved tab-open on win8 (and didn't affect tab-open on win xp/7)?
https://hg.mozilla.org/mozilla-central/rev/a725e81dac6b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P1][Australis:M9][fixed-in-ux] → [Australis:P1][Australis:M9]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.