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

NEW
Unassigned

Status

()

P3
normal
a year ago
a year ago

People

(Reporter: jmaher, Unassigned)

Tracking

(Depends on: 1 bug, {perf, regression, talos-regression})

53 Branch
perf, regression, talos-regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fix-optional)

Details

(Reporter)

Description

a year ago
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
(Reporter)

Comment 1

a year ago
this regression occurs during a large build breakage, so all the bugs related to this landed during the build breakage
Bug 1356271 only touched test code.
No longer blocks: 1356271
(Reporter)

Updated

a year ago
Blocks: 1373320
No longer blocks: 1386631
Bug 1385137 only touched test code as well.
No longer blocks: 1385137
Bug 1367551 only touched http/2 push code, which would not be exercised by tabpaint
No longer blocks: 1367551
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.
No longer blocks: 1385201
No longer blocks: 1385391
No longer blocks: 1384680
No longer blocks: 1376306
(Reporter)

Comment 7

a year ago
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
No longer blocks: 1172574, 1255227, 1383338, 1384241, 1384695, 1384792
Flags: needinfo?(bwinton)
(Reporter)

Updated

a year ago
Component: Untriaged → Tabbed Browser
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.)
Flags: needinfo?(bwinton) → needinfo?(mconley)
Mike, did you get the time to look over this?
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.
Depends on: 1355424
Flags: needinfo?(mconley)
status-firefox57: --- → fix-optional
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.