Summary: | [ESNext] Async iteration - Implement Async Generator - runtime | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | GSkachkov <gskachkov> | ||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | GSkachkov <gskachkov> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | buildbot, saam, webkit-bug-importer, ysuzuki | ||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=175432 | ||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||
Bug Blocks: | 166695 | ||||||||||||||||||||||||
Attachments: |
|
Description
GSkachkov
2017-08-05 11:26:49 PDT
Created attachment 317346 [details]
Patch
Patch with runtime part with fixed comments
Created attachment 317362 [details]
Patch
Rebased patch
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? Created attachment 317429 [details]
Patch
Fix comments
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 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. Created attachment 317696 [details]
Patch
Rebased patch with using @skip
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 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 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 Created attachment 317970 [details]
Patch
New patch with fixes
Created attachment 318160 [details]
Patch
Use queue instead of array
Created attachment 318162 [details]
Patch
Use queue instead of array && remove unused code
Created attachment 318272 [details]
Patch
Use queue instead of array && remove unused code && fix small issue in linked list
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. Created attachment 318795 [details]
Patch
Upload fixed version
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 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 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 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. 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 on attachment 318795 [details] Patch Patch is landed: https://trac.webkit.org/r221080 Clear flags *** Bug 166848 has been marked as a duplicate of this bug. *** *** Bug 167727 has been marked as a duplicate of this bug. *** *** Bug 166857 has been marked as a duplicate of this bug. *** |