Closed
Bug 1234600
Opened 5 years ago
Closed 5 years ago
2% linux/win damp regression on inbound (v.46) on Dec 21, from push 0df8478ca614
Categories
(DevTools :: Framework, defect)
DevTools
Framework
Tracking
(firefox46 fixed)
RESOLVED
FIXED
Firefox 46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: jmaher, Assigned: jryans)
References
Details
(Keywords: perf, regression, Whiteboard: [talos_regression])
Attachments
(1 file)
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•5 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)
Assignee | ||
Comment 2•5 years ago
|
||
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•5 years ago
|
||
thanks for the quick reply ;)
Comment 4•5 years ago
|
||
(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)
Comment 5•5 years ago
|
||
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)
Assignee | ||
Comment 6•5 years ago
|
||
(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)
Comment 7•5 years ago
|
||
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 | ||
Updated•5 years ago
|
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•5 years ago
|
||
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)
Updated•5 years ago
|
Attachment #8705431 -
Flags: review?(nfitzgerald) → review+
Comment 9•5 years ago
|
||
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! :)
Assignee | ||
Comment 10•5 years ago
|
||
: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)
Assignee | ||
Updated•5 years ago
|
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•5 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)
Assignee | ||
Comment 12•5 years ago
|
||
Looks like the g2 runs suggest it does fix the regression, so let's land it.
Comment 14•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/92cb3865720d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Reporter | ||
Comment 15•5 years ago
|
||
verified on graph server! Thanks for fixing this
Comment 16•5 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
Updated•5 years ago
|
Version: unspecified → Trunk
Updated•3 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•