Mike Conley (:mconley) (:⚙️) Assignee
	Description • 11 years ago

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

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 1 • 11 years ago

Here is the more general compare-talos for these two changesets:

http://compare-talos.mattn.ca/?oldRevs=955e76aa16e7&newRev=0b6c881ba74c&server=graphs.mattn.ca&submit=true

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 2 • 11 years ago

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

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 3 • 11 years ago

Our try results are in. It looks like simple-close-DPI1, icon-close-DPI1 and icon-close-DPI2 took the biggest hit:

http://compare-talos.mattn.ca/?oldRevs=681a04310dd3&newRev=f3da800bed49&server=graphs.mozilla.org&submit=true

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

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 4 • 11 years ago

It looks like the big hit came from 09fd1ae57a2e:

http://compare-talos.mattn.ca/?oldRevs=681a04310dd3&newRev=924ed72235db&server=graphs.mozilla.org&submit=true

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

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 5 • 11 years ago

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.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 6 • 11 years ago

It would probably help if the patch contained something.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 7 • 11 years ago

Linux next, and then try push.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 8 • 11 years ago

I'll note that this also backs out bug 899587 and bug 895724.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 9 • 11 years ago

Ok, this does all three.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 10 • 11 years ago

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.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 11 • 11 years ago

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.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 12 • 11 years ago

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?

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 13 • 11 years ago

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.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 14 • 11 years ago

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.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 15 • 11 years ago

Pushed attachment 809486 [details] [diff] [review] to try.

We can use the same baseline as before:

Baseline: https://tbpl.mozilla.org/?tree=Try&rev=618b2baa290b
Patch: https://tbpl.mozilla.org/?tree=Try&rev=0f1b2b7edcdd

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 16 • 11 years ago

Note that this would very much explain bug 911431, as the back button would not be affected during the fade tests.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 17 • 11 years ago

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?

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 18 • 11 years ago

(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.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 19 • 11 years ago

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.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 20 • 11 years ago

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.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 21 • 11 years ago

(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.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 22 • 11 years ago

Thanks! Landed on UX as https://hg.mozilla.org/projects/ux/rev/a725e81dac6b

	Avi Halachmi (:avih)
	Comment 23 • 11 years ago

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)?

	:Gijs (he/him)
	Comment 24 • 11 years ago

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