Remove wrapper functions for async functions/generators
Categories
(Core :: JavaScript Engine, enhancement)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox67 | --- | fixed |
People
(Reporter: anba, Assigned: anba)
References
Details
Attachments
(8 files, 4 obsolete files)
|
55.13 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
|
34.36 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
|
75.33 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
|
21.93 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
|
16.63 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
|
36.10 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
|
38.74 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
|
63.26 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
It doesn't look like it's too hard to remove the wrapper functions for async functions/generators. This should help to prevent me adding another sec-bug like bug 1528057. :-P
|
Assignee | |
Comment 1•6 years ago
|
||
In preparation for later patches, the internal GeneratorObject for async functions is now stored in the PromiseReaction instead of the PromiseObject. This matches the existing behaviour for async generators and removes the (shared) PromiseSlot_AwaitGenerator slot from PromiseObject.
Later patches will also clean-up some of the extra lines introduced here, so this more or less all just a transient state.
|
||
Comment 2•6 years ago
|
||
|
|
Assignee | |
Comment 3•6 years ago
|
||
Adds a new base class for generator objects called AbstractGeneratorObject. This isn't the most original name, but there's lot of existing code spread across the whole code base which refers to "generators", so it seemed useful to me to keep GeneratorObject in the name for the new base class.
This class will be used as the super-class for distinct generator objects created for generator functions, async functions, and async generator functions. For now there's only one concrete sub-class GeneratorObject, but the next patch will add more sub-classes.
I've used auto in a couple of places instead of AbstractGeneratorObject to avoid awkward line breaks, because of the longer class name AbstractGeneratorObject when compared to just GeneratorObject and the new 80 columns line length limit.
Drive-by change:
- Cleaned up some #includes.
- Cleaned access to
RESUME_INDEX_SLOTin debugger code.
|
|
Assignee | |
Comment 4•6 years ago
|
||
Adds new generator object sub-classes for async functions (AsyncFunctionGeneratorObject) and async generators (AsyncGeneratorGeneratorObject). AsyncGeneratorGeneratorObject will later be combined with the existing AsyncGeneratorObject, so the silly "GeneratorGenerator" name won't exist for long :-)
Also adds specialised self-hosted functions to resume these generator objects, because the extra suspension checks and generator closing steps in Generator[Next,Return,Throw] aren't needed here. (Or actually the closing steps happen now in native code.)
This patch adds extra checks for the generator result value in WrappedAsyncFunction and WrappedAsyncGenerator, these will be removed later on. (The new test "js/src/jit-test/tests/debug/onEnterFrame-async-resumption-06.js" actually asserts on current tip, too!)
The various "js/src/jit-test/tests/debug/onEnterFrame-async-resumption-*" tests will change a few times with next patches, so (light spoiler alert!) you don't need to worry about Skynet too much. :-D
|
|
Assignee | |
Comment 5•6 years ago
|
||
Combines AsyncGeneratorObject and AsyncGeneratorGeneratorObject, and then removes the extra native wrapper for async generators.
For the most part only code removal:
- Some places now need extra checks for async functions (of the form
isAsync && !isGenerator), these will be removed in a later patch.
Added a new jit-test for off-thread compilation of different function types which can be run from the shell. This is currently only covered implicitly in browser tests, but not when running jstest/jit-tests from the shell.
|
|
Assignee | |
Comment 6•6 years ago
|
||
More or less a continuation of part 1. The result promise for async functions is now allocated when creating the generator object of async functions and stored within that generator object.
Two things noteworthy:
- This changes the allocation site of the promise, so we now need to use the parent frame for
AutoSetAsyncStackForNewCallsinAsyncFunctionResume. - I'm using
NewBuiltinClassInstanceinAsyncFunctionGeneratorObject::create, because that ensures the allocation can use the new-object cache. The new-object cache can only be used when theprotois non-null. So even though we never expose the internal generator object for async functions, so maybe a more natural solution would be to usenullas the proto, it's necessary to use the default proto (here: %ObjectPrototype%) for performance reasons.
|
|
Assignee | |
Comment 7•6 years ago
|
||
We need two additional changes before we can finally remove the wrapper for async functions:
- We need to fulfill resp. reject the async function result promise on
returnresp. when an exception was thrown. - And we also need to perform
Awaitdirectly in the async function bytecode.
This and the next patch will make these changes.
Adds new JSOP_ASYNCRESOLVE bytecode to resolve the result promise of async functions.
return statements now perform an additional JSOP_ASYNCRESOLVE to fulfill the result promise, that means for example we're going to emit the following bytecode sequence for return x.
JSOP_GETGNAME "x"
JSOP_SETRVAL
JSOP_GETRVAL
JSOP_GETALIASEDVAR ".generator"
JSOP_ASYNCRESOLVE "fulfill"
JSOP_SETRVAL
JSOP_GETALIASEDVAR ".generator"
JSOP_FINALYIELDRVAL
instead of
JSOP_GETGNAME "x"
JSOP_SETRVAL
JSOP_GETALIASEDVAR ".generator"
JSOP_FINALYIELDRVAL
(Obviously the JSOP_SETRVAL + JSOP_GETRVAL sequence is not optimal and maybe it's also more performant to perform some stack juggling to avoid the two JSOP_GETALIASEDVAR. But I thought for reviewing it made more sense to use the more minimal approach which requires less additional code changes and keep the peephole optimisations for follow-up bugs.)
Promise rejections due to exceptions are a bit more difficult to handle, because there's no explicit code location like a return statement which need to be adjusted. Instead this patch simply adds a big try-catch block around the whole function body (yuk!). This seems to be an okay-ish solution as far as interpreter and baseline performance is concerned, but maybe we'll need something better for Ion (because some Ion-optimisations are disabled in try-catch blocks). But since we don't yet Ion-compile async functions the try-catch block solution should work for now. (The back-up plan is to hook into the places where we're currently calling HandleClosingGeneratorReturn, reject the result promise and then perform a forced-return via js::jit::ForcedReturn.)
There's a TODO note in the patch for BytecodeEmitter, which will be removed in the next patch.
There are existing debugger tests which require us to handle debugger-inserted return/throw completions for async functions. And because we no longer handle exceptions/return values in the native async function wrapper, but now within the async function bytecode, the AdjustGeneratorResumptionValue debugger function had to be err... "adjusted" as well. In AdjustGeneratorResumptionValue we're now resolving the result promise, which can lead to enqueueing new promise jobs, so I had to move some calls to JS::AutoDebuggerJobQueueInterruption::runJobs to ensure any jobs enqueued while the debugger is running are run before any previous, user-generated promise jobs.
This uncovered an existing bug in js::InternalJobQueue::SavedQueue, where the js::InternalJobQueue::draining_ state wasn't properly reset. A regression test for this bug was added at "js/src/jit-test/tests/debug/save-queue-resets-draining.js" (fail in current tip).
And then there's also "js/src/jit-test/tests/debug/Frame-onStep-async-02.js" which started to fail with this patch, because of bug 1470558. I've added a band-aid to ScriptedOnPopHandler::onPop to kind of make it possible to detect await for debugger consumers, but the proper fix should still happen in bug 1470558.
I think that's all for this patch. Turned out to be a bit more tricky than the previous patches, but the final patch should make it worthwhile nonetheless! :-)
|
|
Assignee | |
Comment 8•6 years ago
|
||
This patch adds a new JSOP_ASYNCAWAIT bytecode for awaiting values in await. The important bit here isn't actually to move the awaiting part from native code to bytecode, but that the new opcode allows to remove JSOP_INITIALYIELD for async functions, so that when an async function is called, we no longer have need to suspend and then immediately resume the function execution.
JSOP_ASYNCAWAIT pushes the async function result promise on the stack, so that when await is called for the very first time and the execution is suspended, the top stack value (= the result promise) is passed via JSOP_AWAIT back to the caller, similar to how JSOP_INITIALYIELD passes the generator object to the caller code.
We're now emitting the following bytecode sequence for await x:
JSOP_GETGNAME "x"
JSOP_TRYSKIPAWAIT
JSOP_NOT
JSOP_IFEQ
JSOP_JUMPTARGET
JSOP_GETALIASEDVAR ".generator"
JSOP_ASYNCAWAIT
JSOP_GETALIASEDVAR ".generator"
JSOP_AWAIT
JSOP_DEBUGAFTERYIELD
JSOP_JUMPTARGET
instead of
JSOP_GETGNAME "x"
JSOP_TRYSKIPAWAIT
JSOP_NOT
JSOP_IFEQ
JSOP_JUMPTARGET
JSOP_GETALIASEDVAR ".generator"
JSOP_AWAIT
JSOP_DEBUGAFTERYIELD
JSOP_JUMPTARGET
Additionally we now can move JSOP_GENERATOR to the very top of the generated bytecode, even before initialising default parameters. This ensures the try-catch block for promise rejections added in the previous patch no longer needs to handle the case when ".generator" is not yet present.
And more debugger tests which had to be updated: Because JSOP_INITIALYIELD is no longer emitted, there's one less on-enter-frame event, so the test expectations had to be adjusted.
|
|
Assignee | |
Comment 9•6 years ago
|
||
And finally the last patch. Similar to patch 4 this is mostly code removal.
GlobalObject::initAsyncFunction was missing a call to JSObject::setDelegate, I've moved it to NewSingletonObjectWithFunctionPrototype, so we don't need to call setDelegate manually in all callers to NewSingletonObjectWithFunctionPrototype.
|
|
||
Comment 10•6 years ago
|
||
|
|
||
Updated•6 years ago
|
|
|
||
Comment 11•6 years ago
|
||
|
|
||
Updated•6 years ago
|
|
|
||
Comment 12•6 years ago
|
||
will review remaining parts tonight.
|
|
||
Comment 13•6 years ago
|
||
|
|
||
Comment 14•6 years ago
|
||
|
|
||
Comment 15•6 years ago
|
||
|
|
Assignee | |
Comment 16•6 years ago
|
||
Updated part 1 per review comments, carrying r+.
|
|
Assignee | |
Comment 17•6 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #10)
Can you add some more comment what kind of situation "then" and "else"
branches correspond to?
also in the later patch that adds more branches.
With all patches applied the if-else statements look like
Rooted<AbstractGeneratorObject*> genObj(cx);
if (!fun->isAsync()) {
genObj = GeneratorObject::create(cx, fun);
} else if (fun->isGenerator()) {
genObj = AsyncGeneratorObject::create(cx, fun);
} else {
genObj = AsyncFunctionGeneratorObject::create(cx, fun);
}
which should be self-documenting. Do you have any specific comments in my mind which should be added here to improve clarity?
|
|
Assignee | |
Comment 18•6 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #11)
- MOZ_MUST_USE bool emitInitHomeObject(bool isAsyncNonGenerator = false);
I prefer not-having bool parameter as much as possible.
but I suppose this method be changed again later
Yes, later patches will remove the bool parameter. :-)
::: js/src/vm/JSFunction.cpp
@@ +613,5 @@if (mode == XDR_DECODE) {
RootedObject proto(cx);
- if ((firstword & IsGenerator) || (firstword & IsAsync)) {
if ((firstword & IsGenerator) && (firstword & IsAsync)) {can you assign both
firstword & IsGeneratorandfirstword & IsAsyncto
local variable?
I've already prepared, but not yet posted, a follow-up patch which will clean this up, therefore I'd like to keep this part as is for now.
|
|
Assignee | |
Comment 19•6 years ago
|
||
Updated comments for JSOP_ASYNCRESOLVE per review comments, carrying r+.
|
|
Assignee | |
Comment 20•6 years ago
|
||
Updated part 7 per review comments, carrying r+.
(In reply to Tooru Fujisawa [:arai] from comment #14)
@@ +1635,2 @@
*/ \
- MACRO(JSOP_ASYNCAWAIT, 151, "async-await", NULL, 1, 2, 1, JOF_BYTE) \
having JSOP_AWAIT and JSOP_ASYNCAWAIT is very confusing...
how do you think renaming JSOP_AWAIT to JSOP_ASYNCSUSPEND?
given now JSOP_AWAIT just suspends the execution.
Agreed, I'm not really happen to have both JSOP_AWAIT and JSOP_ASYNCAWAIT. I think I'd like to try to use the new JSOP_ASYNCAWAIT in async generators somehow, too. And if that works out, I'll try to merge JSOP_AWAIT and JSOP_ASYNCAWAIT, so that there's ultimately only a single JSOP_AWAIT opcode. But before we can do that, async generators need a few changes. For example this code should be changed for performance reasons (creating an iterator result object only to wrap the "value" property is a bit wasteful) and also for spec compliance reasons (the access to "value" needs to happen in the async generator execution): The following test case should print "SUCCESS", but currently throws an uncaught exception.
async function* f() {
var token = {};
try {
yield* {
[Symbol.asyncIterator]() {
return {
next() {
return {
done: false,
get value() {
throw token;
}
};
}
};
}
};
} catch (e) {
print((e === token) ? "SUCCESS" : "FAIL");
}
}
f().next();
|
|
Assignee | |
Comment 21•6 years ago
|
||
Updated part 8 per review comments, carrying r+.
|
|
Assignee | |
Comment 22•6 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b7dea9f7cf19e61b6c447a61329d89078b8c0b5
|
||
Comment 23•6 years ago
|
||
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dd26ddfc373
Part 1: Store the internal generator of async functions in the Promise reaction. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/90d0e91224a9
Part 2: Add abstract super class for GeneratorObject. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/e125746cec95
Part 3: Add separate AbstractGeneratorObject subclasses for async functions and async generators. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/135c13d4ceba
Part 4: Remove wrapper function for async generators. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/48fb1e2b6e97
Part 5: Store result promise in the internal generator object of async functions. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/b84fd1d91da2
Part 6: Add JSOP_ASYNCRESOLVE to fulfill/reject an async function promise. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfcdd2084fea
Part 7: Remove initial-yield for async functions. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/55b6a8c4e015
Part 8: Remove wrapper function for async functions. r=arai
|
||
Comment 24•6 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/1dd26ddfc373
https://hg.mozilla.org/mozilla-central/rev/90d0e91224a9
https://hg.mozilla.org/mozilla-central/rev/e125746cec95
https://hg.mozilla.org/mozilla-central/rev/135c13d4ceba
https://hg.mozilla.org/mozilla-central/rev/48fb1e2b6e97
https://hg.mozilla.org/mozilla-central/rev/b84fd1d91da2
https://hg.mozilla.org/mozilla-central/rev/dfcdd2084fea
https://hg.mozilla.org/mozilla-central/rev/55b6a8c4e015
|
||
Comment 25•6 years ago
|
||
== Change summary for alert #19689 (as of Wed, 27 Feb 2019 19:20:17 GMT) ==
Improvements:
1% Base Content JS osx-10-10 opt stylo 4,077,512.00 -> 4,032,372.00
1% Base Content JS windows10-64-pgo-qr opt stylo 4,132,861.33 -> 4,092,798.67
For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=19689
Description
•