Talos has detected a Firefox performance regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=f6a7e3d2a75386db5685db7779abe90dd932905e

As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

19% tart linux64-shippable opt e10s stylo 1.82 -> 2.17
13% tart linux64-shippable opt e10s stylo 1.91 -> 2.16
10% tart windows10-64-shippable-qr opt e10s stylo 2.50 -> 2.73
6% tart windows7-32-shippable opt e10s stylo 2.15 -> 2.29
6% tart windows10-64-shippable opt e10s stylo 2.16 -> 2.29

You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=24597

On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the Talos jobs in a pushlog format.

To learn more about the regressing test(s), please see: https://wiki.mozilla.org/TestEngineering/Performance/Talos

For information on reproducing and debugging the regression, either on try or locally, see: https://wiki.mozilla.org/TestEngineering/Performance/Talos/Running

*** Please let us know your plans within 3 business days, or the offending patch(es) will be backed out! ***

Our wiki page outlines the common responses and expectations: https://wiki.mozilla.org/TestEngineering/Performance/Talos/RegressionBugsHandling

Looks like Bug 1603778 is the culprit here. A try run that disables the Top Sites behaviour shows mixed stylo results, whereas a try run that disables openViewOnFocus shows clear improvement.

Dao, do you have any hunches as to how openViewOnFocus might be causing these performance regressions? I'm not very familiar with what exactly is being tested here.

I assume somewhere as part of tart the address bar is focused (in a tab that has something loaded other than about:home or about:newtab) and we open the results panel due to our new behavior. florian*, does that sound right? If so we might just accept that this is now part of the test and close this, or modify the test to not focus the address bar. The former seems preferable to me.

*: not sure who wrote this test, mconley might be better positioned to answer this.

(In reply to Dão Gottwald [::dao] from comment #3)

*: not sure who wrote this test, mconley might be better positioned to answer this.

I think Mike wrote it.

We're not shipping this by the looks of it, so let's track this properly first.

Dão, can you outline under what circumstances the dropdown is and is not supposed to open when tabs are opened and when tabs are switched, and how the new behaviour differs from the current behaviour? It's not clear to me from the comments here or in the other bug.

(In reply to :Gijs (he/him) from comment #5)

Dão, can you outline under what circumstances the dropdown is and is not supposed to open when tabs are opened and when tabs are switched, and how the new behaviour differs from the current behaviour? It's not clear to me from the comments here or in the other bug.

The dropdown should open when clicking in the address bar or hitting Accel+L, except when about:home or about:newtab is loaded. I wouldn't actually expect tart to trigger it, so this is irritating.

The other effect of browser.urlbar.openViewOnFocus = true is to hide the dropmarker, but it's equally puzzling to me how that would regress tart.

(In reply to Dão Gottwald [::dao] from comment #6)

(In reply to :Gijs (he/him) from comment #5)

Dão, can you outline under what circumstances the dropdown is and is not supposed to open when tabs are opened and when tabs are switched, and how the new behaviour differs from the current behaviour? It's not clear to me from the comments here or in the other bug.

The dropdown should open when clicking in the address bar or hitting Accel+L, except when about:home or about:newtab is loaded. I wouldn't actually expect tart to trigger it, so this is irritating.

Looking through testing/talos/talos/tests/tart/addon/content/tart.js, I see that we change the newtab url in a few places, so that would sidestep the about:newtab restriction. However I still don't see where tart would click in the address bar or hit Accel+L / call openLocation.

Running tart locally I don't see it open the results popup. The only visible difference appears to be the dropmarker being hidden. I'll have to disable the dropmarker hiding to see if that affects the numbers.

(In reply to Dão Gottwald [::dao] from comment #9)

Running tart locally I don't see it open the results popup. The only visible difference appears to be the dropmarker being hidden. I'll have to disable the dropmarker hiding to see if that affects the numbers.

Yeah, force-showing the dropmarker with browser.urlbar.openViewOnFocus = true fixes the regression.

Comment on attachment 9119698 [details]
browser.urlbar.openViewOnFocus_true__dropmarker_shown.json

Mike, does anything jump out at you here? I see that subtests 3, 4, 9, 10, and 15 get 0 as their value which probably shouldn't happen?

Sorry for the delay - looking now.

(In reply to Florian Quèze [:florian] from comment #4)

I think Mike wrote it.

Going to declare innocence here and stress that I did not write the TART test. Ownership has fallen onto me, however, since it seems I'm the one who's kinda become the expert on what it does, and on grappling with it.

To be clear, what TART measures is the time to paint each frame of the tab animation when running in ASAP mode (not waiting for the 16ms refresh tick, but painting as soon as an invalidation occurs). This makes it pretty sensitive to things like layerization changes, or changes in the complexity of what is invalidated.

So after poking at this a while, I have a hypothesis.

The TART test works by kicking off some JS to do an animation (opening and closing tabs, typically), and then uses a privileged API to get information about how long it took to paint each frame from the start of that animation to the transitionend firing on the relevant DOM node.

My current hypothesis is that the 0.15s opacity transition on the dropmarker has been somehow contaminating these numbers, adding additional frames to the recording that brings down the average score. When setting the browser.urlbar.openViewOnFocus pref to true, the dropmarker is always hidden, so we don't get to transition its opacity, so we don't get those added frames, and so the score bounces back to the more realistic number that doesn't include the dropmarker.

I'm going to test this hypothesis by pushing to try with the dropmarker opacity transition disabled with and without the pref. If my theory holds, then I think this is a WONTFIX, and maybe we should file a new bug to make TART less sensitive to these sorts of "external" animations.

Here's the comparison:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=0264b3820c44192cf7167de2e4929e487848b457&newProject=try&newRevision=2be5825f1adc06e30eded30bb4496d086fe86390&framework=1

When I disable the dropmarker animation, setting browser.urlbar.openViewOnFocus to true actually seems to improve the score, but only a tiny bit (likely because we're not having to spend time drawing the dropmarker SVG).

So I'm reasonably confident now that my hypothesis in comment 15 was correct.

I think there was no real regression here. The TART test's original scores before the regressing patch landed were incorrect because of the extra frames recorded due to the dropmarker animation, and now they're "more correct".

I've filed bug 1610379 to try to head this sort of thing off in the future.

Closing this as WONTFIX.

Awesome, thanks for this analysis Mike!