2.48 - 10.64% damp / kraken / tp5o / tp5o responsiveness / tsvgr_opacity / tsvgx (linux64, osx-10-10) regression on push 76ad1c764e5c626b20b87099fe9b822e21dc23e9 (Mon Mar 27 2017)

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: igoldan, Assigned: dao)

Tracking

({perf, regression, talos-regression})

Trunk
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 unaffected, firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55 fixed)

Details

Attachments

(1 obsolete attachment)

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
Assignee

Comment 1

2 years ago
jwatt / jaws, any thoughts on this?
Flags: needinfo?(jwatt)
Flags: needinfo?(jaws)
(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.
Flags: needinfo?(jwatt) → needinfo?(ionut.goldan)
_Maybe_ bug 1351986 will help, but I'd be amazed if the DOCTYPE declaration made so much difference.
Depends on: 1351986
Flags: needinfo?(ionut.goldan)
Target Milestone: Firefox 55 → ---
Version: 53 Branch → Trunk
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)...
Assignee

Comment 7

2 years ago
(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.
Assignee

Comment 8

2 years ago
(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.
Comment hidden (mozreview-request)

Comment 11

2 years ago
mozreview-review
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)."
Assignee

Comment 12

2 years ago
mozreview-review
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?
Attachment #8853567 - Flags: review?(dao+bmo)
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.
Attachment #8853567 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Depends on: 1354080
Flags: needinfo?(mconley)
: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.
Flags: needinfo?(dao+bmo)
Assignee

Comment 17

2 years ago
(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.
Flags: needinfo?(dao+bmo)
::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.
Flags: needinfo?(mconley)
You need to log in before you can comment on or make changes to this bug.