1234600 - 2% linux/win damp regression on inbound (v.46) on Dec 21, from push 0df8478ca614

Status: RESOLVED FIXED

Description

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

Talos has detected a Firefox performance regression from your commit 0df8478ca614b4a02a1dc9e78a6d4ed785fa1506 in bug 1151413.  We need you to address this regression.

This is a list of all known regressions and improvements related to your bug:
http://alertmanager.allizom.org:8080/alerts.html?rev=0df8478ca614b4a02a1dc9e78a6d4ed785fa1506&showAll=1

On the page above you can see Talos 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, 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,win32 -u none -t g2  # add "mozharness: --spsProfile" to generate profile data

To run the test locally and do a more in-depth investigation, first set up a local Talos environment:
https://wiki.mozilla.org/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

Making a decision:
As the patch author we need your feedback to help us handle this regression.


Our wiki page outlines the common responses and expectations:
https://wiki.mozilla.org/Buildbot/Talos/RegressionBugsHandling


here is a comparison view to see the regressions:
https://treeherder.allizom.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=3569bc40c181&newProject=mozilla-inbound&newRevision=0df8478ca614&framework=1&filter=damp&showOnlyConfident=1

Comment 1

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

:jryans, can you comment on this.  I assume the change you made is done as optimized as possible, but if there is something we can do to reduce the regression, all the better!

Comment 2

[J. Ryan Stinnett [:jryans]]

This is expected due to the way our async stacks work currently.  There are other bugs tracking performance regressions from this if you look at the tree under bug 981514:

https://bugzilla.mozilla.org/showdependencytree.cgi?id=981514&hide_resolved=1

I think the debugging assistance provided by async stacks outweighs the performance regression.  Also, the async stack feature is disabled for release / beta channels.  So, I think it seems reasonable to accept this regression, knowing that should not actually regress release users.

:fitzgen / :bgrins, any additional comments about this?

Comment 3

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

thanks for the quick reply ;)

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #2)
> This is expected due to the way our async stacks work currently.  There are
> other bugs tracking performance regressions from this if you look at the
> tree under bug 981514:
> 
> https://bugzilla.mozilla.org/showdependencytree.cgi?id=981514&hide_resolved=1
> 
> I think the debugging assistance provided by async stacks outweighs the
> performance regression.  Also, the async stack feature is disabled for
> release / beta channels.  So, I think it seems reasonable to accept this
> regression, knowing that should not actually regress release users.
> 
> :fitzgen / :bgrins, any additional comments about this?

Are you sure that explicitly calling Cu.callFunctionWithAsyncStack will not cause any slowdown on beta/release builds?  And/or that it will work properly?

If this is in the same bucket as other async stack regressions then I suppose we can just block Bug 981514 and once perf requirements are satisfied to turn that on in release this should resolve itself.

One other idea is that we can pref this behavior in nightly / local builds only since that's when the extra debugging data seems most useful and our Dev Edition users don't have to get hit with the regression.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Dev Edition _is_ our (devtools) release channel, so I think saying that the regression is ok because it doesn't affect release is misleading.

(In reply to Brian Grinstead [:bgrins] from comment #4)
> One other idea is that we can pref this behavior in nightly / local builds
> only since that's when the extra debugging data seems most useful and our
> Dev Edition users don't have to get hit with the regression.

DEBUG_JS_MODULES maybe?

Comment 6

[J. Ryan Stinnett [:jryans]]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #5)
> Dev Edition _is_ our (devtools) release channel, so I think saying that the
> regression is ok because it doesn't affect release is misleading.

Well, fair enough about Dev. Edition.

> (In reply to Brian Grinstead [:bgrins] from comment #4)
> > One other idea is that we can pref this behavior in nightly / local builds
> > only since that's when the extra debugging data seems most useful and our
> > Dev Edition users don't have to get hit with the regression.
> 
> DEBUG_JS_MODULES maybe?

I would be fine with this myself.  What implementation would you expect here:

(a) Disable the feature pref "javascript.options.asyncstack" unless DEBUG_JS_MODULES is enabled
(b) Wrap each call of `Cu.callFunctionWithAsyncStack` in DevTools plumbing with a check on DEBUG_JS_MODULES

Note that this bug is only about the most recent such addition.  I assume you would want this to affect all DevTools uses of Cu.callFunctionWithAsyncStack.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I would go with (b).

> Note that this bug is only about the most recent such addition.
> I assume you would want this to affect all DevTools uses of
> Cu.callFunctionWithAsyncStack.

If a specific call site isn't affecting a performance metric we care about, perhaps it makes sense to leave it in. That said, our performance metrics are rather limited at this point. Either way, call sites unrelated to this bug needn't be handled in this bug.

Comment 8

[J. Ryan Stinnett [:jryans]]

Created attachment 8705431 [details]
MozReview Request: Bug 1234600 - executeSoon async stacks only when DEBUG_JS_MODULES enabled. r=fitzgen

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

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8705431 [details]
MozReview Request: Bug 1234600 - executeSoon async stacks only when DEBUG_JS_MODULES enabled. r=fitzgen

https://reviewboard.mozilla.org/r/29979/#review26791

Perfect! :)

Comment 10

[J. Ryan Stinnett [:jryans]]

:jmaher, do the Talos results look here look good to you?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=083b6fd43ccc&selectedJob=15186163

I tried to use Perfherder Compare, but it just kept saying "tests with no results: build times tsan, installer size tsan, tp4m opt, tp4m shutdown opt" or similar.

Comment 11

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

thanks for retriggering the 'g2' job.  Looking at win8, I see a result of 250, that is the original value (250-258 range), and the regression was 260-268.

Comment 12

[J. Ryan Stinnett [:jryans]]

Looks like the g2 runs suggest it does fix the regression, so let's land it.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92cb3865720d

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/92cb3865720d

Comment 15

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

verified on graph server! Thanks for fixing this

Comment 16

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel	beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS				Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected