2.05 - 31.67% cart / damp / tart / tp5o % Processor Time / tp5o Main_RSS (linux64, osx-cross, windows7-32) regression on push 66528bf3d70ddd5a43a9017605dd4942ed09d122 (Sat Jul 8 2017)

RESOLVED FIXED in Firefox 57

Status

()

Firefox
General
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: igoldan, Assigned: jaws)

Tracking

(Depends on: 1 bug, {perf, regression, talos-regression})

55 Branch
Firefox 57
perf, regression, talos-regression
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 disabled, firefox57 fixed)

Details

(Whiteboard: [reserve-photon-animation])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Talos has detected a Firefox performance regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=66528bf3d70ddd5a43a9017605dd4942ed09d122

As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

 32%  tp5o % Processor Time windows7-32 pgo e10s     43.14 -> 56.81
 31%  tp5o % Processor Time windows7-32 opt e10s     43.68 -> 57.03
 20%  cart summary linux64 pgo e10s                  7.63 -> 9.17
 17%  cart summary linux64 opt e10s                  8.49 -> 9.96
 15%  tart summary osx-cross opt e10s                11.09 -> 12.79
 10%  tart summary linux64 opt e10s                  5.01 -> 5.52
 10%  cart summary osx-cross opt e10s                11.99 -> 13.21
 10%  tart summary linux64 pgo e10s                  4.29 -> 4.72
  4%  tart summary windows7-32 opt e10s              6.87 -> 7.11
  3%  tart summary windows7-32 pgo e10s              4.95 -> 5.11
  2%  tp5o Main_RSS linux64 opt e10s                 186,097,278.53 -> 190,461,111.77
  2%  damp summary windows7-32 opt e10s              249.13 -> 254.78
  2%  tp5o Main_RSS linux64 pgo e10s                 179,556,267.87 -> 183,311,095.44
  2%  tp5o Main_RSS windows7-32 opt e10s             122,356,461.08 -> 124,859,416.05


You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=7755

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
Component: Untriaged → General
:jaws These are most of the regressions observed after landing in the animation feature. Please provide up-to-date status regarding which ones should be fixed and which should be accepted - e.g I know the tp5o regressions are acceptable, due to trade-offs, but what about the other regressions?
Flags: needinfo?(jaws)
Bug 1376892 should fix the cart and tart regression
Flags: needinfo?(jaws)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> Bug 1376892 should fix the cart and tart regression

