1272870 - 6% damp (linux64) regression on push bec48bfd05a8 (Fri May 13 2016)

Status: RESOLVED FIXED

(DevTools :: Framework, defect, P1)

Description

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

Talos has detected a Firefox performance regression from push bec48bfd05a8. As author of one of the patches included in that push, we need your help to address this regression.

This is a list of all known regressions and improvements related to the push:
https://treeherder.mozilla.org/perf.html#/alerts?id=1195

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#DAMP

Reproducing and debugging the regression:
If you would like to re-run this Talos test on a potential fix, use try with the following syntax:
try: -b o -p linux64 -u none -t g2-e10s --rebuild 5  # add "mozharness: --spsProfile" to generate profile data

(we suggest --rebuild 5 to be more confident in the results)

To run the test locally and do a more in-depth investigation, first set up a local Talos environment:
https://wiki.mozilla.lorg/Buildbot/Talos/Running#Running_locally_-_Source_Code

Then run the following command from the directory where you set up Talos:
talos --develop -e [path]/firefox -a damp --e10s

Making a decision:

As the patch author we need your feedback to help us handle this regression.
*** 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

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

this is the push that caused the regression:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=be6dbfceb79e570e6859200c2c5a8673f58c277e&tochange=bec48bfd05a8c6703aab63181441869a3dcaa4cc

I am still collecting more data to see if this is on other platforms or on non-e10s as well.  

looking at the subtests, we have this view:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=fx-team&originalRevision=be6dbfceb79e&newProject=fx-team&newRevision=bec48bfd05a8&originalSignature=2becc39f2d1a40c23b420d838d99330621d50c26&newSignature=2becc39f2d1a40c23b420d838d99330621d50c26&framework=1

^ keep in mind more data will come in over the next few hours.  Stay tuned, I will try to update the bug when I check in later today or tomorrow.

Jan, can you look into why this might be happening.  Just understanding this could help justify or lead us to a fix to reduce the regression.

Comment 2

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

all data has come in:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx-team&originalRevision=be6dbfceb79e&newProject=fx-team&newRevision=bec48bfd05a8&framework=1

we have these regressions:
linux64:  2.5% damp  (pgo is 3%)
linux64:  6% damp e10s
win7:     2.5% damp
win7:     1.6% damp e10s (pgo shows 2.5%)
win8:     3.5% damp (pgo shows 7%)
win8 pgo: 3% damp e10s

I believe that is all that we have.  OSX has some changes, but it is a noisier platform, so it is hard to quantify that fully.

Comment 3

[Brian Grinstead [:bgrins]]

Could we wait to call _buildNotificationBox until getNotificationBox is called (or make this.notificationBox a getter)?  If that doesn't fix it, we could also try making React and ReactDOM getters instead of blocking toolbox startup on them being loaded.

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1272870.patch

Patch attached.
- React loaded when used for the first time
- The notification box built when needed

Try push (with the arguments from comment #0):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e53305ec7587

Honza

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8752788 [details] [diff] [review]
bug1272870.patch

Review of attachment 8752788 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks good, please confirm with Joel that the try push is showing a fix for the regression

::: devtools/client/framework/toolbox.js
@@ +484,5 @@
> +   * Avoid loading React during (Firefox) start time.
> +   */
> +  get React() {
> +    if (!this._react) {
> +      this._react = this.browserRequire(

Is browserRequire be smart enough for us to just return this.browserRequire(...), or would it reimport the module / not give an identical object each time in that case?

@@ +490,5 @@
> +    }
> +    return this._react;
> +  },
> +
> +  get ReactDOM() {

There is a line that sets React and ReactDOM to null during destroy, in this case it'd need to change to _react and _reactDOM

@@ +827,5 @@
> +   * Build the notification box as soon as needed.
> +   */
> +  get notificationBox() {
> +    if (!this._notificationBox) {
> +      this._notificationBox = this._buildNotificationBox();

I'd prefer to see the logic in _buildNotificationBox folded into this condition so there's one less function on the Toolbox object, and it's only ever needed for this getter.

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1272870.patch

(In reply to (Unavailable until May 23) Brian Grinstead [:bgrins] from comment #5)
> Comment on attachment 8752788 [details] [diff] [review]
> bug1272870.patch
> 
> Review of attachment 8752788 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall looks good, please confirm with Joel that the try push is showing a
> fix for the regression
> 
> ::: devtools/client/framework/toolbox.js
> @@ +484,5 @@
> > +   * Avoid loading React during (Firefox) start time.
> > +   */
> > +  get React() {
> > +    if (!this._react) {
> > +      this._react = this.browserRequire(
> 
> Is browserRequire be smart enough for us to just return
> this.browserRequire(...), or would it reimport the module / not give an
> identical object each time in that case?
Yes, identical, done.

> 
> @@ +490,5 @@
> > +    }
> > +    return this._react;
> > +  },
> > +
> > +  get ReactDOM() {
> 
> There is a line that sets React and ReactDOM to null during destroy, in this
> case it'd need to change to _react and _reactDOM
Done

> 
> @@ +827,5 @@
> > +   * Build the notification box as soon as needed.
> > +   */
> > +  get notificationBox() {
> > +    if (!this._notificationBox) {
> > +      this._notificationBox = this._buildNotificationBox();
> 
> I'd prefer to see the logic in _buildNotificationBox folded into this
> condition so there's one less function on the Toolbox object, and it's only
> ever needed for this getter.
Done

Honza

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

New try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f51ede27803

(the above try has errors, I think it should be fixed now)

Honza

Comment 8

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

hey, this seems to do the trick:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx-team&originalRevision=4c709f7a0335&newProject=try&newRevision=7f51ede27803&framework=1&filter=damp&showOnlyImportant=0

thanks for fixing this!

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Joel Maher (:jmaher) from comment #8)
> hey, this seems to do the trick:
> https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx-
> team&originalRevision=4c709f7a0335&newProject=try&newRevision=7f51ede27803&fr
> amework=1&filter=damp&showOnlyImportant=0
> 
> thanks for fixing this!

Nice, thanks all!

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing against fx-team tip.

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1272870.patch

Rebased

Honza

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ecc4da006c2f

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 49 since this is a recent regression.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

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

this looks to do the trick!  Unfortunately there was another regression which landed right after this (still investigating it), so our graphs can be misleading.

Thanks for fixing this.