Bug 164753 – [JSC] do not reference AwaitExpression Promises in async function Promise chain

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Caitlin Potter (:caitp)]

[JSC] do not reference AwaitExpression Promises in async function Promise chain

[Caitlin Potter (:caitp)]

Created attachment 294781 [details]
Patch

[Caitlin Potter (:caitp)]

Comment on attachment 294781 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294781&action=review

> Source/JavaScriptCore/builtins/AsyncFunctionPrototype.js:40
> +        value = generator.@generatorNext.@call(generator.@generatorThis, generator, @GeneratorStateExecuting, @undefined, @GeneratorResumeModeNormal, generator.@generatorFrame);

It might make sense to make this a part of the wrapper function's bytecode, but I'm not entirely sure how to allocate the closures on lines 53 and 54 with the right captured variables. What do you think?

> JSTests/stress/async-await-throw-loop.js:5
> +// TODO: @runFTLNoCJIT("--gcMaxHeapSize=2000000")

TODO: runNoCJIT()

[Yusuke Suzuki]

Comment on attachment 294781 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294781&action=review

> Source/JavaScriptCore/ChangeLog:7
> +

Please write detailed comments on ChangeLog. It's super useful when we understand / debug related things :)

> Source/JavaScriptCore/builtins/AsyncFunctionPrototype.js:32
> +        @throwTypeError("Async function illegally spawned");

Could you add a test to ensure this error is raised in some situation correctly?

>> Source/JavaScriptCore/builtins/AsyncFunctionPrototype.js:40
>> +        value = generator.@generatorNext.@call(generator.@generatorThis, generator, @GeneratorStateExecuting, @undefined, @GeneratorResumeModeNormal, generator.@generatorFrame);
> 
> It might make sense to make this a part of the wrapper function's bytecode, but I'm not entirely sure how to allocate the closures on lines 53 and 54 with the right captured variables. What do you think?

If we can merge @asyncFunctionResume and @asyncFunctionSpawn, we do not need to emit code in the wrapper function side I think.

> Source/JavaScriptCore/builtins/AsyncFunctionPrototype.js:54
> +        function(value) { @asyncFunctionResume(generator, promiseCapability, value, @GeneratorResumeModeNormal); },
> +        function(error) { @asyncFunctionResume(generator, promiseCapability, error, @GeneratorResumeModeThrow); });

In both functions, we do not return the result of the function here. So I think we do not need to have 2 functions, right?
I think we just use `@asyncFunctionResume`. Create the result promise capability in the async function itself and pass it to @asyncFunctionResume can drop @asyncFunctionSpawn.

> Source/JavaScriptCore/builtins/AsyncFunctionPrototype.js:75
> +            return;

Let's return promiseCapability.@promise as similar to @asyncFunctionSpawn.

> Source/JavaScriptCore/builtins/AsyncFunctionPrototype.js:80
> +        return;

Let's return promiseCapability.@promise as similar to @asyncFunctionSpawn.

> Source/JavaScriptCore/builtins/AsyncFunctionPrototype.js:85
> +        function(error) { @asyncFunctionResume(generator, promiseCapability, error, @GeneratorResumeModeThrow); });

Let's return promiseCapability.@promise as similar to @asyncFunctionSpawn.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3468
>  

You can create the result promise capability here.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3471
> +        auto var = generator.variable(generator.propertyNames().builtinNames().asyncFunctionSpawnPrivateName());

And use @asyncFunctionResume instead.

[Caitlin Potter (:caitp)]

Created attachment 294798 [details]
Patch

[Caitlin Potter (:caitp)]

Created attachment 294799 [details]
Patch

[Caitlin Potter (:caitp)]

Comment on attachment 294781 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294781&action=review

>> Source/JavaScriptCore/ChangeLog:7
>> +
> 
> Please write detailed comments on ChangeLog. It's super useful when we understand / debug related things :)

added a bit more

>> Source/JavaScriptCore/builtins/AsyncFunctionPrototype.js:32
>> +        @throwTypeError("Async function illegally spawned");
> 
> Could you add a test to ensure this error is raised in some situation correctly?

There should not be any observable code that can run into this. It's more like an assertion than anything else. Same as the one in @asyncFunctionResume

>> Source/JavaScriptCore/builtins/AsyncFunctionPrototype.js:54
>> +        function(error) { @asyncFunctionResume(generator, promiseCapability, error, @GeneratorResumeModeThrow); });
> 
> In both functions, we do not return the result of the function here. So I think we do not need to have 2 functions, right?
> I think we just use `@asyncFunctionResume`. Create the result promise capability in the async function itself and pass it to @asyncFunctionResume can drop @asyncFunctionSpawn.

done

[Yusuke Suzuki]

Comment on attachment 294799 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 294799 [details]
Patch

Clearing flags on attachment: 294799

Committed r208726: <http://trac.webkit.org/changeset/208726>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.