Closed Bug 1158133 Opened 5 years ago Closed 5 years ago

Add a way to disable async stacks, and disable by default on mobile platforms

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox39 + fixed
firefox40 + fixed
firefox41 + fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

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.
Tracking this bug instead of bug 1163139. We should disable on Beta and Release for Desktop until bug 1142577 is done.
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)
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)
Attached file MozReview Request: bz://1158133/paolo (obsolete) —
/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/
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.
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)
Feel free to redirect the review as appropriate. I think we need to land this before Beta goes to build though.
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)
(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)
Yes, thanks Paolo! We will need to uplift this for 39 beta 1 once it lands.
> 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?
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)
(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.
Attachment #8606300 - Flags: review?(jimb)
Attachment #8606300 - Flags: review?(bzbarsky)
Attachment #8606300 - Flags: review?(bent.mozilla)
Comment on attachment 8606300 [details]
MozReview Request: bz://1158133/paolo

Looks good, thanks!
Attachment #8606300 - Flags: review?(bent.mozilla) → review+
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)
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)
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)
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)
(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)
...may reveal that there are no issues in the wild.
OK thanks, let's aim for beta 2 then, later next week if that works out.
Comment on attachment 8606300 [details]
MozReview Request: bz://1158133/paolo

https://reviewboard.mozilla.org/r/8821/#review7983
Attachment #8606300 - Flags: review?(jimb) → review+
jimb and fitzgen can review async stack. I pinged jimb and looks like he already got to it.
Flags: needinfo?(nihsanullah)
Flags: needinfo?(jimb)
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)
Waiting on word from Paolo
Flags: needinfo?(jimb)
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)
I've also updated the patch to take care of bug 1168178, where preferences in "firefox.js" aren't applied to xpcshell tests.
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)
Attachment #8606300 - Attachment is obsolete: true
Attachment #8620153 - Flags: review+
Blocks: 1148593
https://hg.mozilla.org/mozilla-central/rev/f537b304247e
Assignee: nobody → paolo.mozmail
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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?
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 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+
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: --- → ?
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)
(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: ? → ---
As noted, some tests may require adjustment as the patch is uplifted.
Keywords: checkin-needed
Blocks: 1280819
You need to log in before you can comment on or make changes to this bug.