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

RESOLVED FIXED in Firefox 46

Status

()

Firefox
Developer Tools: Framework
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jmaher, Assigned: jryans)

Tracking

({perf, regression})

Trunk
Firefox 46
perf, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [talos_regression])

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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
(Reporter)

Comment 1

2 years ago
: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!
Flags: needinfo?(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?
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(jryans)
Flags: needinfo?(bgrinstead)
(Reporter)

Comment 3

2 years ago
thanks for the quick reply ;)
(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.
Flags: needinfo?(bgrinstead)
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?
Flags: needinfo?(nfitzgerald)
(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.
Flags: needinfo?(nfitzgerald)
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.
Flags: needinfo?(nfitzgerald)
Assignee: nobody → jryans
Status: NEW → ASSIGNED
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/
Attachment #8705431 - Flags: review?(nfitzgerald)
Attachment #8705431 - Flags: review?(nfitzgerald) → review+
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! :)
: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.
Flags: needinfo?(jmaher)
Summary: 2% linux/win damp regression on inbound (v.45) on Dec 21, from push 0df8478ca614 → 2% linux/win damp regression on inbound (v.46) on Dec 21, from push 0df8478ca614
(Reporter)

Comment 11

2 years ago
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.
Flags: needinfo?(jmaher)
Looks like the g2 runs suggest it does fix the regression, so let's land it.

Comment 13

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

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/92cb3865720d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
(Reporter)

Comment 15

2 years ago
verified on graph server! Thanks for fixing this

Comment 16

2 years ago
[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
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.