Joel Maher ( :jmaher ) (UTC -8) Reporter
	Description • 8 years ago

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

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 1 • 8 years ago

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.

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 2 • 8 years ago

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.

	Brian Grinstead [:bgrins]
	Comment 3 • 8 years ago

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.

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 4 • 8 years ago

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

	Brian Grinstead [:bgrins]
	Comment 5 • 8 years ago

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.

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 6 • 8 years ago

(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

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 7 • 8 years ago

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

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 8 • 8 years ago

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!

	Brian Grinstead [:bgrins]
	Comment 9 • 8 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 8 years ago

Needs rebasing against fx-team tip.

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 11 • 8 years ago

Rebased

Honza

	Pulsebot
	Comment 12 • 8 years ago

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

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 13 • 8 years ago

Tracking for 49 since this is a recent regression.

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 8 years ago
bugherder

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

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 15 • 8 years ago

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.