1379620 - 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)

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

[Ionuț Goldan [:igoldan]]

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

Comment 1

[Ionuț Goldan [:igoldan]]

: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?

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Bug 1376892 should fix the cart and tart regression

Comment 3

[Dão Gottwald [:dao]]

(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.

Comment 4

[Ionuț Goldan [:igoldan]]

: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%)?

Comment 5

[Dão Gottwald [:dao]]

(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.

Comment 6

[Ionuț Goldan [:igoldan]]

(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.

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

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?

Comment 8

[Dão Gottwald [:dao]]

(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.

Comment 9

[Ionuț Goldan [:igoldan]]

I filed bug 1383658 for this issue.

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

for this bug, are we closing this as wontfix given that we believe tart isn't measuring properly?

Comment 11

[Dão Gottwald [:dao]]

(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.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=91d6bca244694539a83ab39e10e77ac96a10df9c&newProject=try&newRevision=a52430e351027bda72cb83b5e59eb7ded9a8b955&framework=1&showOnlyImportant=0

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1379620 - Disable stop-reload animation while a tab is opening or closing.

Review commit: https://reviewboard.mozilla.org/r/161688/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/161688/

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161688/diff/1-2/

Comment 16

[Dão Gottwald [:dao]]

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.

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 18

[Dão Gottwald [:dao]]

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?

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

(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.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161688/diff/2-3/

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=bc48d2ca199a6f74f52c6be4842ac366480a8382&newProject=try&newRevision=f93e0e135eb2df45f394017f43a77671ddbb181c&framework=1&showOnlyImportant=0

Comment 22

[Dão Gottwald [:dao]]

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

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161688/diff/3-4/

Comment 24

[Dão Gottwald [:dao]]

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

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161688/diff/4-5/

Comment 26

[Dão Gottwald [:dao]]

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.

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161688/diff/5-6/

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f1a531978f4
Disable stop-reload animation while a tab is opening or closing. r=dao

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 30

[Jared Wein [:jaws] (please needinfo? me)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161688/diff/6-7/

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9f57aa650148
Disable stop-reload animation while a tab is opening or closing. r=dao

Comment 32

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out on request from jaws:

https://hg.mozilla.org/integration/autoland/rev/189eb3e434bd0c69ec3ad5c366b12d523e6b0eeb

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9f57aa650148256831be610b0da9df08cc66522c&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=120475147&repo=autoland

Comment 33

[Jared Wein [:jaws] (please needinfo? me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c066980955b47b2132fe332fc182d14373e290f6

Comment 34

[Jared Wein [:jaws] (please needinfo? me)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161688/diff/7-8/

Comment 35

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 36

[Jared Wein [:jaws] (please needinfo? me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca222461fa5fc9dadb76f3afaf3689962f830be0

Comment 37

[Jared Wein [:jaws] (please needinfo? me)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161688/diff/8-9/

Comment 38

[Dão Gottwald [:dao]]

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

Comment 39

[Jared Wein [:jaws] (please needinfo? me)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161688/diff/9-10/

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fba389c98329
Disable stop-reload animation while a tab is opening or closing. r=dao

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fba389c98329