Closed Bug 1331449 Opened 3 years ago Closed 3 years ago

2.1 - 3.29% tpaint (osx-10-10, windows8-64) regression on push 4bbe0c7e648909d6118e9cc4eea14296569db80b (Fri Jan 13 2017)

Categories

(Firefox :: Theme, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 53
Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 --- verified

People

(Reporter: wlach, Assigned: bgrins)

References

Details

(Keywords: perf, regression, talos-regression)

Attachments

(1 file)

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
Flags: needinfo?(bgrinstead)
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.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Component: Untriaged → Theme
(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.
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.
Flags: needinfo?(bgrinstead)
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.
Attachment #8828637 - Flags: review?(gijskruitbosch+bugs) → review+
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
https://hg.mozilla.org/mozilla-central/rev/ddad29b58dc1
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
:wlach, can you verify this is fixed?
Flags: needinfo?(wlachance)
I have verified this is all good on the graphs!  Thanks for getting this fixed :)
Flags: needinfo?(wlachance)
Status: RESOLVED → VERIFIED

the issue is now fixed
changing status to fixed
i am wondering if it is fixed?

Have a good summer,
Paul's Tech Tips

Flags: needinfo?(wlachance)
Flags: needinfo?(bgrinstead)
Flags: needinfo?(paulflaherty5)
Flags: needinfo?(wlachance)
Flags: needinfo?(paulflaherty5)
Flags: needinfo?(bgrinstead)
You need to log in before you can comment on or make changes to this bug.