Bug 175891 – [ESNext] Async iteration - Implement Async Generator - optimization

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[GSkachkov]

Suggestion from Yusuke Suzuki:
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]

Created attachment 320460 [details]
Patch

Upload patch: implemented 1 and 2 point. Thinking about 3 one

[Yusuke Suzuki]

Comment on attachment 320460 [details]
Patch

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

I think the current one has a bug. But the direction looks very nice to me.

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:36
>  {

This function is unnecessary.

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:40
> +    value.next = null;
> +    value.previous = previous;

I think this is not safe. Users can trap this by setting Object.prototype.{next,previous} setters.
When writing builtins, you need to take care of property setters. Basically, creating a new property is always dangerous (except for @privateProperty).

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:46
> +function asyncGeneratorQueueEnqueue(generator, value)

Let's rename `value` to `item`.

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:51
> +        generator.@asyncGeneratorQueueFirst = @asyncGeneratorQueueCreateItem(value, null);

Change it to `generator.@asyncGeneratorQueueFirst = item;`. item.previous should be null when passing asyncGeneratorQueueEnqueue().
And let's insert `@assert(item.previous === null);` and `@assert(item.next === null);`.

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:54
> +        const item = @asyncGeneratorQueueCreateItem(value, generator.@asyncGeneratorQueueLast);

Change this to,

`item.previous = generator.@asyncGeneratorQueueLast;`.

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

Set `next` and `previous` fields with `null` for this `item`.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2889
> +    emitDirectPutById(m_generatorRegister, propertyNames().builtinNames().asyncGeneratorQueueLastPrivateName(), emitLoad(nullptr, jsNull()), PropertyNode::KnownDirect);

Nice.

[GSkachkov]

Comment on attachment 320460 [details]
Patch

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

>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:36
>>  {
> 
> This function is unnecessary.

Removed

>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:40
>> +    value.previous = previous;
> 
> I think this is not safe. Users can trap this by setting Object.prototype.{next,previous} setters.
> When writing builtins, you need to take care of property setters. Basically, creating a new property is always dangerous (except for @privateProperty).

replaced by asyncGeneratorQueueItemPrevious and asyncGeneratorQueueItemNext properties.

>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:46
>> +function asyncGeneratorQueueEnqueue(generator, value)
> 
> Let's rename `value` to `item`.

Done

>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:51
>> +        generator.@asyncGeneratorQueueFirst = @asyncGeneratorQueueCreateItem(value, null);
> 
> Change it to `generator.@asyncGeneratorQueueFirst = item;`. item.previous should be null when passing asyncGeneratorQueueEnqueue().
> And let's insert `@assert(item.previous === null);` and `@assert(item.next === null);`.

Done

>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:54
>> +        const item = @asyncGeneratorQueueCreateItem(value, generator.@asyncGeneratorQueueLast);
> 
> Change this to,
> 
> `item.previous = generator.@asyncGeneratorQueueLast;`.

Done

>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:275
>> +    @asyncGeneratorQueueEnqueue(generator, {resumeMode, value, promiseCapability});
> 
> Set `next` and `previous` fields with `null` for this `item`.

Done

[GSkachkov]

Created attachment 321031 [details]
Patch

Fix comments

[Yusuke Suzuki]

Comment on attachment 321031 [details]
Patch

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

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:41
> +    if (generator.@asyncGeneratorQueueFirst === null && generator.@asyncGeneratorQueueLast === null) {

Why do we need to check both? `generator.@asyncGeneratorQueueFirst === null` is enough, right?
And we can say @assert(generator.@asyncGeneratorQueueLast === null); in this brace, correct?

> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:43
> +        generator.@asyncGeneratorQueueLast = generator.@asyncGeneratorQueueFirst;

Make it `generator.@asyncGeneratorQueueLast = item;`

[GSkachkov]

Patch landed https://trac.webkit.org/r222425 
Close the bug

[Radar WebKit Bug Importer]

<rdar://problem/34694241>