Bug 175240 – [ESNext] Async iteration - Implement Async Generator - runtime

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[GSkachkov]

Implement Async Generator - runtime part

[GSkachkov]

Created attachment 317346 [details]
Patch

Patch with runtime part with fixed comments

[GSkachkov]

Created attachment 317362 [details]
Patch

Rebased patch

[Yusuke Suzuki]

Comment on attachment 317362 [details]
Patch

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

Reviewed LLInt, Baseline DFG, FTL, and some js part. Not reviewed large part of JS, bytecompiler yet.

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:228
> +    if (generator.@asyncGeneratorQueue === @undefined)
> +        generator.@asyncGeneratorQueue = [];

You should put it in the constructor. Use new_array_with_size or new_array.

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:230
> +    generator.@asyncGeneratorQueue.@push({resumeMode, value, promiseCapability});

Unfortunately, @push is observable because it uses Set operation. (Consider setting a setter to Array.prototype).
Let's use

@putByValDirect(generator.@asyncGeneratorQueue, [e-mail], {resumeMode, value, promiseCapability});

instead.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3562
> +        else

Let's put ASSERT in else block.

> Source/JavaScriptCore/jit/JITOpcodes.cpp:990
> +    else if (opcodeID == op_new_async_generator_func)
> +        callOperation(operationNewAsyncGeneratorFunction, dst, regT0, funcExec);
> +    else {
> +        ASSERT(opcodeID == op_new_async_generator_func);
> +        callOperation(operationNewAsyncGeneratorFunction, dst, regT0, funcExec);

Why do we have op_new_async_generator_func cases twice?

[GSkachkov]

Created attachment 317429 [details]
Patch

Fix comments

[GSkachkov]

Comment on attachment 317362 [details]
Patch

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

>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:228
>> +        generator.@asyncGeneratorQueue = [];
> 
> You should put it in the constructor. Use new_array_with_size or new_array.

Fixed

>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:230
>> +    generator.@asyncGeneratorQueue.@push({resumeMode, value, promiseCapability});
> 
> Unfortunately, @push is observable because it uses Set operation. (Consider setting a setter to Array.prototype).
> Let's use
> 
> @putByValDirect(generator.@asyncGeneratorQueue, [e-mail], {resumeMode, value, promiseCapability});
> 
> instead.

Done

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3562
>> +        else
> 
> Let's put ASSERT in else block.

Done

>> Source/JavaScriptCore/jit/JITOpcodes.cpp:990
>> +        callOperation(operationNewAsyncGeneratorFunction, dst, regT0, funcExec);
> 
> Why do we have op_new_async_generator_func cases twice?

Fixed

[Saam Barati]

Comment on attachment 317362 [details]
Patch

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

>>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:230
>>> +    generator.@asyncGeneratorQueue.@push({resumeMode, value, promiseCapability});
>> 
>> Unfortunately, @push is observable because it uses Set operation. (Consider setting a setter to Array.prototype).
>> Let's use
>> 
>> @putByValDirect(generator.@asyncGeneratorQueue, [e-mail], {resumeMode, value, promiseCapability});
>> 
>> instead.
> 
> Done

Oh wow. I didn't even realize this. It looks like we do this in a few places in our builtins, we should fix that.

[GSkachkov]

Created attachment 317696 [details]
Patch

Rebased patch with using @skip

[Yusuke Suzuki]

Comment on attachment 317696 [details]
Patch

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

Quick look. Seems very fine.

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:31
> +    const O = this;

Use the name `object` instead of `O`. I know this is the name used in the spec, but `object` is more readable.

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:37
> +        const nextResult = syncIterator.next(nextValue);
> +        const { done, value } = nextResult;

Let's make it,

const { nextDone, nextValue } = syncIterator.next(nextValue);

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:41
> +            function(result) { promiseCapability.@resolve.@call(@undefined, { done: !!done, value: result }); },

Style: add space between function and (.

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:42
> +            function(error) { promiseCapability.@reject.@call(@undefined, error); });

Ditto.

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:51
> +function return(returnValue)

Rename this `returnValue` to `value`.

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:55
> +    const O = this;

Ditto.

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:57
> +    const syncIterator = O.@syncIterator;

Could you add a test if O is not async iterator.
In that case, we should return rejected promise. But this code just throws an error, correct?

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:58
> +    const returnMethod = syncIterator.return;

This could throw an error (if return is getter). In that case, we should return rejected promise.

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:73
> +        const { done, value } = returnResult;

Rename this to `returnDone` and `returnValue` to align this to the spec.

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:78
> +            function(result) { promiseCapability.@resolve.@call(@undefined, { done, value: result }); },

Style: add space between function and (.

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:79
> +            function(error) { promiseCapability.@reject.@call(@undefined, error); });

Ditto.

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:92
> +    const O = this;

Ditto.

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:94
> +    const syncIterator = O.@syncIterator;

Could you add a test if O is not async iterator.
In that case, we should return rejected promise. But this code just throws an error, correct?

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:95
> +    const throwMethod = syncIterator.throw;

This could throw an error (if throw is getter). In that case, we should return rejected promise.

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:110
> +        const { done, value } = throwResult;

Rename `throwDone` and `throwValue`.

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:115
> +            function(result) { promiseCapability.@resolve.@call(@undefined, { done, value: result }); },

Style: add space between function and (.

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:116
> +            function(error) { promiseCapability.@reject.@call(@undefined, error); });

Ditto.

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:140
> +    this.next = @next;
> +    this.return = @return;
> +    this.throw = @throw;

They should be defined in AsyncFromSyncIteratorPrototype. Not the member of the instance.
Let's define AsyncFromSyncIteratorPrototype (maybe in C++). And set AsyncFromSyncIteratorConstructor.prototype to it.

See ArrayIterator case.

And you need to add @toStringTag with "Async-from-Sync Iterator".

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:37
> +    return queue.@shift();

The problem is that @shift() is also observable.
So, what do you think of constructing doubly-linked list here?

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:199
> +                promiseCapability.@resolve.@call(@undefined, next.value);
> +                promiseCapability.@promise.@then(function (result) { generator.@generatorState = @AsyncGeneratorStateCompleted; @asyncGeneratorResolve(generator, result, true); },
> +                                                 function (error) { generator.@generatorState = @AsyncGeneratorStateCompleted; @asyncGeneratorReject(generator, error); });
> +
> +                return @undefined;

Do we need throwawayCapability (with @promiseIsHandled = true)?

> Source/JavaScriptCore/parser/ASTBuilder.h:457
> +            parameterCount, mode, isArrowFunctionBodyExpression);

