Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Description • 8 years ago

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?

	Till Schneidereit [:till]
	Comment 1 • 8 years ago

(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.

	:Paolo Amadini
	Comment 2 • 8 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 8 years ago

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.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 4 • 8 years ago

(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.

	Martin Best (:mbest)
	Comment 5 • 8 years ago

(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?

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 8 years ago

> 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/

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 7 • 8 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 8 • 8 years ago

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.

	Till Schneidereit [:till]
	Comment 9 • 8 years ago

(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.

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 10 • 8 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 11 • 8 years ago

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

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 12 • 8 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 13 • 8 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 14 • 8 years ago

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)

	(not currently active) Ted Mielczarek
	Comment 15 • 8 years ago

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`.

	:Paolo Amadini
	Comment 16 • 8 years ago

(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

	:Paolo Amadini
	Comment 17 • 8 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi] Reporter
	Comment 18 • 8 years ago

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?

	:Paolo Amadini
	Comment 19 • 8 years ago

(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.

	Till Schneidereit [:till]
	Comment 20 • 8 years ago

(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.

	(no longer active)
	Comment 21 • 8 years ago

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