There were several commits flagged in the push, but it looks like jmaher did some retriggering (e.g. https://treeherder.mozilla.org/perf.html#/graphs?series=%5Bmozilla-inbound,38229d537cd5265f429b0916d523305fa6ce1993,1,1%5D&series=%5Bautoland,38229d537cd5265f429b0916d523305fa6ce1993,1,1%5D) and it now seems pretty clear that bug 1314091 is the culprit. :bgrins, could you have a look? -- Talos has detected a Firefox performance regression from push 4bbe0c7e648909d6118e9cc4eea14296569db80b. As author of one of the patches included in that push, we need your help to address this regression. Regressions: 3% tpaint osx-10-10 opt 366.76 -> 378.84 2% tpaint windows8-64 opt 296.5 -> 303.06 2% tpaint windows8-64 pgo 232.75 -> 237.64 You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=4800 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

https://hg.mozilla.org/mozilla-central/rev/4bbe0c7e648909d6118e9cc4eea14296569db80b has to do with syncing the lightweight theme and devtools prefs. I'll do some debugging to figure out if it's simply adding the observer that slows things down or if it's the pref / lwt access.

(In reply to Brian Grinstead [:bgrins] from comment #1) > https://hg.mozilla.org/mozilla-central/rev/ > 4bbe0c7e648909d6118e9cc4eea14296569db80b has to do with syncing the > lightweight theme and devtools prefs. I'll do some debugging to figure out > if it's simply adding the observer that slows things down or if it's the > pref / lwt access. Again, just guessing, but my guess would be that we're now setting the devtoolstheme attribute on the root of the window at a different time than before (used to be in onload, now after delayedstartup finishes) and that that triggers a reflow/restyle/extra paint, which is what slows things down. Though AFAICT setting the attribute should be a no-op from the POV of styling by now (for non-devtools UI, and I presume we don't show the devtools UI for tpaint, so...). So maybe that hypothesis is not right. :-\

(In reply to :Gijs from comment #2) > (In reply to Brian Grinstead [:bgrins] from comment #1) > > https://hg.mozilla.org/mozilla-central/rev/ > > 4bbe0c7e648909d6118e9cc4eea14296569db80b has to do with syncing the > > lightweight theme and devtools prefs. I'll do some debugging to figure out > > if it's simply adding the observer that slows things down or if it's the > > pref / lwt access. > > Again, just guessing, but my guess would be that we're now setting the > devtoolstheme attribute on the root of the window at a different time than > before (used to be in onload, now after delayedstartup finishes) and that > that triggers a reflow/restyle/extra paint, which is what slows things down. > Though AFAICT setting the attribute should be a no-op from the POV of > styling by now (for non-devtools UI, and I presume we don't show the > devtools UI for tpaint, so...). So maybe that hypothesis is not right. :-\ So commenting out the logic inside the observers didn't have any effect: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=7b5a398ca123083515a8c4f93b5c78e6b2a86770&newProject=try&newRevision=03d3cc6a87c8ba6dcc7653731636fcbc366647a3&framework=1&filter=tpaint&showOnlyImportant=0 / https://hg.mozilla.org/try/rev/0b598cfc928112410e30cf0a2ffc4341ed28ae72. Something to do with the devtoolstheme attribute would make sense. I'll look at that next.

It looks like removing the attribute does fix the regression (and then some): https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f5739344fd48ec4f3abf2168f48ea4aa08455948&newProject=try&newRevision=44b62dbfa366f6052cce7f86bb16a4b3ef94e31b&framework=1&filter=tpaint&showOnlyImportant=0. Of course we can't just remove the attribute - maybe we need to wait until delayedstartup as suggested in Comment 2

Actually, the theme is already not set until "browser-delayed-startup-finished": https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/devtools-browser.js#171 and https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/devtools-browser.js#519

(In reply to Brian Grinstead [:bgrins] from comment #5) > Actually, the theme is already not set until > "browser-delayed-startup-finished": > https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/ > devtools-browser.js#171 and > https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/ > devtools-browser.js#519 Yes, my suggestion was that we used to set the attribute earlier and setting it later is what's causing the regression, because we're going through layout + composite more often before we manage to paint. I don't know *why* that would happen though. Are there any rules still hanging off that attribute that I've forgotten about?

Have a promising push that instead of setting the attribute on the root element it sets it on browser-bottombox so that we can style gcli correctly: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f5739344fd48ec4f3abf2168f48ea4aa08455948&newProject=try&newRevision=d21cb40274644654e7a01fb6a1b1a5148c708192&framework=1&filter=tpaint&showOnlyImportant=0 / https://hg.mozilla.org/try/rev/0641c3bf63dd4bd1051f3db166ab07727e931bc8. Unfortunately we also need to style the devtools-side-splitter / devtools-horizontal-splitter which is inside #content so I'll need to see if the improvement still holds up.

Setting it on the two individual elements also seems to do the trick: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f9e0e4711360428082aa22e61f88c7e814b414d8&newProject=try&newRevision=1d95d96deb9cd99a983d3e448573b81250e58f6b&framework=1&filter=tpaint&showOnlyImportant=0 / https://hg.mozilla.org/try/rev/9541842b9a72bbbd4a61dfd362a1dab853f77a2f

Not in love with the fix but it does appear to fix the regression: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f9e0e4711360428082aa22e61f88c7e814b414d8&newProject=try&newRevision=7013c2330ef4a7b46aedf319953e22127b6e2d7c&framework=1&filter=tpaint&showOnlyImportant=0. We can't directly set an attribute on gcli and the splitter elements since they aren't there at startup, so I picked the nearest parent elements to apply the attribute to.

Comment on attachment 8828637 [details] Bug 1331449 - Set the [devtoolstheme] attribute on more specific nodes to improve tpaint; https://reviewboard.mozilla.org/r/105960/#review106994 rs=me if this fixes the tpaint thing, though note that this is technically a devtools patch so I dunno if you want review from :ochameau or someone else who knows that code better.

Thanks for the review, the main part I wanted a review on is setting the attribute on the #browser-bottombox and #content nodes. With this patch, I am seeing a tpaint improvement that at least offsets the regression: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f9e0e4711360428082aa22e61f88c7e814b414d8&newProject=try&newRevision=7f04744c56715a0b197fc25933aff65e27c43c9c&framework=1&filter=tpaint&showOnlyImportant=0

Pushed by bgrinstead@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ddad29b58dc1 Set the [devtoolstheme] attribute on more specific nodes to improve tpaint;r=Gijs

:wlach, can you verify this is fixed?

I have verified this is all good on the graphs! Thanks for getting this fixed :)