Is this correct? If it's correct, we do not need bodySourceParseMode.

> Source/JavaScriptCore/runtime/AsyncGeneratorFunctionPrototype.cpp:53
> +    putDirectWithoutTransition(vm, vm.propertyNames->toStringTagSymbol, jsString(&vm, "AsyncGenerator"), DontEnum | ReadOnly);

This should be "AsyncGeneratorFunction". Could you add a test for this?

[Saam Barati]

Comment on attachment 317696 [details]
Patch

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

>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:37
>> +        const { done, value } = nextResult;
> 
> Let's make it,
> 
> const { nextDone, nextValue } = syncIterator.next(nextValue);

I think it’ll have to be:
const {next: nextDone, value: nextValue}

[GSkachkov]

Comment on attachment 317696 [details]
Patch

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

Thanks for review!

>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:31
>> +    const O = this;
> 
> Use the name `object` instead of `O`. I know this is the name used in the spec, but `object` is more readable.

Done

>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:37
>> +        const { done, value } = nextResult;
> 
> Let's make it,
> 
> const { nextDone, nextValue } = syncIterator.next(nextValue);

Done

>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:41
>> +            function(result) { promiseCapability.@resolve.@call(@undefined, { done: !!done, value: result }); },
> 
> Style: add space between function and (.

Done

>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:42
>> +            function(error) { promiseCapability.@reject.@call(@undefined, error); });
> 
> Ditto.

Done

>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:51
>> +function return(returnValue)
> 
> Rename this `returnValue` to `value`.

Done

>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:55
>> +    const O = this;
> 
> Ditto.

Done

>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:57
>> +    const syncIterator = O.@syncIterator;
> 
> Could you add a test if O is not async iterator.
> In that case, we should return rejected promise. But this code just throws an error, correct?

Done

>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:58
>> +    const returnMethod = syncIterator.return;
> 
> This could throw an error (if return is getter). In that case, we should return rejected promise.

Done

>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:73
>> +        const { done, value } = returnResult;
> 
> Rename this to `returnDone` and `returnValue` to align this to the spec.

Done

>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:78
>> +            function(result) { promiseCapability.@resolve.@call(@undefined, { done, value: result }); },
> 
> Style: add space between function and (.

Done

>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:79
>> +            function(error) { promiseCapability.@reject.@call(@undefined, error); });
> 
> Ditto.

Done

>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:92
>> +    const O = this;
> 
> Ditto.

