Bug 175240 - [ESNext] Async iteration - Implement Async Generator - runtime
Summary: [ESNext] Async iteration - Implement Async Generator - runtime
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: GSkachkov
URL:
Keywords: InRadar
: 166848 166857 167727 (view as bug list)
Depends on:
Blocks: 166695
  Show dependency treegraph
 
Reported: 2017-08-05 11:26 PDT by GSkachkov
Modified: 2017-08-23 10:48 PDT (History)
4 users (show)

See Also:


Attachments
Patch (240.36 KB, patch)
2017-08-05 13:24 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (241.57 KB, patch)
2017-08-06 07:33 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (262.24 KB, patch)
2017-08-07 10:10 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (263.33 KB, patch)
2017-08-09 05:50 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (253.78 KB, patch)
2017-08-11 15:30 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (256.03 KB, patch)
2017-08-15 13:26 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (255.90 KB, patch)
2017-08-15 13:36 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (255.96 KB, patch)
2017-08-16 10:49 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (257.54 KB, patch)
2017-08-22 14:01 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-elcapitan (1.93 MB, application/zip)
2017-08-22 16:00 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description GSkachkov 2017-08-05 11:26:49 PDT
Implement Async Generator - runtime part
Comment 1 GSkachkov 2017-08-05 13:24:22 PDT
Created attachment 317346 [details]
Patch

Patch with runtime part with fixed comments
Comment 2 GSkachkov 2017-08-06 07:33:22 PDT
Created attachment 317362 [details]
Patch

Rebased patch
Comment 3 Yusuke Suzuki 2017-08-06 12:41:55 PDT
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, generator.@asyncGeneratorQueue.length, {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?
Comment 4 GSkachkov 2017-08-07 10:10:45 PDT
Created attachment 317429 [details]
Patch

Fix comments
Comment 5 GSkachkov 2017-08-07 10:18:48 PDT
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, generator.@asyncGeneratorQueue.length, {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
Comment 6 Saam Barati 2017-08-07 18:52:02 PDT
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, generator.@asyncGeneratorQueue.length, {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.
Comment 7 GSkachkov 2017-08-09 05:50:50 PDT
Created attachment 317696 [details]
Patch

Rebased patch with using @skip
Comment 8 Yusuke Suzuki 2017-08-10 19:08:31 PDT
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?
Comment 9 Saam Barati 2017-08-11 08:32:04 PDT
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}
Comment 10 GSkachkov 2017-08-11 15:29:57 PDT
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
Comment 11 GSkachkov 2017-08-11 15:30:41 PDT
Created attachment 317970 [details]
Patch

New patch with fixes
Comment 12 GSkachkov 2017-08-15 13:26:49 PDT
Created attachment 318160 [details]
Patch

Use queue instead of array
Comment 13 GSkachkov 2017-08-15 13:36:50 PDT
Created attachment 318162 [details]
Patch

Use queue instead of array && remove unused code
Comment 14 GSkachkov 2017-08-16 10:49:32 PDT
Created attachment 318272 [details]
Patch

Use queue instead of array && remove unused code && fix small issue in linked list
Comment 15 Yusuke Suzuki 2017-08-22 06:51:08 PDT
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.
Comment 16 GSkachkov 2017-08-22 14:01:11 PDT
Created attachment 318795 [details]
Patch

Upload fixed version
Comment 17 GSkachkov 2017-08-22 14:04:29 PDT
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.
Comment 18 Build Bot 2017-08-22 16:00:26 PDT
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
Comment 19 Build Bot 2017-08-22 16:00:28 PDT
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
Comment 20 Yusuke Suzuki 2017-08-23 07:18:40 PDT
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.
Comment 21 Yusuke Suzuki 2017-08-23 10:33:23 PDT
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.
Comment 22 GSkachkov 2017-08-23 10:46:32 PDT
Comment on attachment 318795 [details]
Patch

Patch is landed:
https://trac.webkit.org/r221080 
Clear flags
Comment 23 GSkachkov 2017-08-23 10:47:12 PDT
*** Bug 166848 has been marked as a duplicate of this bug. ***
Comment 24 Radar WebKit Bug Importer 2017-08-23 10:47:44 PDT
<rdar://problem/34038451>
Comment 25 GSkachkov 2017-08-23 10:47:50 PDT
*** Bug 167727 has been marked as a duplicate of this bug. ***
Comment 26 GSkachkov 2017-08-23 10:48:47 PDT
*** Bug 166857 has been marked as a duplicate of this bug. ***