Closed Bug 1390824 Opened 6 years ago Closed 9 months ago
.07% tabpaint (linux64) regression on push 616afd45b6e3d765bbc6451bec03d99931cb5b67 (Fri Jul 28 2017)
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
this regression occurs during a large build breakage, so all the bugs related to this landed during the build breakage
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
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
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
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.