Talos has detected a Firefox performance regression from push 76ad1c764e5c626b20b87099fe9b822e21dc23e9. As author of one of the patches included in that push, we need your help to address this regression. Regressions: 11% kraken summary osx-10-10 opt 1465.97 -> 1621.92 10% tsvgx summary osx-10-10 opt 500.81 -> 548.42 7% damp summary linux64 opt 337.84 -> 362.28 7% tp5o summary osx-10-10 opt 304.65 -> 325.9 6% damp summary linux64 pgo 274.88 -> 292.12 6% tsvgr_opacity summary osx-10-10 opt 432.98 -> 459.73 5% tp5o responsiveness linux64 opt 50.35 -> 52.8 5% tp5o responsiveness linux64 pgo 27.33 -> 28.59 3% tsvgx summary linux64 opt 531.18 -> 549.25 3% tp5o summary linux64 opt 362.75 -> 373.88 3% tp5o summary linux64 pgo 258.32 -> 265.28 2% tsvgx summary linux64 pgo 373.07 -> 382.31 Improvements: 10% tart summary osx-10-10 opt 11.15 -> 10.09 9% tart summary linux64 opt e10s 6.61 -> 6.02 8% tart summary linux64 opt 6.27 -> 5.79 6% tart summary linux64 pgo e10s 5.05 -> 4.75 5% tart summary linux64 pgo 4.49 -> 4.26 You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=5695 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

jwatt / jaws, any thoughts on this?

