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.
Created attachment 320460 [details] Patch Upload patch: implemented 1 and 2 point. Thinking about 3 one
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.
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
Created attachment 321031 [details] Patch Fix comments
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;`
Patch landed https://trac.webkit.org/r222425 Close the bug
<rdar://problem/34694241>