1280819 - Disable async stacks by default in more cases

Status: NEW

(Core :: JavaScript Engine, defect, P3)

Description

[Olli Pettay [:smaug][[e-mail]]]

Given that async stacks cause significant regressions to performance (Bug 1152893, bug 1273481, etc), we should, IMO, just disable them by default everywhere. Currently they are enabled on non-release desktop builds.
(I'm not aware of anyone doing work to make async stack handling any faster atm)

Perhaps devtools could have a checkbox for enabling async stacks?

Comment 1

[Till Schneidereit [:till]]

(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #0)
> Perhaps devtools could have a checkbox for enabling async stacks?

Or we just enable them only when the devtools are open? For the new Promise implementation they'll be enabled either if the config flag is set to true or if the compartment is a debuggee. That seems to make sense to me.

Comment 2

[:Paolo Amadini]

We also use this feature extensively to get sane stacks in regression tests, we should ensure it's enabled during test execution, rather than always disabled.

For the rest, we could make a call on what we want to do by default for developers in Nightly and Developer Edition. I think the current state of performance isn't as bad as comment 0 suggests, especially for everyday usage rather than microbenchmarks.

If we really need to optimize for running specific microbenchmarks in Nightly and Developer Edition, we could also consider applying a restriction to the specific instance of async stack usage.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Note that the perf of async stacks also came up in bug 1273481.  Yes, microbenchmark, yes, silly, but people are actively caring about these results.  Which channels they care in is an interesting question that we should get figured out.

Comment 4

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

(In reply to Till Schneidereit [:till] from comment #1)
> (In reply to Olli Pettay [:smaug] (high review load, please consider other
> reviewers) from comment #0)
> > Perhaps devtools could have a checkbox for enabling async stacks?
> 
> Or we just enable them only when the devtools are open? For the new Promise
> implementation they'll be enabled either if the config flag is set to true
> or if the compartment is a debuggee. That seems to make sense to me.

That makes sense for me.

Comment 5

[Martin Best (:mbest)]

(In reply to Boris Zbarsky [:bz] (Out June 25-July 6) from comment #3)
> Note that the perf of async stacks also came up in bug 1273481.  Yes,
> microbenchmark, yes, silly, but people are actively caring about these
> results.  Which channels they care in is an interesting question that we
> should get figured out.

I have not seen people really hold use to this type of performance result until it's in release or we have a new feature coming that is really giving us a big jump forward and an intent to ship.  I'm not clear on what benchmarks are impacted by this?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> I have not seen people really hold use to this type of performance result until it's in release 

I've seen lots of people benchmarking nightlies.  Just talk to the JS team about it; they keep griping about it.

> I'm not clear on what benchmarks are impacted by this

Anything that uses lots of promises (e.g. streams stuff will be hit by this).  Anything that uses lots of DOM callbacks.  For the issue in bug 1273481 it was the Knockout benchmark at https://chrisharrington.github.io/demos/performance/

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Volunteers providing cleopatra profiles need to know to set the pref false, otherwise profiles can be a bit useless.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

till, could you point to the code which does https://bugzilla.mozilla.org/show_bug.cgi?id=1280819#c1
I think async stacks have caused enough trouble that we really need to disable them by default (when devtools aren't open). And possibly enable when running tests.

Comment 9

[Till Schneidereit [:till]]

(In reply to Olli Pettay [:smaug] (vacation Aug 25-28) from comment #8)
> till, could you point to the code which does
> https://bugzilla.mozilla.org/show_bug.cgi?id=1280819#c1
> I think async stacks have caused enough trouble that we really need to
> disable them by default (when devtools aren't open). And possibly enable
> when running tests.

Sure. The stacks are taken in two places:
https://dxr.mozilla.org/mozilla-central/source/js/src/builtin/Promise.cpp#137
https://dxr.mozilla.org/mozilla-central/source/js/src/builtin/Promise.cpp#427

As you can see, all that's needed is to remove the first half of the condition.

I wonder though whether it wouldn't make more sense to just set javascript.otions.asyncstack to false by default in Nightly, too.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

yeah, we could set the pref false and change CallbackObject to check also isDebuggee().
Though, it is a bit annoying that by default opening devtools would slow down execution significantly.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: disabled_async_stack.diff

https://bug1273481.bmoattachments.org/attachment.cgi?id=8753356 is my testcase here.
Without devtools time is ~700ms, devtools open ~2000.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bef212da8c92

Comment 12

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

Comment on attachment 8785623 [details] [diff] [review]
disabled_async_stack.diff

Review of attachment 8785623 [details] [diff] [review]:
-----------------------------------------------------------------

Tiny drive-by nit.

::: js/src/jsfriendapi.h
@@ +2915,5 @@
>  extern JS_FRIEND_API(JSObject*)
>  ToWindowIfWindowProxy(JSObject* obj);
>  
> +extern JS_FRIEND_API(bool)
> +CurrentCompartmentIsDebuggee(JSContext* aCx);

Nit: I would put this in js/public/Debug.h

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

ok. I put it in jsfriendapi.h since someone suggested that.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Debug.h

like this.

The change (previous patch at least) seems to break something in selftest.py. I have no idea what that is or how to run it. MDN doesn't seem to have any documentation, nor selftest.py itself. It seems to write some testfiles somehow and run them and then something... 
ted, paolo, you seem to have played with selftest.py. How does one run the tests it has?
(I don't understand why it is run as part of build [B] and not part of xpcshell tests [X]. The file is in xpcshell/ and it runs some tests)

Comment 15

[(not currently active) Ted Mielczarek]

smaug: selftest.py runs as part of PYTHON_UNIT_TESTS:
https://dxr.mozilla.org/mozilla-central/source/testing/xpcshell/moz.build#13

It doesn't run as part of the xpcshell tests because it's not xpcshell tests, it's a set of tests for the xpcshell *harness*.

Anyway, you can run that by running `mach python-test testing/xpcshell/selftest.py`.

Comment 16

[:Paolo Amadini]

(In reply to Olli Pettay [:smaug] from comment #14)
> The change (previous patch at least) seems to break something in selftest.py.

My guess is that this patch breaks proper reporting of xpcshell test stacks. We should keep async stacks always enabled during test runs because they provide valuable information on test failure.

Based on previous comments, it seems that we want three states for async stacks:

1) Always enabled
2) Enabled when the compartment is a debuggee
3) Always disabled

We should have one centralized function we can use to determine if an async stack should be captured for a given compartment. We currently duplicate the logic across call sites, which may give inconsistent results. We can call the same function from AutoSetAsyncStackForNewCalls to determine if a previously captured async stack should be used.

It seems to me that the defaults we want are:

1) Always enabled: default only when running test harnesses
2) Enabled when the compartment is a debuggee: default on Nightly and Aurora
3) Always disabled: default on Beta and Release

Comment 17

[:Paolo Amadini]

To implement the always enabled state in the test harness we could set a new preference in the harness-specific preference files, change the current preference to have three states, or add a special call at the beginning of the harness to enable async stacks.

Updating the summary so it doesn't make assumptions on the implementation.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Note, the current setup for Promises implemented in SM already enables stacks with debuggee, even in release versions (I don't know if Promises implemented in SM is enabled by default yet).

why do we want to have different behavior on release/beta than in aurora/nightly?

Comment 19

[:Paolo Amadini]

(In reply to Olli Pettay [:smaug] from comment #18)
> why do we want to have different behavior on release/beta than in
> aurora/nightly?

If others are fine with enabling async stacks when the debugger is open in Beta and Release, I'm fine with that! It would make it much easier for developers to use them. Maybe we should do that in another bug so if that causes any issues, the work in this bug does not get backed out.

Comment 20

[Till Schneidereit [:till]]

(In reply to Olli Pettay [:smaug] from comment #18)
> Note, the current setup for Promises implemented in SM already enables
> stacks with debuggee, even in release versions (I don't know if Promises
> implemented in SM is enabled by default yet).

They are. On Aurora right now, riding the trains.



(In reply to :Paolo Amadini from comment #19)
> (In reply to Olli Pettay [:smaug] from comment #18)
> > why do we want to have different behavior on release/beta than in
> > aurora/nightly?
> 
> If others are fine with enabling async stacks when the debugger is open in
> Beta and Release, I'm fine with that! It would make it much easier for
> developers to use them. Maybe we should do that in another bug so if that
> causes any issues, the work in this bug does not get backed out.

Could we disable them while the profiler is running? That's my main concern: people trying to understand some perf issue using the profiler and then seeing something completely different because their workload happens to cause lots of promises to be used. (More and more realistic going forward, what with async/await and all.)

That'd make it even more important to have a centralized way of checking whether async stacks should be recorded.

Comment 21

[(no longer active)]

To reduce the pain caused by this when using the Gecko Profiler at least, bug 1329861 disables this pref while profiling.

Comment 22

[Andrew Creskey [:acreskey]]

Because the performance tests use nightly and thus have async stacks on for desktop this came us the reason why we're seeing ~2-3% lower speedometer results within raptor than with the browsertime framework (which disabled async stacks).
In Bug 1595537 I proposed that we disable async stacks for the performance profile since this more closely matches the user experience.
These performance results are used for catching regressions and improvements and also for comparing ourselves to other browsers.

Comment 23

[Jim Blandy :jimb]

By the way, bug 1589523 describes a way for us to include async calls in JS error stacks with none of this overhead. That technique doesn't help provide stacks for DOM callbacks or promise creation and resolution, however.

Comment 24

[Logan Smyth [:loganfsmyth]]

I'm working through some of the issues discussed here in https://bugzilla.mozilla.org/show_bug.cgi?id=1601179#c4 if anyone would like to offer feedback to my questions there.