(In reply to igoldan from comment #0) > Talos has detected a Firefox performance regression from push > 76ad1c764e5c626b20b87099fe9b822e21dc23e9. Can you give an actual link to hg.m.o in future? If one person does it, it saves everyone else having to manually copy, figure out the link, paste and go.

Speaking of which, it's: https://hg.mozilla.org/mozilla-central/rev/76ad1c764e5c626b20b87099fe9b822e21dc23e9

_Maybe_ bug 1351986 will help, but I'd be amazed if the DOCTYPE declaration made so much difference.

I took a look at Kraken. Flipping nglayout.debug.paint_flashing_chrome to true shows that the title text in the tab is invalidating constantly, as is the connecting throbber. This seems to be because Kraken makes a network request for kraken.css something like 140 times while running, causing the tab text to change and the throbber spinning to restart each time.

(In reply to Jonathan Watt [:jwatt] from comment #5) > ...causing the tab text to change... Well, not change, just set to the parent document's title (which is always the same) each time a subdocument makes a network request. It seems like we shouldn't restart the throbber spin if it's already spinning (probably something that needs to be changed in the frontend code)... Or invalidate the title text if it is set to the same value (something that would need to be changed in the gecko code, but is probably bug 725221 which stalled because the spec requires the text node to be replaced regardless)...

(In reply to Jonathan Watt [:jwatt] from comment #5) > I took a look at Kraken. Flipping nglayout.debug.paint_flashing_chrome to > true shows that the title text in the tab is invalidating constantly, as is > the connecting throbber. Hmm, we removed layer="true" from .tab-throbber in bug 759252 because of bug 759252 comment 36. I can check if this is still an issue.

(In reply to Dão Gottwald [::dao] from comment #7) > (In reply to Jonathan Watt [:jwatt] from comment #5) > > I took a look at Kraken. Flipping nglayout.debug.paint_flashing_chrome to > > true shows that the title text in the tab is invalidating constantly, as is > > the connecting throbber. > > Hmm, we removed layer="true" from .tab-throbber in bug 759252 because of bug > 759252 comment 36. I can check if this is still an issue. Yep, layer="true" still makes the throbber disappear. Also, in the current Nightly on Linux I can't seem to reproduce what you're seeing. The tab label seems to invalidate only sporadically while running Kraken, probably because it changes to "Connecting..." every now and then.

Perfherder results, https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=67c6a220d380fcee0aa1cb770e4a3d8c5629b094&newProject=try&newRevision=440035359a53476422143bf1fe188b952b9b80af&framework=1&showOnlyImportant=0

Comment on attachment 8853567 [details] Bug 1352085 - Only update the title and busy/progress attribute of the tab when it has changed. https://reviewboard.mozilla.org/r/125626/#review128236 ::: browser/base/content/tabbrowser.xml:1442 (Diff revision 1) > <method name="setTabTitleLoading"> > <parameter name="aTab"/> > <body> > <![CDATA[ > - aTab.label = this.mStringBundle.getString("tabs.connecting"); > + let connectingString = this.mStringBundle.getString("tabs.connecting"); > + // Setting the label to its identity will invalidate the text It might not be immediately obvious what "will invalidate the text" means in this context to some people who may come across thing. It might be worth trying to make that clearer by saying something like "Setting the label to its identity can cause performance issues because it invalidates the text and causes a repaint (see bug 725221)."

Comment on attachment 8853567 [details] Bug 1352085 - Only update the title and busy/progress attribute of the tab when it has changed. https://reviewboard.mozilla.org/r/125626/#review128258 ::: browser/base/content/tabbrowser.xml:596 (Diff revision 1) > if (!this._shouldShowProgress(aRequest)) > return; > > - if (this.mTotalProgress) > + if (this.mTotalProgress && > + this.mTab.getAttribute("progress") != "true") > this.mTab.setAttribute("progress", "true"); I don't see how this would make a difference. ::: browser/base/content/tabbrowser.xml:682 (Diff revision 1) > } > > if (this._shouldShowProgress(aRequest)) { > if (!(aStateFlags & nsIWebProgressListener.STATE_RESTORING)) { > + if (this.mTab.getAttribute("busy") != "true") > - this.mTab.setAttribute("busy", "true"); > + this.mTab.setAttribute("busy", "true"); ditto ::: browser/base/content/tabbrowser.xml:1446 (Diff revision 1) > - aTab.label = this.mStringBundle.getString("tabs.connecting"); > + let connectingString = this.mStringBundle.getString("tabs.connecting"); > + // Setting the label to its identity will invalidate the text > + // since the actual textNode still gets replaced (bug 725221). > + if (aTab.label != connectingString) { > + aTab.label = connectingString; > + } I guess this could make sense given bug 725221, but this would leave open the question why the Kraken regression is only present on Mac. I thought this could make a difference: http://searchfox.org/mozilla-central/rev/f668d2b2c1413452d536210b3ea2999eb81824f3/browser/themes/osx/browser.css#2375-2376 But no, when I port that to Linux, I still don't see the problem running Kraken. Also perfherder doesn't seem to show an improvement anywhere. Am I missing something? Does this patch make a difference for Kraken on Mac with nglayout.debug.paint_flashing_chrome=true?

I pushed it to review before getting perfherder results. I agree that this didn't show any wins. I also tried manually setting the attribute while the animation is running and paint-flashing doesn't change the color of the box and the animation doesn't "restart", further confirming the first two parts of comment #12.

Fixed by backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/d0f1f01d369641c422db403eb5550b40cae59889 https://hg.mozilla.org/integration/mozilla-inbound/rev/5110f7c040365cacc750021b6b2aeeaa74728e7e

:dao I'm confirming the backout with following results: == Change summary for alert #5971 (as of April 12 2017 08:30 UTC) == Regressions: 10% tart summary osx-10-10 opt 10.11 -> 11.1 9% tart summary linux64 opt e10s 6.01 -> 6.55 8% tart summary linux64 opt 5.73 -> 6.17 6% tart summary linux64 pgo e10s 4.69 -> 4.97 6% tart summary windows8-64 opt 6.35 -> 6.73 5% tart summary linux64 pgo 4.22 -> 4.43 Improvements: 11% tp5o % Processor Time windows7-32 pgo e10s 49.81 -> 44.44 10% kraken summary osx-10-10 opt 1605.54 -> 1452.47 9% tsvgx summary osx-10-10 opt 513.83 -> 465.34 7% damp summary linux64 pgo 269.93 -> 249.71 7% damp summary linux64 opt 335.44 -> 310.35 6% tp5o summary osx-10-10 opt 316.02 -> 298 5% tp5o % Processor Time windows7-32 opt 89.86 -> 84.99 5% tp5o % Processor Time windows7-32 pgo 84.01 -> 79.85 4% damp summary osx-10-10 opt 313.57 -> 299.73 3% tsvgx summary linux64 opt 508.74 -> 491.88 3% tpaint osx-10-10 opt 389.67 -> 377.21 3% tsvgx summary linux64 pgo 355.38 -> 344.81 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=5971

::dao If you plan on making an update to keep the old tart improvements, please let me know.

(In reply to igoldan from comment #16) > ::dao If you plan on making an update to keep the old tart improvements, > please let me know. Unfortunately, no. Maybe bug 1352119 will bring these improvements back.

::dao Thanks for the heads up!

I've filed bug 1357093 to investigate what happened here, in the hopes that we can preempt any kind of similar regressions when more GPU-powered Photon animations start to land.