Closed
Bug 1158133
Opened 9 years ago
Closed 9 years ago
Add a way to disable async stacks, and disable by default on mobile platforms
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla41
People
(Reporter: Paolo, Assigned: Paolo)
References
(Blocks 2 open bugs)
Details
Attachments
(1 file, 1 obsolete file)
39 bytes,
text/x-review-board-request
|
jimb
:
review+
bent.mozilla
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details |
Async stack collection is a useful feature though it may use more CPU and memory resources, like reported in bug 1152893. For this reason, we should allow the JS engine to run with a configuration where this feature is disabled, and disable it by default on mobile platforms where the trade-offs are different than on Desktop.
Assignee | ||
Comment 1•9 years ago
|
||
Tracking this bug instead of bug 1163139. We should disable on Beta and Release for Desktop until bug 1142577 is done.
tracking-firefox39:
--- → ?
Assignee | ||
Comment 2•9 years ago
|
||
Where should we place the preference or flag to disable async stacks in the JS engine? I think what we want to do is make AutoSetAsyncStackForNewCalls a no-op, so we save memory as well as CPU time when we capture later. We'll then need to check whether the feature is enabled or not in some JS-implemented tests in Gecko. Something user-settable would be nice but we could also do something at build time.
Flags: needinfo?(jimb)
Flags: needinfo?(bzbarsky)
Comment 3•9 years ago
|
||
If we want to do it via a preference, then presumably we want to do it via the runtime options like we do the other preferences that affect the JS engine. See ReloadPrefsCallback and the various other pref stuff in js/xpconnect/src/XPCJSRuntime.cpp and the LoadRuntimeOptions bits in dom/workers/RuntimeService.cpp. If we want to just do a compile-time thing, then of course we can just do that with ifdefs inside the JS engine.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 4•9 years ago
|
||
/r/8823 - Bug 1158133 - Add a way to disable async stacks, and disable by default on mobile platforms. Pull down this commit: hg pull -r ec1b17ffb2572da812761f437380a11cf70533b1 https://reviewboard-hg.mozilla.org/gecko/
Assignee | ||
Comment 5•9 years ago
|
||
https://reviewboard.mozilla.org/r/8821/#review7515 This seems to do the job. It's unclear to me what LoadRuntimeOptions in dom/workers/RuntimeService.cpp does, given that most options have a default value of "true", but I've added a line there similar to the existing ones as well.
Assignee | ||
Comment 6•9 years ago
|
||
Comment on attachment 8606300 [details] MozReview Request: bz://1158133/paolo /r/8823 - Bug 1158133 - Add a way to disable async stacks, and disable by default on mobile platforms. Pull down this commit: hg pull -r ec1b17ffb2572da812761f437380a11cf70533b1 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8606300 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 7•9 years ago
|
||
Feel free to redirect the review as appropriate. I think we need to land this before Beta goes to build though.
Comment 8•9 years ago
|
||
Comment on attachment 8606300 [details] MozReview Request: bz://1158133/paolo The XPConnect parts look fine. The JS parts need review from a JS person. Maybe Jim? The worker parts could use review from bent. You're right that the setup there looks broken, and it's possible that I accidentally broke it in bug 1160311. We should probably be doing something more like the XPConnect code: setting the options unconditionally to the pref get results...
Attachment #8606300 -
Flags: review?(jimb)
Attachment #8606300 -
Flags: review?(bzbarsky)
Attachment #8606300 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 9•9 years ago
|
||
(In reply to Not doing reviews right now from comment #8) > We should probably be doing something more like the > XPConnect code: setting the options unconditionally to the pref get > results... Cool, though I feel we shouldn't block this bug on that change. Liz, do you think we can set the tracking flag for Firefox 39 now? The Beta build is soon and I don't think I have the energy for pinging reviewers... best to shift the burden on Release Management with the aid of our automated reminders :-)
Flags: needinfo?(lhenry)
Updated•9 years ago
|
Comment 10•9 years ago
|
||
Yes, thanks Paolo! We will need to uplift this for 39 beta 1 once it lands.
Comment 11•9 years ago
|
||
> Cool, though I feel we shouldn't block this bug on that change.
Well... right now your patch is leaving async stacks enabled in workers afaict. Is that the intent?
Assignee | ||
Comment 12•9 years ago
|
||
Comment on attachment 8606300 [details] MozReview Request: bz://1158133/paolo /r/8823 - Bug 1158133 - Add a way to disable async stacks, and disable by default on mobile platforms. Pull down this commit: hg pull -r 4ef6fb482f05423eb0dae9f10bf2dafcd8cba07a https://reviewboard-hg.mozilla.org/gecko/
Attachment #8606300 -
Flags: review?(jimb)
Attachment #8606300 -
Flags: review?(bzbarsky)
Attachment #8606300 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 13•9 years ago
|
||
(In reply to Not doing reviews right now from comment #11) > Well... right now your patch is leaving async stacks enabled in workers > afaict. Is that the intent? Ah, you're right! For some reason I missed this was main-thead code directly reading the preferences already, so it's quite a simple change.
Assignee | ||
Updated•9 years ago
|
Attachment #8606300 -
Flags: review?(jimb)
Attachment #8606300 -
Flags: review?(bzbarsky)
Attachment #8606300 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 14•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=693a530e94ff
Comment on attachment 8606300 [details]
MozReview Request: bz://1158133/paolo
Looks good, thanks!
Attachment #8606300 -
Flags: review?(bent.mozilla) → review+
Comment 16•9 years ago
|
||
Paolo, still hoping to land this for 39 beta 1. We can still take a patch before Friday morning (Pacific time) if you are ready to request uplift to mozilla-beta.
Flags: needinfo?(paolo.mozmail)
Assignee | ||
Comment 17•9 years ago
|
||
The patch is still blocked from landing on a JS peer review. If Jim is not available maybe you can find someone else to rubberstamp the patch? Unfortunately I'll not be available during the morning at Pacific Time today. In the meantime, barring any issues the development work is done and the MozReview patch is up-to-date. Maybe you can try to do a Beta build with the patch included and see if there are any issues?
Flags: needinfo?(paolo.mozmail)
Comment 18•9 years ago
|
||
Naveed is this actually blocked by JS peer review? How would I be able to tell? I'm not familiar with your team's usual process.
Flags: needinfo?(nihsanullah)
Comment 19•9 years ago
|
||
What I'm not clear on here is: does this make Firefox crash if we don't get this into beta 1? HOw significant of a performance hit is this? What is the result of it not being disabled on 39 Beta 1? If we don't think it will be all that bad of a result, and it's just that we want to land this first before other work can be done, then I won't fret about getting into the build tomorrow morning.
Flags: needinfo?(paolo.mozmail)
Assignee | ||
Comment 20•9 years ago
|
||
(In reply to Liz Henry (:lizzard) from comment #19) > What I'm not clear on here is: does this make Firefox crash if we don't get > this into beta 1? No. > HOw significant of a performance hit is this? I think the word "significant" in the summary of bug 1152893 should be definitely considered in relation to the measurement system used. As of now, we only had one instance of a very specific synthetic benchmark reporting a slowdown. We don't have reason to believe this would cause issues with websites in the wild. > What is the result of it not being disabled on 39 Beta 1? Most of the discussion is in bug 1152893. I think we definitely want to test this feature in the wild on Beta, but at the same time there was some interest in treating Promises and callback-based code with parity, basically to avoid placing the blame for any performance issues on Promises, hence the interest in delaying the existing Promises-only usage by one cycle on Beta. > If we don't think it will be all that bad of a result, and it's just that we > want to land this first before other work can be done, then I won't fret > about getting into the build tomorrow morning. The other unknowns are mobile platforms for which we suspect a performance issue (memory, CPU) may be more noticeable. But we may be wrong. My opinion is that letting ride early Beta may not reveal any issue in the wild.
Flags: needinfo?(paolo.mozmail)
Assignee | ||
Comment 21•9 years ago
|
||
...may reveal that there are no issues in the wild.
Comment 22•9 years ago
|
||
OK thanks, let's aim for beta 2 then, later next week if that works out.
Comment 23•9 years ago
|
||
Comment on attachment 8606300 [details] MozReview Request: bz://1158133/paolo https://reviewboard.mozilla.org/r/8821/#review7983
Attachment #8606300 -
Flags: review?(jimb) → review+
Comment 24•9 years ago
|
||
jimb and fitzgen can review async stack. I pinged jimb and looks like he already got to it.
Flags: needinfo?(nihsanullah)
Updated•9 years ago
|
Flags: needinfo?(jimb)
Updated•9 years ago
|
Comment 25•9 years ago
|
||
Just checking in, what is the status on this bug? Beta 4 for 39 is coming up, with fixes/verifications needed by this week to make it in.
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(jimb)
Assignee | ||
Comment 27•9 years ago
|
||
The ping was useful, I had issues with bugmail and neither comment 23 nor comment 24 made it to my inbox! I just updated and rebased the patch on the latest mozilla-central, but the trees are closed so the tryserver build has to wait.
Flags: needinfo?(paolo.mozmail)
Assignee | ||
Comment 28•9 years ago
|
||
I've also updated the patch to take care of bug 1168178, where preferences in "firefox.js" aren't applied to xpcshell tests.
Comment 29•9 years ago
|
||
Thanks Paolo! I'll put this on my list of things that need to land on the next beta (that will be beta 5 later this week). Please request uplift once this lands and looks ok on m-c.
Flags: needinfo?(paolo.mozmail)
Assignee | ||
Comment 30•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8874b3e214ee
Assignee | ||
Comment 32•9 years ago
|
||
Attachment #8606300 -
Attachment is obsolete: true
Attachment #8620153 -
Flags: review+
Assignee | ||
Comment 33•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f537b304247e
Assignee: nobody → paolo.mozmail
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee | ||
Comment 35•9 years ago
|
||
Comment on attachment 8620153 [details] MozReview Request: Bug 1158133 - Add a way to disable async stacks, and disable by default on mobile platforms. Looks like we're ready to uplift this to Aurora and Beta.
Flags: needinfo?(paolo.mozmail)
Attachment #8620153 -
Flags: approval-mozilla-beta?
Attachment #8620153 -
Flags: approval-mozilla-aurora?
Comment 36•9 years ago
|
||
Thanks Paolo. My worry here is that disabling this now (Beta 5, of 7 total beta cycles) may cause other problems to surface. Hopefully we can keep an eye on that. What are the risks at this point, of disabling async stacks? Do you expect problems and if so, how can we look for them? Can you actually fill out the uplift request form? Here it is: Approval Request Comment [Feature/regressing bug #]: [User impact if declined]: [Describe test coverage new/current, TreeHerder]: [Risks and why]: [String/UUID change made/needed]:
Flags: needinfo?(paolo.mozmail)
Comment 37•9 years ago
|
||
Comment on attachment 8620153 [details] MozReview Request: Bug 1158133 - Add a way to disable async stacks, and disable by default on mobile platforms. Approved for uplift to beta and aurora since we need to turn this off for release.
Attachment #8620153 -
Flags: approval-mozilla-beta?
Attachment #8620153 -
Flags: approval-mozilla-beta+
Attachment #8620153 -
Flags: approval-mozilla-aurora?
Attachment #8620153 -
Flags: approval-mozilla-aurora+
Comment 38•9 years ago
|
||
I mean, we need to turn it off by default on mobile. Maybe this should also have a release note. Paolo would that be useful for mobile developers?
relnote-firefox:
--- → ?
Assignee | ||
Comment 39•9 years ago
|
||
Approval Request Comment [Feature/regressing bug #]: Async Stacks [User impact if declined]: We might end up alternating the state of async stacks in Release between enabled and disabled. We don't have a lot of information about the performance implications of capturing async stacks. At present, we're only capturing those for Promise on Beta, but this hasn't apparently caused any regression in the wild. [Describe test coverage new/current, TreeHerder]: The patch updates its respective tests. Minor adjustments to the tests may be required when uplifting if the tests are different. [Risks and why]: We may have landed some code that indirectly relies on the feature. Given that this is mainly a debugging feature, this seems unlikely. [String/UUID change made/needed]: None
Flags: needinfo?(paolo.mozmail)
Assignee | ||
Comment 40•9 years ago
|
||
(In reply to Liz Henry (:lizzard) from comment #38) > I mean, we need to turn it off by default on mobile. > Maybe this should also have a release note. Paolo would that be useful for > mobile developers? Note that this patch keeps the feature enabled only on Desktop Nightly and Developer Edition. All other platforms, including mobile, has the feature disabled. I'll nominate the bug where we're enabling async stacks for the general case for a relnote.
relnote-firefox:
? → ---
Assignee | ||
Comment 41•9 years ago
|
||
As noted, some tests may require adjustment as the patch is uplifted.
Keywords: checkin-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•