Done

>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:110
>> +        const { done, value } = throwResult;
> 
> Rename `throwDone` and `throwValue`.

Done

>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:115
>> +            function(result) { promiseCapability.@resolve.@call(@undefined, { done, value: result }); },
> 
> Style: add space between function and (.

Done

>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:140
>> +    this.throw = @throw;
> 
> They should be defined in AsyncFromSyncIteratorPrototype. Not the member of the instance.
> Let's define AsyncFromSyncIteratorPrototype (maybe in C++). And set AsyncFromSyncIteratorConstructor.prototype to it.
> 
> See ArrayIterator case.
> 
> And you need to add @toStringTag with "Async-from-Sync Iterator".

Done

>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:37
>> +    return queue.@shift();
> 
> The problem is that @shift() is also observable.
> So, what do you think of constructing doubly-linked list here?

Is there way to use similar to @shift function without adding new structure? I would like to avoid adding new data structure there :(

>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:199
>> +                return @undefined;
> 
> Do we need throwawayCapability (with @promiseIsHandled = true)?

I thought that:
promiseCapability.@resolve.@call(@undefined, next.value); -  create promise with @promiseIsHandled = true
promiseCapability.@promise.@then - construct  throwawayCapability

[GSkachkov]

Created attachment 317970 [details]
Patch

New patch with fixes

[GSkachkov]

Created attachment 318160 [details]
Patch

Use queue instead of array

[GSkachkov]

Created attachment 318162 [details]
Patch

Use queue instead of array && remove unused code

[GSkachkov]

Created attachment 318272 [details]
Patch

Use queue instead of array && remove unused code && fix small issue in linked list

[Yusuke Suzuki]

Comment on attachment 318272 [details]
Patch

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

Nice. While several runtime fixes are required, other part seems fine.

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:26
> +@globalPrivate

Do not need this @globalPrivate.

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:31
> +    const object = this;

Just using |this| would be nice.

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:55
> +@globalPrivate

Ditto.

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:60
> +    const object = this;

Ditto.

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:106
> +@globalPrivate

Ditto.

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:29
> +    return new @AsyncGeneratorQueue();

I think returning { first: null, last: null } is better. The reason is described in the following comment.

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:39
> +    this.isEmpty = function ()

If user has Object.prototype.isEmpty setter, user can observe this and break things!

I think defining these async generator queue functions as @globalPrivate, and passing async generator queue like { first: null, last: null } is better.
Like,

@globalPrivate
function newAsyncGeneratorQueue()
{
    'use strict'

     return { first: null, last: null };
}

@globalPrivate
function asyncGeneratorIsEmpty(queue)
{
    'use strict';

    return queue.last === null;
}

@globalPrivate
function asyncGeneratorQueueCreateItem(value, previous)
{
    'use strict';

    return { value, previous, next: null };
}

@globalPrivate
function asyncGeneratorQueueEnqueue(queue, value)
{
    ...
}

etc.

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:78
> +function asyncGeneratorDenqueue(generator)

Typo: denqueue => dequeue

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:132
> +    const next = @asyncGeneratorDenqueue(generator);
> +
> +    const { promiseCapability } = next;

Let's merge them. `const { promiseCapability } = @asyncGeneratorDequeue(...)`

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:245
> +                promiseCapability.@promise.@then(function (result) { generator.@generatorState = @AsyncGeneratorStateCompleted; @asyncGeneratorResolve(generator, result, true); },

@then returns promise (throwawayCapability.promise). But it is not marked @promiseIsHandled.
We need to set @promiseIsHandled = true for this.

[GSkachkov]

Created attachment 318795 [details]
Patch

Upload fixed version

[GSkachkov]

Comment on attachment 318272 [details]
Patch

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

>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:26
>> +@globalPrivate
> 
> Do not need this @globalPrivate.

Removed

>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:31
>> +    const object = this;
> 
> Just using |this| would be nice.

Done

>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:55
>> +@globalPrivate
> 
> Ditto.

Removed

>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:60
>> +    const object = this;
> 
> Ditto.

Done

>> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:106
>> +@globalPrivate
> 
> Ditto.

Removed

>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:39
>> +    this.isEmpty = function ()
> 
> If user has Object.prototype.isEmpty setter, user can observe this and break things!
> 
> I think defining these async generator queue functions as @globalPrivate, and passing async generator queue like { first: null, last: null } is better.
> Like,
> 
> @globalPrivate
> function newAsyncGeneratorQueue()
> {
>     'use strict'
> 
>      return { first: null, last: null };
> }
> 
> @globalPrivate
> function asyncGeneratorIsEmpty(queue)
> {
>     'use strict';
> 
>     return queue.last === null;
> }
> 
> @globalPrivate
> function asyncGeneratorQueueCreateItem(value, previous)
> {
>     'use strict';
> 
>     return { value, previous, next: null };
> }
> 
> @globalPrivate
> function asyncGeneratorQueueEnqueue(queue, value)
> {
>     ...
> }
> 
> etc.

Implemented

>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:78
>> +function asyncGeneratorDenqueue(generator)
> 
> Typo: denqueue => dequeue

Fixed

>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:132
>> +    const { promiseCapability } = next;
> 
> Let's merge them. `const { promiseCapability } = @asyncGeneratorDequeue(...)`

Done

>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:245
>> +                promiseCapability.@promise.@then(function (result) { generator.@generatorState = @AsyncGeneratorStateCompleted; @asyncGeneratorResolve(generator, result, true); },
> 
> @then returns promise (throwawayCapability.promise). But it is not marked @promiseIsHandled.
> We need to set @promiseIsHandled = true for this.

Hopefully fixed, if I understand correctly.

[Build Bot]

Comment on attachment 318795 [details]
Patch

Attachment 318795 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4363862

New failing tests:
media/track/track-element-dom-change-crash.html

[Build Bot]

Created attachment 318817 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Yusuke Suzuki]

Comment on attachment 318795 [details]
Patch

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

I think we need to update test262 too.

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:72
> +    if (queue.first === null) return null;

Style: use webkit style here.

if (queue.first === null)
    return null;

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:77
> +    if (queue.first === null) queue.last = null;

Ditto.

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:263
> +            } else {

This else should be removed since the previous if-brace ends with `return`.

if (...) {
   ...
   return @undefined;
}
@assert(...);
return ...;

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2874
> +    auto varCreateAsyncFromSyncIterator = variable(propertyNames().builtinNames().createAsyncGeneratorQueuePrivateName());
> +    RefPtr<RegisterID> scope = newTemporary();
> +    RefPtr<RegisterID> queue = newTemporary();
> +    moveToDestinationIfNeeded(scope.get(), emitResolveScope(scope.get(), varCreateAsyncFromSyncIterator));
> +    RefPtr<RegisterID> createAsyncFromSyncIterator = emitGetFromScope(newTemporary(), scope.get(), varCreateAsyncFromSyncIterator, ThrowIfNotFound);
> +
> +    CallArguments args(*this, nullptr, 0);
> +    emitLoad(args.thisRegister(), jsUndefined());
> +
> +    emitCall(queue.get(), createAsyncFromSyncIterator.get(), NoExpectedFunction, args, divot, divot, divot, DebuggableCall::No);
> +

We should use createAsyncGeneratorQueue, correct?

> JSTests/stress/async-iteration-async-from-sync.js:2
> +//@ skip

Let's remove this @skip.

> JSTests/stress/async-iteration-basic.js:2
> +//@ skip

Let's remove this @skip.

> JSTests/stress/async-iteration-syntax.js:2
>  //@ skip

Let's remove this @skip. (And comment).

> JSTests/stress/async-iteration-yield-promise.js:2
> +//@ skip

Let's remove this @skip.

> JSTests/stress/async-iteration-yield-star-interface.js:2
> +// This test requires enableAsyncIterator to be enabled at run time.
> +//@ skip

Let's remove this @skip.

> JSTests/stress/async-iteration-yield-star.js:2
> +// This test requires enableAsyncIterator to be enabled at run time.
> +//@ skip

Let's remove this @skip.

[Yusuke Suzuki]

Note that we can further optimize this in a separate patch.

1. merging async generator queue to async generator itself
generator.@first / generator.@last is enough. by doing so, we can remove one unnecessary object alloc.

2. merging request with queue item
queueItem.value is also object. I think we can merge this two things. request should have next, and it should be queue item.

3. consider the case we call bunch of next() for async generator.
we should clear the queue once the generator is completed.

[GSkachkov]

Comment on attachment 318795 [details]
Patch

Patch is landed:
https://trac.webkit.org/r221080 
Clear flags

[GSkachkov]

*** Bug 166848 has been marked as a duplicate of this bug. ***

[Radar WebKit Bug Importer]

<rdar://problem/34038451>

[GSkachkov]

*** Bug 167727 has been marked as a duplicate of this bug. ***

[GSkachkov]

*** Bug 166857 has been marked as a duplicate of this bug. ***