1390824 - 3.07% tabpaint (linux64) regression on push 616afd45b6e3d765bbc6451bec03d99931cb5b67 (Fri Jul 28 2017)

Status: RESOLVED WONTFIX

(Firefox :: Tabbed Browser, defect, P3)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

Talos has detected a Firefox performance regression from push:

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

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

Regressions:

  3%  tabpaint summary linux64 opt e10s     65.42 -> 67.42


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

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/Buildbot/Talos/Tests

For information on reproducing and debugging the regression, either on try or locally, see: https://wiki.mozilla.org/Buildbot/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/Buildbot/Talos/RegressionBugsHandling

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

this regression occurs during a large build breakage, so all the bugs related to this landed during the build breakage

Comment 2

[Florian Quèze [:florian]]

Bug 1356271 only touched test code.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

this is the range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=3b56ac96c918e042194781d0021f5f243babc5e7&tochange=392203e4639777b3cd3049636eca0ccfc922b1e4

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Bug 1385137 only touched test code as well.

Comment 5

[u408661]

Bug 1367551 only touched http/2 push code, which would not be exercised by tabpaint

Comment 6

[Matthew N. [:MattN]]

Bug 1379270 seems most relevant from skimming the bug summaries. 

Bug 1385201 only changed a pref's type+name which is checked once during startup so doesn't seem relevant.

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

after backing out a couple of bugs on try, it looks like bug 1379270 is the culprit.  :bwinton, I see you are the patch author here- this is sort of late- please don't drop everything for this as you landed this code 2.5 weeks ago- could you look at this and see if there is a fix related to your code in this revision:
https://hg.mozilla.org/integration/autoland/rev/2ef9bf18b361

Comment 8

[Blake Winton (:bwinton) (:☕️)]

Redirecting to mconley, cause he's a few orders of magnitude more familiar with the code and possible perf regressions than I am…  ;)

(Also, I've noticed that if we have the urlbar selected and open a new tab, the new tab's urlbar won't be selected, so if we ended up backing out that patch and going with a different, more functional fix, I wouldn't particularly mind.)

Comment 9

[Ionuț Goldan [:igoldan]]

Mike, did you get the time to look over this?

Comment 10

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

Was able to reproduce the regression locally and get profiles.

Before patch (no regression): http://bit.ly/2wCnvIl

After patch (with regression): http://bit.ly/2wCw3ir

What I'm noticing here is that we're causing a style flush due to the later focus call. This is probably because some DOM stuff changed between the last natural flush and the call to focus(). This wasn't the case before because the focus() call was being called earlier, before updateCurrentBrowser had an opportunity to dirty the DOM.

We intentionally moved the focus() call to after updateCurrentBrowser, and updateCurrentBrowser is going to update the DOM - there's no real way around that.

If we decide to do bug 1355424, what we might want to do is defer focusing the URL bar until either:

1) The DOM hasn't been dirtied and so no style flush is required (the helpers in BrowserUtils can help there)

OR

2) We process a key input

In the event of a key input, it'll be necessary to cause any lazy flushes to complete in order to send the key event to the right thing.

I don't think this should block 57 going out, but I think we should see if we can do this if / when we do bug 1355424.

Comment 11

[:Gijs (out for now; he/him)]

It's been nearly 6 years, realistically we're not going to specifically address this tabpaint regression.