There would still be a regression when opening a remote page in a new tab, which users do all the time. Bug 1376892 would hide this because tart uses local pages. That's gaming the system. Talos reporting no regressions isn't an end in itself -- it's the real world performance that matters at the end of the day.
:jaws I see bug 1376892 relanded, and it brought the improvement on tart and cart. What's left out of the regression list are the DAMP regressions. Can you look over that too, because on Windows 7 is pretty high (~10%)?
Flags: needinfo?(jaws)
(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #4)
> :jaws I see bug 1376892 relanded, and it brought the improvement on tart and
> cart. What's left out of the regression list are the DAMP regressions.

Did you see comment 3? Do you understand what I was trying to say? In all likelihood there's still a tab animation regression, talos just doesn't see it anymore.

We should probably have a tart mode that works with example.com rather than just chrome pages.
Flags: needinfo?(ionut.goldan)
(In reply to Dão Gottwald [::dao] from comment #5)
> We should probably have a tart mode that works with example.com rather than
> just chrome pages.

Will look over this. Thanks for this observation.
Flags: needinfo?(ionut.goldan) → needinfo?(jmaher)
lets get a bug on file to discuss what to measure.  I have heard that TART is not a useful test in the new world of photon, maybe we just disable it?  A new bug to discuss this would be best.  :igoldan, could you file one and summarize where we are at?
Flags: needinfo?(jmaher) → needinfo?(ionut.goldan)
(In reply to Joel Maher ( :jmaher) (UTC-8) from comment #7)
> lets get a bug on file to discuss what to measure.  I have heard that TART
> is not a useful test in the new world of photon, maybe we just disable it?

That depends on how exactly the new tab animations will be implemented, I guess. Though even if we can do the animation off the main thread, it could still get bogged down by other expensive operations running in parallel. So having some sort of tart will probably still make sense.

Anyway, as it stands it's not even clear that new tab animations will be ready for 57, so regressions with the current animations are quite relevant.
Depends on: 1383658
I filed bug 1383658 for this issue.
Flags: needinfo?(ionut.goldan)
for this bug, are we closing this as wontfix given that we believe tart isn't measuring properly?
Flags: needinfo?(dao+bmo)
(In reply to Joel Maher ( :jmaher) (UTC-8) from comment #10)
> for this bug, are we closing this as wontfix given that we believe tart
> isn't measuring properly?

I don't understand. Regardless of whether tart sees the regression (it used to but now it doesn't), if there's a tab animation regression when opening a remote page in a new tab, then we should fix that or establish that accepting the regression is the right trade-off which so far nobody has advocated for.
Flags: needinfo?(dao+bmo)
I've been looking in to this. I should have commented in the bug sooner. I am working on a patch that will disable the stop/reload animation if there is a tab animation in progress.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Flags: needinfo?(jaws)
Flags: qe-verify-
Whiteboard: [photon-animation]
Iteration: --- → 56.4 - Aug 1
Priority: -- → P1
Whiteboard: [photon-animation] → [reserve-photon-animation]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 16

a year ago
mozreview-review
Comment on attachment 8890549 [details]
Bug 1379620 - Disable stop-reload animation while a tab is opening or closing.

https://reviewboard.mozilla.org/r/161688/#review168772

I had already prepared feedback for this. The transitonend listeners shouldn't be needed because we already have one. _handleNewTab and _endRemoveTab is where you should update tabAnimationsInProgress. I'd probably just increment and decrement the counter even if we skip the animation for a particular tab.
Attachment #8890549 - Flags: review-
Sorry, the patch had been up for review for some time now and I was chatting with Gijs over Vidyo about changing some of these event listeners so I redirected to him with those new changes.
Sure, too bad I couldn't get to this earlier. (Well, I could have r-'d earlier, but wanted to give it another thought.) Regardless, I still do not think we want these extra event listeners. Can you explain what value they provide over doing this in _handleNewTab and _endRemoveTab?
Flags: needinfo?(jaws)
(In reply to Dão Gottwald [::dao] from comment #18)
> Sure, too bad I couldn't get to this earlier. (Well, I could have r-'d
> earlier, but wanted to give it another thought.) Regardless, I still do not
> think we want these extra event listeners. Can you explain what value they
> provide over doing this in _handleNewTab and _endRemoveTab?

I have a patch that changes to using _handleNewTab and _endRemoveTab, just waiting on Talos numbers first. Try/Treeherder is being very slow.
Flags: needinfo?(jaws)
Comment hidden (mozreview-request)

Comment 22

a year ago
mozreview-review
Comment on attachment 8890549 [details]
Bug 1379620 - Disable stop-reload animation while a tab is opening or closing.

https://reviewboard.mozilla.org/r/161688/#review168922

::: browser/base/content/tabbrowser.xml:2690
(Diff revision 3)
>                this._lastRelatedTab = t;
>              }
>  
> +            // This field is updated regardless if we actually animate
> +            // since it's important that we keep this count correct in all cases.
> +            gBrowser.tabAnimationsInProgress++;

use 'this'

::: browser/base/content/tabbrowser.xml:2900
(Diff revision 3)
>  
>              this._blurTab(aTab);
>              aTab.style.maxWidth = ""; // ensure that fade-out transition happens
>              aTab.removeAttribute("fadein");
>  
> +            gBrowser.tabAnimationsInProgress++;

You need to increment the counter in _beginRemoveTab to correctly mirror _endRemoveTab.

::: browser/base/content/tabbrowser.xml:6806
(Diff revision 3)
>          <parameter name="tab"/>
>          <body><![CDATA[
>            if (tab.parentNode != this)
>              return;
>            tab._fullyOpen = true;
> +          gBrowser.tabAnimationsInProgress--;

use this.tabbrowser
Attachment #8890549 - Flags: review?(dao+bmo) → review-
Comment hidden (mozreview-request)

Comment 24

a year ago
mozreview-review
Comment on attachment 8890549 [details]
Bug 1379620 - Disable stop-reload animation while a tab is opening or closing.

https://reviewboard.mozilla.org/r/161688/#review168962

::: browser/base/content/tabbrowser.xml:2929
(Diff revision 4)
>            <![CDATA[
>              if (aTab.closing ||
>                  this._windowIsClosing)
>                return false;
>  
> +            gBrowser.tabAnimationsInProgress++;

You need to move this after the last 'return false' as we only want to do this when we'll actually close the tab.

::: browser/base/content/tabbrowser.xml:2929
(Diff revision 4)
>            <![CDATA[
>              if (aTab.closing ||
>                  this._windowIsClosing)
>                return false;
>  
> +            gBrowser.tabAnimationsInProgress++;

please use 'this' here too

::: browser/base/content/tabbrowser.xml:3059
(Diff revision 4)
>          <body>
>            <![CDATA[
>              if (!aTab || !aTab._endRemoveArgs)
>                return;
>  
> +            gBrowser.tabAnimationsInProgress--;

ditto
Attachment #8890549 - Flags: review?(dao+bmo) → review-
Comment hidden (mozreview-request)
status-firefox55: --- → unaffected
status-firefox56: --- → disabled

Comment 26

a year ago
mozreview-review
Comment on attachment 8890549 [details]
Bug 1379620 - Disable stop-reload animation while a tab is opening or closing.

https://reviewboard.mozilla.org/r/161688/#review169142

::: browser/base/content/tabbrowser.xml:3067
(Diff revision 5)
>              aTab._endRemoveArgs = null;
>  
>              if (this._windowIsClosing) {
>                aCloseWindow = false;
>                aNewTab = false;
>              }

Let's move this.tabAnimationsInProgress--; until after the above block since all those lines are concerned with _endRemoveArgs, i.e. they belong together.
Attachment #8890549 - Flags: review?(dao+bmo) → review+
Iteration: 56.4 - Aug 1 → 57.1 - Aug 15
Comment hidden (mozreview-request)

Comment 28

a year ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4f1a531978f4
Disable stop-reload animation while a tab is opening or closing. r=dao
Backed out for failing browser_aboutStopReload.js:

https://hg.mozilla.org/integration/autoland/rev/04870a1e761f4f932bc03e974102707b70b95fbf

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=4f1a531978f4b4354afa705d1a83014fcf1f0321&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=120420488&repo=autoland

09:36:57     INFO - TEST-PASS | browser/base/content/test/about/browser_aboutStopReload.js | Test finished: stop-reload does not animate when navigating between local URIs - true == true - 
09:36:57     INFO - Leaving test bound checkDontShowStopFromLocalURI
09:36:57     INFO - Entering test bound checkDontShowStopFromNonLocalURI
09:36:57     INFO - Buffered messages logged at 09:36:13
09:36:57     INFO - TEST-PASS | browser/base/content/test/about/browser_aboutStopReload.js | Test finished: stop-reload does not animate when navigating to local URI from non-local URI - true == true - 
09:36:57     INFO - Leaving test bound checkDontShowStopFromNonLocalURI
09:36:57     INFO - Entering test bound checkDoShowStopOnNewTab
09:36:57     INFO - Buffered messages finished
09:36:57     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/about/browser_aboutStopReload.js | Test timed out - 
09:36:57     INFO - GECKO(1658) | MEMORY STAT | vsize 4514MB | residentFast 481MB | heapAllocated 104MB
09:36:57     INFO - TEST-OK | browser/base/content/test/about/browser_aboutStopReload.js | took 45013ms
09:36:57     INFO - Not taking screenshot here: see the one that was previously logged
09:36:57     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/about/browser_aboutStopReload.js | Found a tab after previous test timed out: https://example.com/ - 
09:36:57     INFO - checking window state
Flags: needinfo?(jaws)
Comment hidden (mozreview-request)
Flags: needinfo?(jaws)

Comment 31

a year ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9f57aa650148
Disable stop-reload animation while a tab is opening or closing. r=dao
Comment hidden (mozreview-request)
Comment on attachment 8890549 [details]
Bug 1379620 - Disable stop-reload animation while a tab is opening or closing.

browser_aboutStopReload.js was failing when running the test directory because the gBrowser.tabAnimationsInProgress became stuck at a non-zero value. This was happening because browser_aboutHome.js opened a tab then closed it before the tab had finished opening[1] (thus _handleNewTab wasn't getting called).

I added a check in _beginRemoveTab to decrement gBrowser.tabAnimationsInProgress if the tab wasn't fully open.

[1] http://searchfox.org/mozilla-central/rev/f0e4ae5f8c40ba742214e89aba3f554da0b89a33/browser/base/content/test/about/browser_aboutHome.js#539-542
Attachment #8890549 - Flags: review+ → review?(dao+bmo)
Comment hidden (mozreview-request)
Attachment #8890549 - Flags: review?(dao+bmo)

Comment 38

a year ago
mozreview-review
Comment on attachment 8890549 [details]
Bug 1379620 - Disable stop-reload animation while a tab is opening or closing.

https://reviewboard.mozilla.org/r/161688/#review169668

::: browser/base/content/test/about/browser.ini:20
(Diff revision 9)
>  skip-if = os == "linux" # Bug 924307
>  [browser_aboutHome.js]
>  [browser_aboutHome_wrapsCorrectly.js]
> +[browser_aboutNetError.js]
> +[browser_aboutStopReload.js]
> +run-if = nightly_build # Photon only

Please remove the run-if while you're at it so that this works when 57 moves to beta.

::: browser/base/content/test/about/browser_aboutStopReload.js:85
(Diff revision 9)
> +
> +  await waitForNoAnimation(stopReloadContainer);
> +  let tab = await BrowserTestUtils.openNewForegroundTab({gBrowser,
> +                                                        waitForStateStop: true});
> +  await BrowserTestUtils.waitForCondition(() => {
> +    info(`Waiting for tabAnimationsInProgress to equal 0, currently ${gBrowser.tabAnimationsInProgress}`);

Why use a template string for this?
Same length and likely performs better:

"Waiting for tabAnimationsInProgress to equal 0, currently " + gBrowser.tabAnimationsInProgress

::: browser/base/content/test/about/browser_aboutStopReload.js:104
(Diff revision 9)
>    await waitForNoAnimation(stopReloadContainer);
>    let tab = await BrowserTestUtils.openNewForegroundTab({gBrowser,
>                                                          opening: "about:robots",
>                                                          waitForStateStop: true});
> +  await BrowserTestUtils.waitForCondition(() => {
> +    info(`Waiting for tabAnimationsInProgress to equal 0, currently ${gBrowser.tabAnimationsInProgress}`);

same
Attachment #8890549 - Flags: review?(dao+bmo) → review+
Comment hidden (mozreview-request)

Comment 40

a year ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fba389c98329
Disable stop-reload animation while a tab is opening or closing. r=dao

Comment 41

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fba389c98329
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
status-firefox-esr52: --- → unaffected
You need to log in before you can comment on or make changes to this bug.