Created attachment 298142 [details]
Patch
WIP
Comment on attachment 298142 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298142&action=review Cool stuff so far. I'd like to look at this more on Friday. > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:39 > + @throwTypeError("Async generator is executing"); Async Generators differ from ordinary generators, in that it's not an error to resume the generator while it's executing. While it's an unlikely case, the generator is allowed to queue up additional requests while executing, and the queue of requests is drained during resumption (the proposal words this as recursive calls from AsyncGeneratorResolve / AsyncGeneratorReject -> AsyncGeneratorResumeNext) > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:65 > + if (generator.@asyncGeneratorState === @AsyncGeneratorStateAwait) { the special generator continuation is a nice idea. Though it doesn't affect JSC just yet due to not being implemented, note that this would _probably_ be observable via the function.sent metaproperty (or at least, it is in the v8 generator implementation without some additional work). Comment on attachment 298142 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298142&action=review >> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:39 >> + @throwTypeError("Async generator is executing"); > > Async Generators differ from ordinary generators, in that it's not an error to resume the generator while it's executing. > > While it's an unlikely case, the generator is allowed to queue up additional requests while executing, and the queue of requests is drained during resumption (the proposal words this as recursive calls from AsyncGeneratorResolve / AsyncGeneratorReject -> AsyncGeneratorResumeNext) Sure I'll remove this line. I've tried to replace queue by chain of promises to allow invoke 'next' many times as well. Also I have not implement&tests methods 'return' & 'throw', not sure that they work correct in this patch. >> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:65 >> + if (generator.@asyncGeneratorState === @AsyncGeneratorStateAwait) { > > the special generator continuation is a nice idea. > > Though it doesn't affect JSC just yet due to not being implemented, note that this would _probably_ be observable via the function.sent metaproperty (or at least, it is in the v8 generator implementation without some additional work). Oh Sorry, not sure that I got it. Do you mean that in case of using chain of promises instead of queue, we will have visible side effects when function.sent would be implemented? Could you please provide some examples that you faced in v8? I'll try to emulate on current solution, possible current approach is not viable at all. Also it seems that I need to replace this by: let wrappedValue = @newPromiseCapability(@Promise); wrappedValue.@resolve.@call(@undefined, value); wrappedValue.@promise.@then( function(result) { @asyncGeneratorResume(generator, promiseCapability, result, @GeneratorResumeModeNormal); }, function(error) { @asyncGeneratorResume(generator, promiseCapability, error, @GeneratorResumeModeThrow); }); Because value is returned by await can be non promise value. Comment on attachment 298142 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298142&action=review >>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:65 >>> + if (generator.@asyncGeneratorState === @AsyncGeneratorStateAwait) { >> >> the special generator continuation is a nice idea. >> >> Though it doesn't affect JSC just yet due to not being implemented, note that this would _probably_ be observable via the function.sent metaproperty (or at least, it is in the v8 generator implementation without some additional work). > > Oh Sorry, not sure that I got it. > Do you mean that in case of using chain of promises instead of queue, we will have visible side effects when function.sent would be implemented? > Could you please provide some examples that you faced in v8? I'll try to emulate on current solution, possible current approach is not viable at all. > > Also it seems that I need to replace this by: > > let wrappedValue = @newPromiseCapability(@Promise); > wrappedValue.@resolve.@call(@undefined, value); > wrappedValue.@promise.@then( > function(result) { @asyncGeneratorResume(generator, promiseCapability, result, @GeneratorResumeModeNormal); }, > function(error) { @asyncGeneratorResume(generator, promiseCapability, error, @GeneratorResumeModeThrow); }); > Because value is returned by await can be non promise value. What I was getting at was about the `function.sent` metaproperty proposal (https://github.com/allenwb/ESideas/blob/master/Generator%20metaproperty.md), which allows the programmer to make `function.sent` an alias to the value the generator was resumed with (eg `generator.next("foo");` makes function.sent === "foo" inside the generator after resuming). So, with Async Generators and await expressions, the awaited value would alias function.sent rather than the value sent via generator.next(), which observably breaks the metaproperty. JSC does not implement function.sent yet, so it isn't really noticeable here, but it is something to pay attention to for the future, unless the proposal is withdrawn Created attachment 298373 [details]
Patch
WiP with fixed comments
Comment on attachment 298142 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298142&action=review >>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:39 >>> + @throwTypeError("Async generator is executing"); >> >> Async Generators differ from ordinary generators, in that it's not an error to resume the generator while it's executing. >> >> While it's an unlikely case, the generator is allowed to queue up additional requests while executing, and the queue of requests is drained during resumption (the proposal words this as recursive calls from AsyncGeneratorResolve / AsyncGeneratorReject -> AsyncGeneratorResumeNext) > > Sure I'll remove this line. > I've tried to replace queue by chain of promises to allow invoke 'next' many times as well. Also I have not implement&tests methods 'return' & 'throw', not sure that they work correct in this patch. I read spec one more time and it seems that implementation of 'AsyncGenerator Abstract Operations' with AsyncGeneratorResolve / AsyncGeneratorReject -> AsyncGeneratorResumeNext will require some additional changes in 'generator' and 'async function', so I decided that it should be not be part of this patch, and be part of the separate task - https://bugs.webkit.org/show_bug.cgi?id=166848 >>>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:65 >>>> + if (generator.@asyncGeneratorState === @AsyncGeneratorStateAwait) { >>> >>> the special generator continuation is a nice idea. >>> >>> Though it doesn't affect JSC just yet due to not being implemented, note that this would _probably_ be observable via the function.sent metaproperty (or at least, it is in the v8 generator implementation without some additional work). >> >> Oh Sorry, not sure that I got it. >> Do you mean that in case of using chain of promises instead of queue, we will have visible side effects when function.sent would be implemented? >> Could you please provide some examples that you faced in v8? I'll try to emulate on current solution, possible current approach is not viable at all. >> >> Also it seems that I need to replace this by: >> >> let wrappedValue = @newPromiseCapability(@Promise); >> wrappedValue.@resolve.@call(@undefined, value); >> wrappedValue.@promise.@then( >> function(result) { @asyncGeneratorResume(generator, promiseCapability, result, @GeneratorResumeModeNormal); }, >> function(error) { @asyncGeneratorResume(generator, promiseCapability, error, @GeneratorResumeModeThrow); }); >> Because value is returned by await can be non promise value. > > What I was getting at was about the `function.sent` metaproperty proposal (https://github.com/allenwb/ESideas/blob/master/Generator%20metaproperty.md), which allows the programmer to make `function.sent` an alias to the value the generator was resumed with (eg `generator.next("foo");` makes function.sent === "foo" inside the generator after resuming). > > So, with Async Generators and await expressions, the awaited value would alias function.sent rather than the value sent via generator.next(), which observably breaks the metaproperty. > > JSC does not implement function.sent yet, so it isn't really noticeable here, but it is something to pay attention to for the future, unless the proposal is withdrawn OK. I'll keep in mind. Thanks for information. Comment on attachment 298373 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298373&action=review quick review. I'll review it in detail later. > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1179 > + dataLogF("Creating async function!\n"); Nit: `async generator function`. > Source/JavaScriptCore/parser/Parser.cpp:2740 > + bool isAsyncGeneratorMethod = false; Instead of adding more flags, cleaning up these flags is better. You can combine `isGenerator`, `isAsync`, `isAsyncGenerator`, `isAsyncMethod`, `isAsyncGeneratorMethod` etc. carefully to one enum, right? > Source/JavaScriptCore/parser/Parser.cpp:3808 > + bool isAsyncGenerator = false; > + bool isAsyncGeneratorMethod = false; Ditto > Source/JavaScriptCore/runtime/AsyncGeneratorFunctionConstructor.h:36 > + typedef InternalFunction Base; For the newer code, let's use `using Base = JSFunction;`. > Source/JavaScriptCore/runtime/AsyncGeneratorFunctionPrototype.h:33 > + typedef JSNonFinalObject Base; For the newer code, let's use `using Base = JSFunction;`. > Source/JavaScriptCore/runtime/AsyncGeneratorPrototype.h:36 > + typedef JSNonFinalObject Base; For the newer code, let's use `using Base = JSFunction;`. > Source/JavaScriptCore/runtime/AsyncIteratorPrototype.h:34 > + typedef JSNonFinalObject Base; For the newer code, let's use `using Base = JSFunction;`. > Source/JavaScriptCore/runtime/JSAsyncGeneratorFunction.h:36 > + typedef JSFunction Base; For the newer code, let's use `using Base = JSFunction;`. > Source/JavaScriptCore/runtime/JSFunction.cpp:356 > + // prototype = constructEmptyObject(exec, thisObject->globalObject(vm)->asyncGeneratorPrototype()); This line is not necessary. > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1223 > + visitor.append(thisObject->m_asyncGeneratorPrototype); m_asyncIteratorPrototype and m_asyncGeneratorFunctionPrototype need to be visited. Created attachment 298777 [details]
Patch
Fix comments
Created attachment 298852 [details]
Patch
Rebased version
Can you rebase and measure the parser performance by octane closure and jquery? I'll attempt to review it tomorrow in JST. Created attachment 300018 [details]
Patch
Rebased version
Created attachment 300020 [details]
Patch
Rebased version #2
Created attachment 300021 [details]
Patch
Rebased version & Fix builds
Comment on attachment 300021 [details] Patch Attachment 300021 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2963388 New failing tests: js/Object-getOwnPropertyNames.html Created attachment 300024 [details]
Archive of layout-test-results from ews101 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 300021 [details] Patch Attachment 300021 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2963391 New failing tests: js/Object-getOwnPropertyNames.html Created attachment 300025 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 300021 [details] Patch Attachment 300021 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2963392 New failing tests: js/Object-getOwnPropertyNames.html Created attachment 300026 [details]
Archive of layout-test-results from ews112 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 300021 [details] Patch Attachment 300021 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2963394 New failing tests: js/Object-getOwnPropertyNames.html Created attachment 300027 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 300030 [details]
Patch
Rebased version & Fix builds #2
Created attachment 300036 [details]
Patch
Rebased version & Fix builds & Fix tests
(In reply to comment #10) > Can you rebase and measure the parser performance by octane closure and > jquery? > I'll attempt to review it tomorrow in JST. I've uploaded rebased version of the patch for Async generator. It builds :-), but there are some already existed tests fail because async generator adds some new entities and allow some new syntax, but not sure that this is important for now, because as for me important to know if approach to implement async generator is correct. (In reply to comment #10) > Can you rebase and measure the parser performance by octane closure and > jquery? > I'll attempt to review it tomorrow in JST. Later I'll provide performance test results. Comment on attachment 300036 [details] Patch Attachment 300036 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2964519 New failing tests: js/Object-getOwnPropertyNames.html Created attachment 300041 [details]
Archive of layout-test-results from ews100 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 300036 [details] Patch Attachment 300036 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2964534 New failing tests: js/Object-getOwnPropertyNames.html Created attachment 300042 [details]
Archive of layout-test-results from ews117 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 300036 [details] Patch Attachment 300036 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2964531 New failing tests: js/Object-getOwnPropertyNames.html Created attachment 300043 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 300052 [details]
Patch
Rebased version & Fix linux builds & Fix tests
Created attachment 300053 [details]
Patch
Rebased version & Fix linux builds #2 & Fix tests
Octane test result:
Collected 4 samples per benchmark/VM, with 4 VM invocations per benchmark. Emitted a call to gc()
between sample measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the
jsc-specific preciseTime() function to get microsecond-level timing. Reporting benchmark execution
times with 95% confidence intervals in milliseconds.
master async_generator
encrypt 0.35928+-0.02440 ? 0.36720+-0.06654 ? might be 1.0220x slower
decrypt 5.98350+-0.60773 ? 6.25282+-0.72142 ? might be 1.0450x slower
deltablue x2 0.27937+-0.06044 0.25207+-0.04363 might be 1.1083x faster
earley 0.54239+-0.06300 0.51464+-0.06347 might be 1.0539x faster
boyer 8.77312+-0.97668 ? 8.99327+-1.49297 ? might be 1.0251x slower
navier-stokes x2 9.22747+-1.32705 ? 9.41712+-1.01753 ? might be 1.0206x slower
raytrace x2 1.67251+-0.10464 1.59693+-0.17772 might be 1.0473x faster
richards x2 0.17337+-0.02606 ? 0.18574+-0.01733 ? might be 1.0713x slower
splay x2 0.64561+-0.06174 ? 0.65691+-0.02068 ? might be 1.0175x slower
regexp x2 38.76431+-4.06309 37.42354+-2.68179 might be 1.0358x faster
pdfjs x2 89.22171+-5.54785 85.98402+-10.39049 might be 1.0377x faster
mandreel x2 117.15908+-23.67078 114.58115+-24.03146 might be 1.0225x faster
gbemu x2 84.56968+-15.85760 ? 97.95185+-29.86294 ? might be 1.1582x slower
closure 1.48272+-0.22510 1.35930+-0.11769 might be 1.0908x faster
jquery 16.93120+-2.10862 16.63362+-1.82305 might be 1.0179x faster
box2d x2 26.95917+-2.68609 26.92955+-2.37428
zlib x2 736.87150+-57.39998 704.60423+-43.28536 might be 1.0458x faster
typescript x2 1485.00702+-133.25049 1472.09821+-48.47523
<geometric> 11.44044+-0.19388 11.38264+-0.56308 might be 1.0051x faster
Comment on attachment 300053 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=300053&action=review Nice! Could you fix gtk/efl/win build failures? > Source/JavaScriptCore/ChangeLog:10 > + and async function. This will be fixed in https://bugs.webkit.org/show_bug.cgi?id=166848 Could you explain more detailed design in ChangeLog? For example, how to implement async iterator's await? What is the difference between async function and async generator? What is the difference between async generator and generator? etc. It helps so much when we extend / modify / debug the features. > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:30 > +// FIXME: Current implementation is not follow spec in part of > +// AsyncGenerator Abstract Operations > +// (https://tc39.github.io/proposal-async-iteration/#sec-asyncgenerator-abstract-operations), > +// to align implementation with spec created issue > +// https://bugs.webkit.org/show_bug.cgi?id=166848 What is the observable behavior? > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:54 > + > + promiseCapability.@resolve({ value, done }); Drop this (see the latter comment). > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:71 > + wrappedValue.@promise.@then( Is this `@then` use correct? > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:73 > + function(error) { @asyncGeneratorResume(generator, promiseCapability, error, @GeneratorResumeModeThrow); }); return here. > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:78 > + promiseCapability.@reject(error); return here. > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:79 > + } Move `promiseCapability.@resolve({ value, done });` here for both state case. > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:89 > + generator.@asyncGeneratorResumePromise.@then( Is this `@then` correct? > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:545 > + case SourceParseMode::AsyncGeneratorMethodMode: Let's name AsyncGeneratorWrapperMethodMode. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:546 > + case SourceParseMode::AsyncGeneratorFunctionMode: Let's name AsyncGeneratorWrapperFunctionMode. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3164 > + case SourceParseMode::AsyncGeneratorFunctionMode: Let's name AsyncGeneratorWrapperFunctionMode. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3165 > + case SourceParseMode::AsyncGeneratorMethodMode: Let's name AsyncGeneratorWrapperMethodMode. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3222 > + else if (function->parseMode() == SourceParseMode::AsyncGeneratorFunctionMode) Let's name AsyncGeneratorWrapperFunctionMode. > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3471 > + case SourceParseMode::AsyncGeneratorMethodMode: Let's name AsyncGeneratorWrapperMethodMode. > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3472 > + case SourceParseMode::AsyncGeneratorFunctionMode: { Let's name AsyncGeneratorWrapperFunctionMode. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6432 > + // TODO: Rename to JSAsyncGenerator We do not use `TODO:`. Use `FIXME:` instead. And if you use FIXME, the comment should have the bug URL. > Source/JavaScriptCore/parser/Parser.cpp:482 > +// TODO: Think about rename to parseAsyncFunctionOrAsyncGeneratorSourceElements We do not use TODO. Use `FIXME` instead. And if you use FIXME, the comment should have a bug URL. > Source/JavaScriptCore/parser/Parser.cpp:1988 > + case SourceParseMode::AsyncGeneratorFunctionMode: Let's name AsyncGeneratorWrapperFunctionMode. > Source/JavaScriptCore/parser/Parser.cpp:1991 > + case SourceParseMode::AsyncGeneratorMethodMode: Let's name AsyncGeneratorWrapperMethodMode. > Source/JavaScriptCore/parser/Parser.cpp:2270 > + semanticFail("Cannot declare async function named 'await'"); // TODO: Fix text message for async generators We do not use TODO. Use FIXME instead. And if you use FIXME, the comment should have a bug URL. > Source/JavaScriptCore/parser/Parser.cpp:2510 > + parseMode = SourceParseMode::AsyncGeneratorFunctionMode; Let's name AsyncGeneratorWrapperFunctionMode. > Source/JavaScriptCore/parser/Parser.cpp:2696 > + ident = m_token.m_data.ident; I do not think this ident is non null. You need to check it, right? If so, please add a test to cover this case since the following assert should fire for such a code right now. > Source/JavaScriptCore/parser/Parser.cpp:2705 > + ? SourceParseMode::AsyncGeneratorMethodMode Let's name AsyncGeneratorWrapperMethodMode. > Source/JavaScriptCore/parser/Parser.cpp:2751 > + if (isAsyncMethodParseMode(parseMode)) { > isConstructor = false; > - parseMode = SourceParseMode::AsyncMethodMode; > semanticFailIfTrue(*ident == m_vm->propertyNames->prototype, "Cannot declare an async method named 'prototype'"); > semanticFailIfTrue(*ident == m_vm->propertyNames->constructor, "Cannot declare an async method named 'constructor'"); > - } else if (isGenerator) { > + } else if (isGeneratorMethodParseMode(parseMode)) { > isConstructor = false; > - parseMode = SourceParseMode::GeneratorWrapperFunctionMode; > semanticFailIfTrue(*ident == m_vm->propertyNames->prototype, "Cannot declare a generator named 'prototype'"); > semanticFailIfTrue(*ident == m_vm->propertyNames->constructor, "Cannot declare a generator named 'constructor'"); > + } else if (isAsyncGeneratorMethodParseMode(parseMode)) { > + isConstructor = false; > + semanticFailIfTrue(*ident == m_vm->propertyNames->prototype, "Cannot declare a async generator named 'prototype'"); > + semanticFailIfTrue(*ident == m_vm->propertyNames->constructor, "Cannot declare a async generator named 'constructor'"); > } Can you clean up these if-else? Only the difference is the error message. You can unify these code and produce the appropriate error message based on the source parse mode. Maybe, stringForFunctionMode can be used. > Source/JavaScriptCore/parser/Parser.cpp:3745 > + isAsyncGenerator = true; If you found `async *`, it immediately means that this is async generator, correct? And you need to check whether the next token is identifier. And could you cover the above case in the test? > Source/JavaScriptCore/parser/Parser.cpp:3798 > + parseMode = SourceParseMode::AsyncGeneratorMethodMode; Let's name AsyncGeneratorWrapperMethodMode. > Source/JavaScriptCore/parser/Parser.cpp:3855 > + ASSERT(parseMode == SourceParseMode::MethodMode || parseMode == SourceParseMode::GeneratorWrapperFunctionMode || parseMode == SourceParseMode::AsyncMethodMode || parseMode == SourceParseMode::AsyncGeneratorMethodMode); Let's name AsyncGeneratorWrapperMethodMode. > Source/JavaScriptCore/parser/Parser.cpp:4164 > + parseMode = SourceParseMode::AsyncGeneratorFunctionMode; Let's name AsyncGeneratorWrapperFunctionMode. > Source/JavaScriptCore/parser/Parser.h:279 > + case SourceParseMode::AsyncGeneratorMethodMode: Let's name AsyncGeneratorWrapperMethodMode. > Source/JavaScriptCore/parser/Parser.h:280 > + case SourceParseMode::AsyncGeneratorFunctionMode: Let's name AsyncGeneratorWrapperFunctionMode. > Source/JavaScriptCore/parser/ParserModes.h:62 > + AsyncGeneratorFunctionMode = 0b00000000000000010000000000000000, Let's name AsyncGeneratorWrapperFunctionMode. > Source/JavaScriptCore/parser/ParserModes.h:63 > + AsyncGeneratorMethodMode = 0b00000000000000100000000000000000, Let's name AsyncGeneratorWrapperMethodMode. > Source/JavaScriptCore/parser/ParserModes.h:110 > + SourceParseMode::AsyncGeneratorFunctionMode, Let's name AsyncGeneratorWrapperFunctionMode. > Source/JavaScriptCore/parser/ParserModes.h:111 > + SourceParseMode::AsyncGeneratorMethodMode).contains(parseMode); Let's name AsyncGeneratorWrappeMethodMode. > Source/JavaScriptCore/parser/ParserModes.h:117 > + SourceParseMode::AsyncGeneratorFunctionMode, Let's name AsyncGeneratorWrapperFunctionMode. > Source/JavaScriptCore/parser/ParserModes.h:119 > + SourceParseMode::AsyncGeneratorMethodMode, Let's name AsyncGeneratorWrapperMethodMode. > Source/JavaScriptCore/parser/ParserModes.h:137 > + SourceParseMode::AsyncGeneratorFunctionMode, Let's name AsyncGeneratorWrapperFunctionMode. > Source/JavaScriptCore/parser/ParserModes.h:138 > + SourceParseMode::AsyncGeneratorMethodMode).contains(parseMode); Let's name AsyncGeneratorWrapperMethodMode. > Source/JavaScriptCore/parser/ParserModes.h:146 > + SourceParseMode::AsyncGeneratorFunctionMode, Let's name AsyncGeneratorWrapperFunctionMode. > Source/JavaScriptCore/parser/ParserModes.h:147 > + SourceParseMode::AsyncGeneratorMethodMode, Let's name AsyncGeneratorWrapperMethodMode. > Source/JavaScriptCore/parser/ParserModes.h:170 > + // FIXME: GeneratorWrapperFunctionMode is not guaranteed to be a method. Can you explain this? It seems wrong. If you need GeneratorWrapperMethod type, I think implementing it in the separate patch is better. (It should be very small I guess). After that patch is landed, we can use it in this patch. > Source/JavaScriptCore/parser/ParserModes.h:181 > + return SourceParseModeSet(SourceParseMode::AsyncGeneratorFunctionMode).contains(parseMode); Let's name AsyncGeneratorWrapperFunctionMode. > Source/JavaScriptCore/parser/ParserModes.h:193 > + SourceParseMode::AsyncGeneratorMethodMode).contains(parseMode); Let's name AsyncGeneratorWrapperMethodMode. > Source/JavaScriptCore/parser/ParserModes.h:211 > + SourceParseMode::AsyncGeneratorFunctionMode, Let's name AsyncGeneratorWrapperFunctionMode. > Source/JavaScriptCore/parser/ParserModes.h:213 > + SourceParseMode::AsyncGeneratorMethodMode).contains(parseMode); Let's name AsyncGeneratorWrapperMethodMode. > Source/JavaScriptCore/runtime/AsyncGeneratorFunctionConstructor.cpp:49 > + // Number of arguments for constructor This comment is unnecessary. > Source/JavaScriptCore/runtime/AsyncGeneratorFunctionConstructor.cpp:56 > + // TODO: Use Async Generator instead We do not use TODO. Use FIXME instead. And if you use FIXME, the comment should have a bug URL. > Source/JavaScriptCore/runtime/AsyncGeneratorPrototype.h:37 > + static const unsigned StructureFlags = Base::StructureFlags | HasStaticPropertyTable; We do not have static property table for this. Drop HasStaticPropertyTable. > Source/JavaScriptCore/runtime/FunctionExecutable.h:126 > + bool isAsyncGenerator() const { return SourceParseModeSet(SourceParseMode::AsyncGeneratorFunctionMode, SourceParseMode::AsyncGeneratorBodyMode).contains(parseMode()); } Let's name AsyncGeneratorWrapperFunctionMode. > Source/JavaScriptCore/runtime/FunctionExecutable.h:141 > + SourceParseMode::AsyncGeneratorFunctionMode, Let's name AsyncGeneratorWrapperFunctionMode. > Source/JavaScriptCore/runtime/JSFunction.cpp:355 > + } else if (thisObject->jsExecutable()->parseMode() == SourceParseMode::AsyncGeneratorFunctionMode) Let's name AsyncGeneratorWrapperFunctionMode. Created attachment 300439 [details]
Patch
Fix comments
Created attachment 300449 [details]
Patch
Fix comments & Fix gtk build
Created attachment 300609 [details]
Patch
Fix gtk build
Comment on attachment 300609 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=300609&action=review I've left a few runtime comments, but I do understand that this patch does not claim to implement the runtime perfectly. > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:90 > + generator.@asyncGeneratorResumePromise.@then( I believe it's observable to implement the queue this way --- AsyncGeneratorResolve and AsyncGeneratorReject immediately begin resuming subsequent requests if the queue is not empty, but this waits until a later turn. I think this could cause weird races between implementations which match the spec and implementations which do this. Maybe Domenic can indicate if this is the intent or not > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:686 > + bool needTryCatch = isAsyncFunctionOrAsyncGeneratorWrapperParseMode(parseMode) && !isSimpleParameterList; async generators don't need a try/catch for the parameter list, they just throw synchronously if parameters cause problems (https://tc39.github.io/proposal-async-iteration/#sec-asyncgenerator-definitions-evaluatebody), and do not return a Promise > Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:142 > + enum class Kind { Escaped, Object, Activation, Function, GeneratorFunction, AsyncFunction, AsyncGeneratorFunction }; I think I would personally prefer it if DFG/FTL passes were untouched in the initial patch, but I suppose that will be left up to reviewers. > Source/JavaScriptCore/parser/Parser.h:261 > + setIsGenerator(); This stuff is getting very complicated, I feel there has to be a better way to track function scope types. I feel this is very error prone. Created attachment 305372 [details]
Patch
WIP
Comment on attachment 305372 [details] Patch Attachment 305372 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/3408447 New failing tests: stress/Object_static_methods_Object.getOwnPropertyDescriptors.js.no-cjit-collect-continuously stress/generator-class-methods-syntax.js.ftl-no-cjit-no-put-stack-validate stress/Object_static_methods_Object.getOwnPropertyDescriptors.js.ftl-no-cjit-no-put-stack-validate stress/Object_static_methods_Object.getOwnPropertyDescriptors.js.no-llint stress/generator-class-methods-syntax.js.ftl-eager-no-cjit stress/generator-class-methods-syntax.js.ftl-no-cjit-no-inline-validate stress/Object_static_methods_Object.getOwnPropertyDescriptors.js.ftl-no-cjit-no-inline-validate stress/Object_static_methods_Object.getOwnPropertyDescriptors.js.dfg-maximal-flush-validate-no-cjit stress/generator-class-methods-syntax.js.dfg-maximal-flush-validate-no-cjit stress/generator-class-methods-syntax.js.default stress/Object_static_methods_Object.getOwnPropertyDescriptors.js.default stress/Object_static_methods_Object.getOwnPropertyDescriptors.js.ftl-eager-no-cjit stress/Object_static_methods_Object.getOwnPropertyDescriptors.js.ftl-no-cjit-validate-sampling-profiler stress/generator-class-methods-syntax.js.no-cjit-collect-continuously stress/Object_static_methods_Object.getOwnPropertyDescriptors.js.dfg-eager-no-cjit-validate stress/generator-class-methods-syntax.js.dfg-eager-no-cjit-validate stress/Object_static_methods_Object.getOwnPropertyDescriptors.js.dfg-eager stress/Object_static_methods_Object.getOwnPropertyDescriptors.js.ftl-eager stress/generator-class-methods-syntax.js.no-ftl stress/generator-class-methods-syntax.js.ftl-no-cjit-validate-sampling-profiler stress/Object_static_methods_Object.getOwnPropertyDescriptors.js.ftl-no-cjit-small-pool stress/generator-class-methods-syntax.js.no-llint stress/generator-class-methods-syntax.js.no-cjit-validate-phases stress/Object_static_methods_Object.getOwnPropertyDescriptors.js.no-ftl stress/Object_static_methods_Object.getOwnPropertyDescriptors.js.no-cjit-validate-phases stress/generator-class-methods-syntax.js.ftl-no-cjit-small-pool stress/generator-class-methods-syntax.js.dfg-eager stress/generator-class-methods-syntax.js.ftl-eager Created attachment 305401 [details]
Patch
WIP with fixed tests
Created attachment 305431 [details]
Patch
Fixed passed arguments in yield* and more tests are coming
Comment on attachment 305431 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305431&action=review > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:77 I think it might be wise to make AsyncGeneratorResumeNext() behave as a loop, so that this recursion can be avoided when there are multiple yields and multiple requests in the queue, unless the spec is changed to get rid of the "evaluate the next item in queue after a yield immediately". I'm sure I've said this in this bug before, but I see that we're still doing this recursively, and without PTC (though I'm not sure if PTC would help much in this case). Comment on attachment 305431 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305431&action=review >> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:77 >> + @asyncGeneratorResumeNext(generator); > > I think it might be wise to make AsyncGeneratorResumeNext() behave as a loop, so that this recursion can be avoided when there are multiple yields and multiple requests in the queue, unless the spec is changed to get rid of the "evaluate the next item in queue after a yield immediately". > > I'm sure I've said this in this bug before, but I see that we're still doing this recursively, and without PTC (though I'm not sure if PTC would help much in this case). Thanks for quick feedback! Yeah, I remember your comment https://bugs.webkit.org/show_bug.cgi?id=166695#c39, however I've tried to be close to spec as much as I can, so I implemented (p 6.4.3.3.11). But you are right, using loop is better in this place, so I'll try to switch to approach with loop. Created attachment 305496 [details]
Patch
More tests and fix several review comments
Comment on attachment 305431 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305431&action=review >>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:77 >>> + @asyncGeneratorResumeNext(generator); >> >> I think it might be wise to make AsyncGeneratorResumeNext() behave as a loop, so that this recursion can be avoided when there are multiple yields and multiple requests in the queue, unless the spec is changed to get rid of the "evaluate the next item in queue after a yield immediately". >> >> I'm sure I've said this in this bug before, but I see that we're still doing this recursively, and without PTC (though I'm not sure if PTC would help much in this case). > > Thanks for quick feedback! > > Yeah, I remember your comment https://bugs.webkit.org/show_bug.cgi?id=166695#c39, however I've tried to be close to spec as much as I can, so I implemented (p 6.4.3.3.11). > But you are right, using loop is better in this place, so I'll try to switch to approach with loop. I added loop into AsyncGeneratorResumeNext in new patch, hope it decrees recursion level. Could you please take a look if it make sense? Comment on attachment 305496 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305496&action=review Looks good overall, just a couple comments. Obviously, it would be great if Sam or one of the other JSC core folks could give their opinion on it, since I can't sign off on WebKit patches. > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:57 > + @asyncGeneratorResumeNext(generator); Parameterizing the recursion seems weird to me still. I've suggested some changes that make this unnecessary > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:80 > + @asyncGeneratorResumeNext(generator); ditto here > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:114 > + const restartResumeNext = true; There's no need to recurse here, really. You can just call AsyncGeneratorResumeNext at the end of the method instead (and, you could even PTC to it in that case, since there's no generator on the stack at this point) > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:125 > + const restartResumeNext = true; ditto here Created attachment 305500 [details]
Patch
Remove AsyncGeneratorResumeNext from Resolve/Reject methods
Comment on attachment 305496 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305496&action=review >> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:114 >> + const restartResumeNext = true; > > There's no need to recurse here, really. You can just call AsyncGeneratorResumeNext at the end of the method instead (and, you could even PTC to it in that case, since there's no generator on the stack at this point) I fixed in new patch, I hope it is what you mean. Comment on attachment 305500 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305500&action=review > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:117 > + } else { So the last thing is these handlers. You could simplify this: ``` wrappedValue.@promise.@then( function(result) { const isDelegetedYield = generator.@asyncGeneratorSuspendReason === @AsyncGeneratorSuspendReasonDelegatedYield; generator.@asyncGeneratorSuspendReason = @AsyncGeneratorSuspendReasonNone; if (isDelegetedYield) { const isDone = false; @asyncGeneratorResolve(generator, result.value, isDone); } return @asyncGeneratorResumeNext(generator); }, function(error) { generator.@generatorState = @AsyncGeneratorStateCompleted; // see note below @asyncGeneratorReject(generator, error); @asyncGeneratorResumeNext(generator); } ``` > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:123 > + generator.@generatorState = @AsyncGeneratorStateCompleted; This actually looks wrong to me, per the current spec. Unless the committee decides to go with the "yielded rejections don't affect control flow of the generator" thing (which is admittedly pretty popular), closing the generator should be left up to @asyncGeneratorResume(). Even if the proposal is changed, this will prevent finally blocks from being reached. E.g. ```js let reject; async function* foo() { try { yield new Promise(function(unused, rej) { reject = rej; }); } finally { doImportantCleanupStuff(); // In this patch, this appears to be unreachable return 0; } } foo().next().then(x => print(x), e => print(e)); reject("Oop!"); ``` I could be wrong about this, but I _believe_ this is broken in this patch due to the generator state change here, and I think it is supposed to work in the current spec Comment on attachment 305500 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305500&action=review >> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:117 >> + } else { > > So the last thing is these handlers. > > You could simplify this: > > ``` > wrappedValue.@promise.@then( > function(result) { > const isDelegetedYield = generator.@asyncGeneratorSuspendReason === @AsyncGeneratorSuspendReasonDelegatedYield; > generator.@asyncGeneratorSuspendReason = @AsyncGeneratorSuspendReasonNone; > if (isDelegetedYield) { > const isDone = false; > @asyncGeneratorResolve(generator, result.value, isDone); > } > return @asyncGeneratorResumeNext(generator); > }, > function(error) { > generator.@generatorState = @AsyncGeneratorStateCompleted; // see note below > @asyncGeneratorReject(generator, error); > @asyncGeneratorResumeNext(generator); > } > ``` Yeah, it seems my patch is not clear enough, because in this asyncGeneratorResume function in resolve callback for wrappedValue, is handled case when we suspend execution control flow because of await or yield*: AsyncGeneratorSuspendReasonAwait/AsyncGeneratorSuspendReasonDelegatedYield For suspended await we need run current function again because because we need to continue execution of function until faced with 'yield' or new await. For value from async generator function that invoked by yield* we need to unwrap value, because it returns promise. >> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:123 >> + generator.@generatorState = @AsyncGeneratorStateCompleted; > > This actually looks wrong to me, per the current spec. Unless the committee decides to go with the "yielded rejections don't affect control flow of the generator" thing (which is admittedly pretty popular), closing the generator should be left up to @asyncGeneratorResume(). > > Even if the proposal is changed, this will prevent finally blocks from being reached. > > E.g. > > ```js > let reject; > async function* foo() { > try { > yield new Promise(function(unused, rej) { reject = rej; }); > } finally { > doImportantCleanupStuff(); // In this patch, this appears to be unreachable > return 0; > } > } > > foo().next().then(x => print(x), e => print(e)); > reject("Oop!"); > ``` > > I could be wrong about this, but I _believe_ this is broken in this patch due to the generator state change here, and I think it is supposed to work in the current spec I see following result: do important staff returned value Oop! Is this result close to spec or 'important staff' should be returned after Oop! To test I used little bit modified function: ``` async function* foo() { try { yield new Promise(function(unused, rej) { reject = rej; }); } finally { print('do important staff'); return 'returned value'; } } var f= foo() f.next().then(({value}) => print(value), e => print(e)); reject("Oop!"); f.next().then(({value}) => print(value), e => print(e)); drainMicrotasks(); ``` Comment on attachment 305500 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305500&action=review >>> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:123 >>> + generator.@generatorState = @AsyncGeneratorStateCompleted; >> >> This actually looks wrong to me, per the current spec. Unless the committee decides to go with the "yielded rejections don't affect control flow of the generator" thing (which is admittedly pretty popular), closing the generator should be left up to @asyncGeneratorResume(). >> >> Even if the proposal is changed, this will prevent finally blocks from being reached. >> >> E.g. >> >> ```js >> let reject; >> async function* foo() { >> try { >> yield new Promise(function(unused, rej) { reject = rej; }); >> } finally { >> doImportantCleanupStuff(); // In this patch, this appears to be unreachable >> return 0; >> } >> } >> >> foo().next().then(x => print(x), e => print(e)); >> reject("Oop!"); >> ``` >> >> I could be wrong about this, but I _believe_ this is broken in this patch due to the generator state change here, and I think it is supposed to work in the current spec > > I see following result: > > do important staff > returned value > Oop! > > Is this result close to spec or 'important staff' should be returned after Oop! > To test I used little bit modified function: > ``` > async function* foo() { > try { > yield new Promise(function(unused, rej) { reject = rej; }); > } finally { > print('do important staff'); > return 'returned value'; > } > } > > var f= foo() > f.next().then(({value}) => print(value), e => print(e)); > reject("Oop!"); > f.next().then(({value}) => print(value), e => print(e)); > drainMicrotasks(); > ``` In #jslang I made a gist and spent some time analyzing it, didn't get through the whole thing, but I pretty much get the same result, so I think it's okay. https://gist.github.com/caitp/9af4de75c5ac6ff7d22b366472617e21 And yeah, the finally block would be reached if another request is queued up, regardless of whether the rejection affects control flow or not. But it still shouldn't close the generator, that doesn't really make sense. Created attachment 306847 [details]
Patch
WiP with Async-from-sync prototype & and small fix & and stil need more tests
Created attachment 307013 [details]
Patch
Added more tests & small fixes & implemented AsyncIterationClose function
Comment on attachment 307013 [details] Patch Attachment 307013 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3530553 New failing tests: webrtc/multi-video.html Created attachment 307034 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 307013 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307013&action=review Wowza, this is a huge patch. I need to review the spec before I can responsibly review this. I'll get back to this tomorrow. Maybe Yusuke/Caitlin can also take a look, since they implemented generators/async functions. > Source/JavaScriptCore/ChangeLog:8 > + Current implementation is draft version of Async Iteration. Can you link to the spec here? > Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:153 > + function(result) { promiseCapability.@resolve.@call(@undefined, { value: result, done }); }, > + function(error) { promiseCapability.@reject.@call(@undefined, error); }); style: indentation should be 4 lines from the statement's starting indentation Comment on attachment 307013 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307013&action=review I haven't gone over the whole patch yet. So, I'm a bit confused about what is gained from having a new "suspend reason" for yield*. Is the goal here to shrink the generated bytecode by performing the Await logic in the runtime function? I've checked for compliance with the couple of spec changes that have landed upstream since I first implemented this in v8, and left a few pointers that might be helpful for improving compliance. > Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:62 > + const { value, done } = nextResult; Per https://github.com/tc39/proposal-async-iteration/commit/395b2e3b2f5acb62f9fae11c5e189423d4af50e6, you need to load `done` before `value` in all of these methods. This is intended to match what yield* does in async generators, and there are tests verifying this in test262. > Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:104 > + const { value, done } = returnResult; ditto here > Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:147 > + const { value, done } = throwResult; ditto here > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4777 > + const Identifier& propertyName = parseMode() == SourceParseMode::AsyncGeneratorBodyMode ? propertyNames().asyncIteratorSymbol : propertyNames().iteratorSymbol; I might suggest putting this logic into an `emitGetIterator(Register iterable)` helper to make it easier to maintain and understand > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:-4783 > - nit: removing this whitespace is noise in the diff > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4881 > + emitYieldPoint(value.get(), JSAsyncGeneratorFunction::AsyncGeneratorSuspendReason::Await); For readability, how about something like `Register emitAwait(dstRegister, promiseRegister)`? This pattern is repeating a lot and it's more to think about than is desirable maybe > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4906 > emitIteratorNextWithValue(value.get(), iterator.get(), value.get(), node); I think the correct thing is: ``` // ... emitLabel(nextElement.get()); emitIteratorNextWithValue(value.get(), iterator.get(), value.get(), node); if (parseMode() == SourceParseMode::AsyncGeneratorBodyMode) { // If generatorKind is async, then set innerResult to ? Await(innerResult). RefPtr<RegisterID> result = emitYield(value.get(), JSAsyncGeneratorFunction::AsyncGeneratorSuspendReason::Await); emitMove(value.get(), result.get()); } emitJumpIfTrue(emitGetById(newTemporary(), value.get(), propertyNames().done), loopDone.get()); emitGetById(value.get(), value.get(), propertyNames().value); emitJump(loopStart.get(); // ... ``` I don't think the generator state check or the other stuff is needed for async generators. I believe you're trying to handle the Awaits inside @asyncGeneratorResumeNext(), but AFAICT this separates the logic from the control flow here, which likely will cause bugs. Instead, I think you should just add Awaits in the places the spec requires you to > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4912 > + emitEqualityOp(op_stricteq, condition.get(), state.get(), emitLoad(nullptr, jsNumber(static_cast<int32_t>(JSGeneratorFunction::GeneratorState::Completed)))); Is this logic not handled by AsyncGeneratorResumeNext()? > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4914 > + emitJumpIfTrue(emitGetById(newTemporary(), value.get(), propertyNames().done), loopDone.get()); This looks like it should be loading the "value" property, not "done" > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4930 > + Nit: unnecessary linefeed > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3494 > + nit: trailing whitespace sneaking in > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3497 > + nit: trailing whitespace sneaking in > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3502 > + here too > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3507 > + and here > Source/JavaScriptCore/dfg/DFGNode.h:594 > + ASSERT(m_op == NewFunction || m_op == NewGeneratorFunction || m_op == NewAsyncFunction || m_op == NewAsyncGeneratorFunction); I still would prefer if changes to DFG/FTL could be deferred to a later bug, in order to keep the initial implementation as simple as possible. > Source/JavaScriptCore/parser/ParserModes.h:61 > + AsyncGeneratorBodyMode = 0b00000000000000001000000000000000, I would suggest making AsyncGenerator<*>Mode == Generator<*>Mode | AsyncFunction<*>Mode, in order to make it easier for the parser (eg, easier to determine that `yield` or `await` is legal). But, if it's already done this way without hurting performance, I suppose it's fine. > Source/JavaScriptCore/runtime/AsyncGeneratorFunctionConstructor.cpp:37 > +const ClassInfo AsyncGeneratorFunctionConstructor::s_info = { "AsyncGenerator", &Base::s_info, nullptr, CREATE_METHOD_TABLE(AsyncGeneratorFunctionConstructor) }; className should be "AsyncGeneratorFunction", similar to the className for GeneratorFunctionConstructor > Source/JavaScriptCore/runtime/FunctionConstructor.cpp:114 > + prefix = "{async function *"; I don't see a change to functionProtoFuncToString() in FunctionPrototype.cpp, but just FYI this formatting contradicts the test262 tests. We seem to have decided on "async function*" (https://github.com/tc39/test262/commit/da764cafa28ea15b194ac545dc1b67c707c62296), though the proposal doesn't really get into it IIRC. This comment is just about "you should add the change to functionProtoFuncToString(), with no space between `function` and `*`" :caitp Thanks for review! I'll try to fix your comments during this week. Created attachment 311243 [details]
Patch
Fix comments except one, and rebase
Comment on attachment 307013 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307013&action=review >> Source/JavaScriptCore/ChangeLog:8 >> + Current implementation is draft version of Async Iteration. > > Can you link to the spec here? Done >> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:62 >> + const { value, done } = nextResult; > > Per https://github.com/tc39/proposal-async-iteration/commit/395b2e3b2f5acb62f9fae11c5e189423d4af50e6, you need to load `done` before `value` in all of these methods. This is intended to match what yield* does in async generators, and there are tests verifying this in test262. I've changed to const { done, value } = nextResult; I hope it Is this correct >> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:104 >> + const { value, done } = returnResult; > > ditto here Done >> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:147 >> + const { value, done } = throwResult; > > ditto here Done >> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:153 >> + function(error) { promiseCapability.@reject.@call(@undefined, error); }); > > style: indentation should be 4 lines from the statement's starting indentation Done >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4777 >> + const Identifier& propertyName = parseMode() == SourceParseMode::AsyncGeneratorBodyMode ? propertyNames().asyncIteratorSymbol : propertyNames().iteratorSymbol; > > I might suggest putting this logic into an `emitGetIterator(Register iterable)` helper to make it easier to maintain and understand Done >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:-4783 >> - > > nit: removing this whitespace is noise in the diff Done >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4881 >> + emitYieldPoint(value.get(), JSAsyncGeneratorFunction::AsyncGeneratorSuspendReason::Await); > > For readability, how about something like `Register emitAwait(dstRegister, promiseRegister)`? This pattern is repeating a lot and it's more to think about than is desirable maybe Done >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4906 >> emitIteratorNextWithValue(value.get(), iterator.get(), value.get(), node); > > I think the correct thing is: > > ``` > // ... > emitLabel(nextElement.get()); > emitIteratorNextWithValue(value.get(), iterator.get(), value.get(), node); > > if (parseMode() == SourceParseMode::AsyncGeneratorBodyMode) { > // If generatorKind is async, then set innerResult to ? Await(innerResult). > RefPtr<RegisterID> result = emitYield(value.get(), JSAsyncGeneratorFunction::AsyncGeneratorSuspendReason::Await); > emitMove(value.get(), result.get()); > } > > emitJumpIfTrue(emitGetById(newTemporary(), value.get(), propertyNames().done), loopDone.get()); > emitGetById(value.get(), value.get(), propertyNames().value); > emitJump(loopStart.get(); > // ... > ``` > > I don't think the generator state check or the other stuff is needed for async generators. I believe you're trying to handle the Awaits inside @asyncGeneratorResumeNext(), but AFAICT this separates the logic from the control flow here, which likely will cause bugs. > > Instead, I think you should just add Awaits in the places the spec requires you to I see, I'll try to do this.. >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4912 >> + emitEqualityOp(op_stricteq, condition.get(), state.get(), emitLoad(nullptr, jsNumber(static_cast<int32_t>(JSGeneratorFunction::GeneratorState::Completed)))); > > Is this logic not handled by AsyncGeneratorResumeNext()? Not sure that I get, what you mean. yield * is handled by this part of code, in asyncGeneratorResume that called from asyncGeneratorResumeNext we set AsyncGeneratorState::Completed that we is checked there. Also I see that I'm using wrong enum name, GeneratorState instead of AsyncGeneratorState >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4914 >> + emitJumpIfTrue(emitGetById(newTemporary(), value.get(), propertyNames().done), loopDone.get()); > > This looks like it should be loading the "value" property, not "done" Nope. In current patch it leads to error. We need to check if Iterator that is invoked by yield * is finished, so we are checking done and if it is finished we go to loopDone, later we load value from value register (line 4842:4929 - emitGetById(value.get(), value.get(), propertyNames().value);) >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4930 >> + > > Nit: unnecessary linefeed Done >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3494 >> + > > nit: trailing whitespace sneaking in Done >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3497 >> + > > nit: trailing whitespace sneaking in Done >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3502 >> + > > here too Done >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3507 >> + > > and here Done >> Source/JavaScriptCore/dfg/DFGNode.h:594 >> + ASSERT(m_op == NewFunction || m_op == NewGeneratorFunction || m_op == NewAsyncFunction || m_op == NewAsyncGeneratorFunction); > > I still would prefer if changes to DFG/FTL could be deferred to a later bug, in order to keep the initial implementation as simple as possible. Ok. Not sure that I know how to avoid use DFG/B3 part with passing stress tests. >> Source/JavaScriptCore/runtime/AsyncGeneratorFunctionConstructor.cpp:37 >> +const ClassInfo AsyncGeneratorFunctionConstructor::s_info = { "AsyncGenerator", &Base::s_info, nullptr, CREATE_METHOD_TABLE(AsyncGeneratorFunctionConstructor) }; > > className should be "AsyncGeneratorFunction", similar to the className for GeneratorFunctionConstructor Done >> Source/JavaScriptCore/runtime/FunctionConstructor.cpp:114 >> + prefix = "{async function *"; > > I don't see a change to functionProtoFuncToString() in FunctionPrototype.cpp, but just FYI this formatting contradicts the test262 tests. We seem to have decided on "async function*" (https://github.com/tc39/test262/commit/da764cafa28ea15b194ac545dc1b67c707c62296), though the proposal doesn't really get into it IIRC. > > This comment is just about "you should add the change to functionProtoFuncToString(), with no space between `function` and `*`" Done Comment on attachment 311243 [details] Patch Attachment 311243 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/3815336 New failing tests: jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-no-cjit stress/generator-class-methods-syntax.js.ftl-no-cjit-no-put-stack-validate stress/generator-class-methods-syntax.js.ftl-no-cjit-b3o1 stress/generator-class-methods-syntax.js.ftl-eager-no-cjit-b3o1 jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-ftl-no-cjit stress/generator-class-methods-syntax.js.ftl-eager-no-cjit stress/generator-class-methods-syntax.js.ftl-no-cjit-no-inline-validate stress/generator-class-methods-syntax.js.dfg-maximal-flush-validate-no-cjit stress/generator-class-methods-syntax.js.default stress/generator-class-methods-syntax.js.no-cjit-collect-continuously jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-no-ftl stress/generator-class-methods-syntax.js.dfg-eager-no-cjit-validate jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-ftl-eager-no-cjit jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-no-llint jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-dfg-eager-no-cjit jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout stress/generator-class-methods-syntax.js.no-ftl stress/generator-class-methods-syntax.js.ftl-no-cjit-validate-sampling-profiler stress/generator-class-methods-syntax.js.no-llint stress/generator-class-methods-syntax.js.no-cjit-validate-phases stress/generator-class-methods-syntax.js.ftl-no-cjit-small-pool stress/generator-class-methods-syntax.js.dfg-eager stress/generator-class-methods-syntax.js.ftl-eager apiTests Comment on attachment 311243 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311243&action=review Other than that, I'm pretty happy with it > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4969 > + emitEqualityOp(op_stricteq, condition.get(), state.get(), emitLoad(nullptr, jsNumber(static_cast<int32_t>(JSAsyncGeneratorFunction::AsyncGeneratorState::Completed)))); As mentioned on earlier patch, I don't think you need this. `state === Completed` should already never be true at this point, and if it is currently true sometimes, there's something else wrong. View in context: https://bugs.webkit.org/attachment.cgi?id=307013&action=review >>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4914 >>> + emitJumpIfTrue(emitGetById(newTemporary(), value.get(), propertyNames().done), loopDone.get()); >> >> This looks like it should be loading the "value" property, not "done" > > Nope. In current patch it leads to error. We need to check if Iterator that is invoked by yield * is finished, so we are checking done and if it is finished we go to loopDone, later we load value from value register (line 4842:4929 - emitGetById(value.get(), value.get(), propertyNames().value);) You're right. On another note, I think the AsyncGeneratorState read is out of place here, because this code should be unreachable if the generator is closed. >>> Source/JavaScriptCore/dfg/DFGNode.h:594 >>> + ASSERT(m_op == NewFunction || m_op == NewGeneratorFunction || m_op == NewAsyncFunction || m_op == NewAsyncGeneratorFunction); >> >> I still would prefer if changes to DFG/FTL could be deferred to a later bug, in order to keep the initial implementation as simple as possible. > > Ok. Not sure that I know how to avoid use DFG/B3 part with passing stress tests. You can limit failing tests to not run with optimizing compilers, using some combination of run-flags. Probably `//@ runNoJIT` and maybe a few other ones would be good too. Comment on attachment 311243 [details] Patch Attachment 311243 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3815375 New failing tests: js/parser-syntax-check.html Created attachment 311255 [details]
Archive of layout-test-results from ews103 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 311243 [details] Patch Attachment 311243 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3815388 New failing tests: js/parser-syntax-check.html Created attachment 311256 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 311243 [details] Patch Attachment 311243 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3815475 New failing tests: js/parser-syntax-check.html fetch/closing-while-fetching-blob.html Created attachment 311262 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Comment on attachment 311243 [details] Patch Attachment 311243 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3815494 New failing tests: js/parser-syntax-check.html js/es6-function-properties.html js/methods-names-should-not-be-in-lexical-scope.html Created attachment 311263 [details]
Archive of layout-test-results from ews115 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Does this need to be reviewed or is the review here stale? (In reply to Saam Barati from comment #73) > Does this need to be reviewed or is the review here stale? Currently, I'm fixing comments from caitp and failing tests. So I think it will be better to review next patch. Comment on attachment 311243 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311243&action=review > Source/JavaScriptCore/parser/Parser.cpp:2731 > + parseMode = SourceParseMode::GeneratorWrapperFunctionMode; This would need to be SourceParseMode::GeneratorWrapperMethodMode (see assertion failure in parser-syntax-check) > JSTests/stress/generator-class-methods-syntax.js:30 > +`, `SyntaxError: Cannot declare a generator function named 'constructor'.`); Why change this? the error message from Parser.cpp line 2811 is unchanged. Comment on attachment 311243 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311243&action=review I've prototyped suggested changes to the parser (diff is at https://www.diffchecker.com/lB9TEMZ6). I didn't get into the object literal parse method, but I'll look at that a bit tomorrow to see if it can be cleaned up a bit. > Source/JavaScriptCore/parser/Parser.cpp:-2643 > - isAsync = !isGenerator && !isAsyncMethod; I think we can clean this up a lot. Something like this: ``` case ASYNC: if (!isGeneratorMethodParseMode(parseMode) && !isAsyncMethodParseMode(parseMode)) { ident = m_token.m_data.ident; next(); if (match(OPENPAREN) || match(COLON) || match(EQUAL) || m_lexer->prevTerminator()) break; <-- If we get here, we can just treat this as an identifier, and avoid doing the getter/setter checks which are guaranteed to be false isAsync = true; << maybe unneeded, or specific to non-generators? if (UNLIKELY(consume(TIMES))) { isAsyncGenerator = true; parseMode = SourceParseMode::AsyncGeneratorWrapperMethodMode; } else { parseMode = SourceParseMode::AsyncMethodMode; } goto parseMethod; } FALLTHROUGH; <-- In the fall through case, just treat ASYNC as an identifier. ``` This allows deleting some code from the more common IDENT path, and I think it allows deleting the `wasAsync` state, etc. Can probably get rid of `isAsync` and `isAsyncGenerator` as well. > Source/JavaScriptCore/parser/Parser.cpp:2763 > + if (!isGeneratorMethodParseMode(parseMode) && !isAsyncMethodParseMode(parseMode) && !isAsyncGeneratorMethodParseMode(parseMode) && (matchIdentifierOrKeyword() || match(STRING) || match(DOUBLE) || match(INTEGER) || match(OPENBRACKET))) { Instead of all of these `!isGeneratorMethodPArseMode(parseMode) && !isAsyncMethod...`, how about just `if (parseMode == SourceParseMode::MethodMode && ...`? Easier to read maybe > Source/JavaScriptCore/parser/Parser.cpp:2768 > + parseMode = (UNLIKELY(isAsyncGenerator)) if my comments above are followed, I believe we can get rid of this whole branch > Source/JavaScriptCore/parser/Parser.cpp:2805 > parseMode = SourceParseMode::AsyncMethodMode; this overwrites the parse mode of an async generator, which will probably cause problems when lazily compiled code is reparsed. > Source/JavaScriptCore/parser/Parser.cpp:3806 > + parseMode = SourceParseMode::GeneratorWrapperFunctionMode; GeneratorWrapperMethodMode, like in parseClass() > Source/JavaScriptCore/parser/Parser.cpp:3812 > + mightBeTimes = true; I think this is slightly broken logic: isAsync might end up being false, e.g. in the case of `*async() {}`. I think this code will still set isAsyncGenerator = true if you do `*async* foo() {}`, though I'm not totally positive. A regression test would be good here. Is there any way we could make the parsing model be a bit closer to what I suggested for parseClass()? I'm pretty happy with the design I proposed up there, it's much cleaner than what I originally checked in IMO. > Source/JavaScriptCore/parser/Parser.cpp:3833 > + mightBeTimes = false; If we do end up needing `mightBeTimes` (can't handle this in `case ASYNC` as suggested before), maybe we could name it something clearer, like `maybeAsyncGenerator` or something. > Source/JavaScriptCore/parser/Parser.cpp:3873 > + parseMode = SourceParseMode::AsyncGeneratorWrapperMethodMode; we can delete these branches if we handle this in the token switch under `case ASYNC:` as suggested in parseClass() > Source/JavaScriptCore/parser/Parser.cpp:3930 > + ASSERT(parseMode == SourceParseMode::MethodMode || parseMode == SourceParseMode::GeneratorWrapperFunctionMode || parseMode == SourceParseMode::AsyncMethodMode || parseMode == SourceParseMode::AsyncGeneratorWrapperMethodMode); How about `ASSERT(isMethodParseMode(parseMode));`? Again, this line uses GeneratorWrapperFunctionMode rather than MethodMode > Source/JavaScriptCore/parser/ParserModes.h:172 > + SourceParseMode::GeneratorWrapperFunctionMode).contains(parseMode); s/Function/Method --- But do we really need this in place of just `parseMode == SourceParseMode::GeneratorWrapperMethodMode` ? > Source/JavaScriptCore/parser/ParserModes.h:182 > + return SourceParseModeSet(SourceParseMode::AsyncGeneratorWrapperFunctionMode).contains(parseMode); s/Function/Method/ --- same comment as before, though. Created attachment 312340 [details]
Patch
Fix comments to last uploade pd patch & more spec aligned to the lastest approved/merged spec
(In reply to Caitlin Potter (:caitp) from comment #76) > Comment on attachment 311243 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=311243&action=review > > I've prototyped suggested changes to the parser (diff is at > https://www.diffchecker.com/lB9TEMZ6). I didn't get into the object literal > parse method, but I'll look at that a bit tomorrow to see if it can be > cleaned up a bit. > > > Source/JavaScriptCore/parser/Parser.cpp:-2643 > > - isAsync = !isGenerator && !isAsyncMethod; > > I think we can clean this up a lot. > > Something like this: > > ``` > case ASYNC: > if (!isGeneratorMethodParseMode(parseMode) && > !isAsyncMethodParseMode(parseMode)) { > ident = m_token.m_data.ident; > next(); > if (match(OPENPAREN) || match(COLON) || match(EQUAL) || > m_lexer->prevTerminator()) > break; <-- If we get here, we can just treat this as > an identifier, and avoid doing the getter/setter checks which are guaranteed > to be false > > isAsync = true; << maybe unneeded, or specific to > non-generators? > if (UNLIKELY(consume(TIMES))) { > isAsyncGenerator = true; > parseMode = > SourceParseMode::AsyncGeneratorWrapperMethodMode; > } else { > parseMode = SourceParseMode::AsyncMethodMode; > } > goto parseMethod; > } > FALLTHROUGH; <-- In the fall through case, just treat ASYNC as > an identifier. > ``` > > This allows deleting some code from the more common IDENT path, and I think > it allows deleting the `wasAsync` state, etc. Can probably get rid of > `isAsync` and `isAsyncGenerator` as well. > > > Source/JavaScriptCore/parser/Parser.cpp:2763 > > + if (!isGeneratorMethodParseMode(parseMode) && !isAsyncMethodParseMode(parseMode) && !isAsyncGeneratorMethodParseMode(parseMode) && (matchIdentifierOrKeyword() || match(STRING) || match(DOUBLE) || match(INTEGER) || match(OPENBRACKET))) { > > Instead of all of these `!isGeneratorMethodPArseMode(parseMode) && > !isAsyncMethod...`, how about just `if (parseMode == > SourceParseMode::MethodMode && ...`? Easier to read maybe > > > Source/JavaScriptCore/parser/Parser.cpp:2768 > > + parseMode = (UNLIKELY(isAsyncGenerator)) > > if my comments above are followed, I believe we can get rid of this whole > branch > > > Source/JavaScriptCore/parser/Parser.cpp:2805 > > parseMode = SourceParseMode::AsyncMethodMode; > > this overwrites the parse mode of an async generator, which will probably > cause problems when lazily compiled code is reparsed. > > > Source/JavaScriptCore/parser/Parser.cpp:3806 > > + parseMode = SourceParseMode::GeneratorWrapperFunctionMode; > > GeneratorWrapperMethodMode, like in parseClass() > > > Source/JavaScriptCore/parser/Parser.cpp:3812 > > + mightBeTimes = true; > > I think this is slightly broken logic: > > isAsync might end up being false, e.g. in the case of `*async() {}`. I think > this code will still set isAsyncGenerator = true if you do `*async* foo() > {}`, though I'm not totally positive. A regression test would be good here. > > Is there any way we could make the parsing model be a bit closer to what I > suggested for parseClass()? I'm pretty happy with the design I proposed up > there, it's much cleaner than what I originally checked in IMO. > > > Source/JavaScriptCore/parser/Parser.cpp:3833 > > + mightBeTimes = false; > > If we do end up needing `mightBeTimes` (can't handle this in `case ASYNC` as > suggested before), maybe we could name it something clearer, like > `maybeAsyncGenerator` or something. > > > Source/JavaScriptCore/parser/Parser.cpp:3873 > > + parseMode = SourceParseMode::AsyncGeneratorWrapperMethodMode; > > we can delete these branches if we handle this in the token switch under > `case ASYNC:` as suggested in parseClass() > > > Source/JavaScriptCore/parser/Parser.cpp:3930 > > + ASSERT(parseMode == SourceParseMode::MethodMode || parseMode == SourceParseMode::GeneratorWrapperFunctionMode || parseMode == SourceParseMode::AsyncMethodMode || parseMode == SourceParseMode::AsyncGeneratorWrapperMethodMode); > > How about `ASSERT(isMethodParseMode(parseMode));`? Again, this line uses > GeneratorWrapperFunctionMode rather than MethodMode > > > Source/JavaScriptCore/parser/ParserModes.h:172 > > + SourceParseMode::GeneratorWrapperFunctionMode).contains(parseMode); > > s/Function/Method --- But do we really need this in place of just `parseMode > == SourceParseMode::GeneratorWrapperMethodMode` ? > > > Source/JavaScriptCore/parser/ParserModes.h:182 > > + return SourceParseModeSet(SourceParseMode::AsyncGeneratorWrapperFunctionMode).contains(parseMode); > > s/Function/Method/ --- same comment as before, though. Thanks for review. I've mostly fixed all your comments, I've back to recursion call of the asyncGeneratorResumeNext, to be more close to spec. I will back to loop after implementation of new version of spec. Also I decided to left DFG/FTL part in patch, it is not so big change. Comment on attachment 312340 [details] Patch Attachment 312340 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3895987 Number of test failures exceeded the failure limit. Created attachment 312349 [details]
Archive of layout-test-results from ews101 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 312340 [details] Patch Attachment 312340 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3896012 Number of test failures exceeded the failure limit. Created attachment 312350 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 312340 [details] Patch Attachment 312340 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3895997 Number of test failures exceeded the failure limit. Created attachment 312352 [details]
Archive of layout-test-results from ews117 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 312340 [details] Patch Attachment 312340 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3896075 New failing tests: jquery/deferred.html imported/w3c/web-platform-tests/streams/readable-streams/general.html jquery/css.html jquery/offset.html jquery/traversing.html jquery/attributes.html jquery/core.html jquery/dimensions.html jquery/data.html imported/w3c/web-platform-tests/streams/readable-streams/general.dedicatedworker.html jquery/event.html Created attachment 312356 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Comment on attachment 312340 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312340&action=review Just a few more points about the parser, but otherwise the changes look really nice. I'll take a look at the runtime again once Domenic uploads the rest of the spec change > Source/JavaScriptCore/parser/Parser.cpp:3826 > + failIfTrue(m_lexer->prevTerminator(), "Expected a property name following keyword 'async'"); I am not sure this is the best way to go here. Example: `({ async : 1 })` I think this line would make that an error, but it should be legal, right? > Source/JavaScriptCore/parser/Parser.cpp:3840 > + parseMode = parseMode = SourceParseMode::AsyncMethodMode; please remove the duplicate `parseMode = ` > Source/JavaScriptCore/parser/Parser.cpp:3851 > + if (UNLIKELY(!isIdentSet)) { I'm not sure I see the point in the `isIdentSet` thing --- it seems to add complexity without necessarily being worthwhile. I'm happy to be convinced otherwise if it turns out to shave off parse time. Comment on attachment 312340 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312340&action=review > Source/JavaScriptCore/parser/Parser.cpp:3828 > + goto parseProperty; Won't this most likely end up on the default case below (particularly for COLON and EQUAL) and report an error? That doesn't seem right. Let me take a look at this... Yeah, just tried this in the repl: ``` >>> ({ ... async ... : 1 }) Unexpected token ':'. Expected a property name following keyword 'async'.:3 >>> ({ async = 1 }) Unexpected token '='. Expected a property name.:1 >>> ({ async: 1 }) Unexpected token ':'. Expected a property name.:1 ``` there are some problems here. Let me see if I can sort this out and I'll send you some suggestions Comment on attachment 312340 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312340&action=review > Source/JavaScriptCore/parser/Parser.cpp:3819 > + if (!isGeneratorMethodParseMode(parseMode) && !isAsyncMethodParseMode(parseMode)) { Here's what I came up with for the ASYNC case: ``` case ASYNC: if (parseMode == SourceParseMode::MethodMode) { SavePoint savePoint = createSavePoint(); next(); if (match(COLON) || match(OPENPAREN) || match(COMMA) || match(CLOSEBRACE)) { restoreSavePoint(savePoint); wasIdent = true; goto namedProperty; } failIfTrue(m_lexer->prevTerminator(), "Expected a property name following keyword 'async'"); isAsync = true; if (UNLIKELY(consume(TIMES))) { isAsyncGenerator = true; parseMode = SourceParseMode::AsyncGeneratorWrapperMethodMode; } else parseMode = parseMode = SourceParseMode::AsyncMethodMode; goto parseProperty; } FALLTHROUGH; ``` So, this would produce a bit of a hiccup for properties named "async" because of the save state creation/restoration, but it's probably not going to show up on any benchmarks, and the cost should be pretty minor. I've removed the `match(EQUAL)` thing, because shorthand property names can't have initializers, and I've re-arranged the prevTerminator check to get a better error message for the following case: ``` ({ async foo() {} }) ``` I think this should fixup the parser issues from the last patch. :caitp Thanks for comments, I'll back with fixes soon Comment on attachment 312340 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312340&action=review I've had a quick look, left a few comments. Looking forward to seeing this land in JSC. > Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:29 > + if (typeof syncIterator !== 'object') new @TypeError('Only objects can be wrapped by async-from-sync wrapper'); Please use the intrinsic `@isObject(syncIterator)`. `typeof syncIterator !== 'object'` does not correspond to the spec clause > Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:32 > + asyncIterator.@syncIterator = syncIterator; If the syncIterator is a parameter to the constructor, we can take advantage of TCO. Might be a good idea? > Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:40 > + this.next = @next; I don't understand these 3 lines. Why not just use the prototype object for this instead? It shouldn't be observable either way AFAIK, so if this is faster in JSC I suppose it's fine. > Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:55 > + if (typeof O !== 'object' || typeof O.@syncIterator === 'undefined') { So, it's possible to get at these functions `(next, throw, return)` using function.caller, that's the only way this could really happen, though. In either case, the shape of the async-from-sync iterator object should always be immutable, so it's probably cheaper to implement something which simply checks the shape of the object rather than performing the named lookup twice (I certainly don't think that's a blocker, and implementing a new type-checking intrinsic may be more trouble than it's worth). Also again, `typeof O !== 'object'` is not enough to match the spec clause. A test cause you could use to verify this: ```js function customIterator() {} let value = 0; customIterator.next = function() { let done = ++i > 5; return { value, done }; } async function f() { let sum = 0; for await (let x of customIterator) { console.log(x); sum += x; } } f().then( function(sum) { assert(sum === 15); }, function(error) { fail(error); // will fail because of the rejection on line line 56 }); ``` I'm sure this has shown up in other places in this patch, I'll try to mark them down so they're easy to find and fix. > Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:62 > + const { done, value } = nextResult; IteratorComplete() needs to convert `done` to a boolean value. This step could be deferred until the resolve call on line 66. This is observable due to the function.caller thing mentioned above. > Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:83 > + if (typeof O !== 'object' || typeof O.@syncIterator === 'undefined') { Also `@isObject(O)` rather than typeof here. > Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:126 > + if (typeof O !== 'object' || typeof O.@syncIterator === 'undefined') { Also `@isObject(O)` rather than typeof here. > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:74 > + const valueWrapperCapability = @newPromiseCapability(@Promise); In https://github.com/tc39/proposal-async-iteration/pull/102/, AsyncGeneratorResolve no longer performs value unwrapping > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:92 > + @asyncGeneratorResolve(generator, value, false); Per https://github.com/tc39/proposal-async-iteration/pull/102, AsyncGeneratorYield now needs to perform `Set _value_ to ? Await(_value_).`. I think you'll need to insert Awaits in the parser/bytecode-generator, I'll leave notes where I think is appropriate. You can probably keep this helper as it is, though. > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:142 > + @awaitValue(generator, value, onFulfilled); This doesn't make a lot of sense to me. It looks like if any awaited Promise is rejected, the generator is immediately closed/aborted, even if there's a try/catch or try/finally around the await. Is this not true? ```js async function* g() { try { await Promise.reject("doop"); } finally { yield* [1, 2, 3]; // Is this line reachable in this implementation? } } ``` > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:182 > + return @asyncGeneratorResolve(generator, next.value, true); https://github.com/tc39/proposal-async-iteration/pull/102/ stuff: you need to do Promise unwrapping with some new closures (similar to the Async-from-Sync Iterator ones) which do AsyncGeneratorResolve / AsyncGeneratorReject respectively. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4806 > + emitYieldPoint(argument, result); Per https://github.com/tc39/proposal-async-iteration/pull/102, there's an implicit `await` for async generator yields. You'll need to await the argument here (at least for normal yields) Also new in that spec change, the `.return` handler needs to await the sent value before continuing. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4852 > + const Identifier& propertyName = parseMode() == SourceParseMode::AsyncGeneratorBodyMode ? propertyNames().asyncIteratorSymbol : propertyNames().iteratorSymbol; I like this helper --- It's probably a good idea to either leave a comment indicating that this will always pick the iterator type based on function type, or else to make it possible to specify the iterator type using something like the hint parameter in the spec method. This would allow it to be used by other GetIterator callers. Comment on attachment 312340 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312340&action=review >> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:55 >> + if (typeof O !== 'object' || typeof O.@syncIterator === 'undefined') { > > So, it's possible to get at these functions `(next, throw, return)` using function.caller, that's the only way this could really happen, though. In either case, the shape of the async-from-sync iterator object should always be immutable, so it's probably cheaper to implement something which simply checks the shape of the object rather than performing the named lookup twice (I certainly don't think that's a blocker, and implementing a new type-checking intrinsic may be more trouble than it's worth). > > Also again, `typeof O !== 'object'` is not enough to match the spec clause. A test cause you could use to verify this: > > ```js > function customIterator() {} > let value = 0; > customIterator.next = function() { > let done = ++i > 5; > return { value, done }; > } > > async function f() { > let sum = 0; > for await (let x of customIterator) { > console.log(x); > sum += x; > } > } > > f().then( > function(sum) { > assert(sum === 15); > }, > function(error) { > fail(error); // will fail because of the rejection on line line 56 > }); > ``` > > I'm sure this has shown up in other places in this patch, I'll try to mark them down so they're easy to find and fix. I may have been mistaken about it being observable through function.caller (though it is in v8, for now). ishell@ had pointed out the annex B forbidden extensions clause which presumably prevents the builtin Async-from-Sync Iterator methods from being returns to the caller. > If an implementation extends non-strict or built-in function objects with an own property named "caller" the value of that property, as observed using [[Get]] or [[GetOwnProperty]], must not be a strict function object. If it is an accessor property, the function that is the value of the property's [[Get]] attribute must never return a strict function when called. The other part of that above comment still applies though. I think not being returnable via Function.caller makes the type checks in the Async-from-Sync Iterator methods redundant, they could be assertions instead. Created attachment 313999 [details]
Patch
Align with latest version of the spec and fixed comments
Created attachment 314003 [details]
Patch
Align with latest version of the spec and fixed comments and small additional fixes
Comment on attachment 313999 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313999&action=review Coming along nicely, great work so far. > Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:35 > + promiseCapability.@reject.@call(@undefined, new @TypeError('syncIterator property is missing.')); Is this still reachable? As discussed previously, I don't think these methods are accessible by users (not through function.caller anyways), so it should be impossible for the receiver to have the wrong type. > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:113 > + @awaitValue(generator, value, asyncGeneratorYieldAwaited); You need to resume the generator with a throw completion if this await fails, like you do for non-yield awaits. Otherwise catch/finally handlers aren't invoked. This is different from how it was when AsyncGeneratorResolve did the unwrapping itself, rather than using Await. The default "AsyncGeneratorReject" call is really only needed for uncaught exceptions. ``` async function* g() { try { yield Promise.reject("donuts"); assertUnreachable(); } catch (e) { assert(e === "donuts"); yield "churros"; } } let it = g(); it.next() .then(x => assert(x.value === "churros"), e => assertUnreachable()) .finally(() => { it.next() .then(x => assert(x.value === undefined), assert(x.done === true), e => assertUnreachable()); }); ``` > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:128 > + @asyncGeneratorReject(generator, error); above comment reflects this > Source/JavaScriptCore/parser/Parser.cpp:2733 > + bool wasAsync = false; wasAsync still not used > Source/JavaScriptCore/parser/Parser.cpp:2760 > + isAsyncGenerator = true; `isAsyncGenerator` still isn't actually used by anything, AFAICT.`isAsync` is no longer used. > Source/JavaScriptCore/parser/Parser.cpp:3807 > + bool isAsyncGenerator = false; isAsyncGenerator and isAsync still not used Created attachment 314040 [details]
Patch
Rebased version
Created attachment 314046 [details]
Patch
Fix comments
Comment on attachment 314046 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314046&action=review > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:216 > + return @asyncGeneratorResolve(generator, next.value, true); You need to do the unwrapping from lines 205-208 for this line too (https://tc39.github.io/proposal-async-iteration/#sec-asyncgeneratorresumenext steps 10.a and 10.b) --- You can probbably combine them similar to how it's done in the spec Test case: ```js async function* g() { yield 1; } let it = g(); it.next(); it.next(); // state is now @AsyncGeneratorStateCompleted it.return(Promise.resolve("Should be a string!")).then( function({ value }) { print(value === "Should be a string!"); // Should be true, but is false due to Promise not being unwrapped }); ``` > Source/JavaScriptCore/parser/Parser.cpp:3905 > + if (complete || (wasIdent && !isGeneratorMethodParseMode(parseMode) && (*ident == m_vm->propertyNames->get || *ident == m_vm->propertyNames->set)) || isAsync) You don't need `isAsync` here because the name following the `async` keyword was already lexed on line 3877. You can acceptably use the else-clause in this case. Unrelated, but could someone explain why we use `nextExpectIdentifier()` in the else clause, since in non-error cases it will always end up on the slow path anyways? Created attachment 314159 [details]
Patch
Fix the latest comments
Comment on attachment 314046 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314046&action=review >> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:216 >> + return @asyncGeneratorResolve(generator, next.value, true); > > You need to do the unwrapping from lines 205-208 for this line too (https://tc39.github.io/proposal-async-iteration/#sec-asyncgeneratorresumenext steps 10.a and 10.b) --- You can probbably combine them similar to how it's done in the spec > > Test case: > > ```js > async function* g() { yield 1; } > let it = g(); > it.next(); > it.next(); // state is now @AsyncGeneratorStateCompleted > it.return(Promise.resolve("Should be a string!")).then( > function({ value }) { > print(value === "Should be a string!"); // Should be true, but is false due to Promise not being unwrapped > }); > ``` Ohh, I missed that. Fixed in new patch. >> Source/JavaScriptCore/parser/Parser.cpp:3905 >> + if (complete || (wasIdent && !isGeneratorMethodParseMode(parseMode) && (*ident == m_vm->propertyNames->get || *ident == m_vm->propertyNames->set)) || isAsync) > > You don't need `isAsync` here because the name following the `async` keyword was already lexed on line 3877. You can acceptably use the else-clause in this case. > > Unrelated, but could someone explain why we use `nextExpectIdentifier()` in the else clause, since in non-error cases it will always end up on the slow path anyways? Fixed Comment on attachment 314159 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314159&action=review Haven't looked at all of the changes, but here's a few more comments about yield* in particular, since it's fresh in my mind from v8 work. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4849 > + if (parseMode() == SourceParseMode::AsyncGeneratorBodyMode) { I don't think this is an acceptable way to do this. The decision making about whether to actually perform an await does not belong inside this function, but in the caller function. Also, the way this is written, it's not useful for any other situation where you'd want to emit an Await. Can we refactor this to be more like this: ``` switch (parseMode()) { case SourceParseMode::AsyncGeneratorBodyMode: // Do the stuff... case SourceParseMode::AsyncFunctionBodyMode: // Do the stuff... case SourceParseMode::AsyncArrowFunctionBodyMode: // Do the stuff... default: RELEASE_ASSERT_NOT_REACHED(); return nullptr; } ``` and thus defer the decision making to the caller? > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4870 > + emitJumpIfFalse(emitIsUndefined(newTemporary(), iterator.get()), asyncIteratorFound.get()); You need to also jump if the iterator method is `null` (The GetMethod() operation from `Set method to ? GetMethod(obj, @@asyncIterator).` converts "null" to undefined). Additionally, it looks like `op_is_undefined` takes the masqueradesAsUndefined flag into account, which I think would lead to incorrect behaviour if you did something like `for await (let x of { [Symbol.asyncIterator]: document.all }) {}`. Kind of a weird case. It should try to call document.all as a function, but it will try to load the sync iterator instead. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4910 > + emitYieldPoint(value.get(), JSAsyncGeneratorFunction::AsyncGeneratorSuspendReason::Yield); Per spec: `If generatorKind is async, then let received be AsyncGeneratorYield(? IteratorValue(innerResult)).` So, when we do the yield, we need to await the value component first > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4913 > Ref<Label> returnLabel = newLabel(); Some comments (can't add them inline :( since your changes don't affect those lines) for Throw resumption: If the "throw" method is not found, you need to perform AsyncIteratorClose(), which Awaits the result of the .return method, preventing execution from continuing until async cleanup has been completed. You also need to await the result of the "throw" method if it is present, before checking .done etc. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4967 > + emitAwait(value.get()); This Await needs to happen before testing if innerResult is an object or not. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4971 > Again, the `emitGetById(value.get(), value.get(), propertyNames().value);` is only technically correct for async generators. For sync generators, supposed to yield the same iterator result returned from the .return() call. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4990 > emitIteratorNextWithValue(value.get(), iterator.get(), value.get(), node); emitIteratorNextWithValue() does its own "iterator result is not an object" exception before the value has been awaited, which is incorrect for async generators. Maybe need to make that function conditionally await the return value (but please don't have it check parseMode() itself). > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4995 > + Ref<Label> iteratorValueIsObject = newLabel(); Per the above comment, we can remove this block, it will be redundant once the await is added in the appropriate place > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:5001 > emitGetById(value.get(), value.get(), propertyNames().value); This is correct for async generators, but it seems like it was incorrect for sync generators, which should have yielded the exact iterator result returned from whichever iterator method. No need to worry about it in this patch, but this is clearly a bug, worth filing: ``` var O = { [Symbol.iterator]() { return this; }, next() { return { floof: 1, done: false, value: 1 }; } }; function* g() { yield* O; } let it = g(); it.next(); // { done: false, value: 1; } --- should be { floof: 1, done: false, value: 1 }, same object returned from .next(); ``` Created attachment 316209 [details]
Patch
Fix comments
Comment on attachment 314159 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314159&action=review Thanks for review. I've most comments >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4849 >> + if (parseMode() == SourceParseMode::AsyncGeneratorBodyMode) { > > I don't think this is an acceptable way to do this. The decision making about whether to actually perform an await does not belong inside this function, but in the caller function. Also, the way this is written, it's not useful for any other situation where you'd want to emit an Await. > > Can we refactor this to be more like this: > > ``` > switch (parseMode()) { > case SourceParseMode::AsyncGeneratorBodyMode: > // Do the stuff... > case SourceParseMode::AsyncFunctionBodyMode: > // Do the stuff... > case SourceParseMode::AsyncArrowFunctionBodyMode: > // Do the stuff... > default: > RELEASE_ASSERT_NOT_REACHED(); > return nullptr; > } > ``` > > and thus defer the decision making to the caller? done >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4870 >> + emitJumpIfFalse(emitIsUndefined(newTemporary(), iterator.get()), asyncIteratorFound.get()); > > You need to also jump if the iterator method is `null` (The GetMethod() operation from `Set method to ? GetMethod(obj, @@asyncIterator).` converts "null" to undefined). > > Additionally, it looks like `op_is_undefined` takes the masqueradesAsUndefined flag into account, which I think would lead to incorrect behaviour if you did something like `for await (let x of { [Symbol.asyncIterator]: document.all }) {}`. Kind of a weird case. It should try to call document.all as a function, but it will try to load the sync iterator instead. I've add one more condition with checking for `null` Hmm, I'll build webkit and look to the document.all function >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4910 >> + emitYieldPoint(value.get(), JSAsyncGeneratorFunction::AsyncGeneratorSuspendReason::Yield); > > Per spec: `If generatorKind is async, then let received be AsyncGeneratorYield(? IteratorValue(innerResult)).` > > So, when we do the yield, we need to await the value component first It is start We do AsyncGeneratorYield in each point. >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4913 >> Ref<Label> returnLabel = newLabel(); > > Some comments (can't add them inline :( since your changes don't affect those lines) > > for Throw resumption: If the "throw" method is not found, you need to perform AsyncIteratorClose(), which Awaits the result of the .return method, preventing execution from continuing until async cleanup has been completed. You also need to await the result of the "throw" method if it is present, before checking .done etc. Yeah, it does by emitIteratorClose() method. Await for throw is done after branchOnResult label >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4967 >> + emitAwait(value.get()); > > This Await needs to happen before testing if innerResult is an object or not. Done >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4990 >> emitIteratorNextWithValue(value.get(), iterator.get(), value.get(), node); > > emitIteratorNextWithValue() does its own "iterator result is not an object" exception before the value has been awaited, which is incorrect for async generators. Maybe need to make that function conditionally await the return value (but please don't have it check parseMode() itself). Done >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4995 >> + Ref<Label> iteratorValueIsObject = newLabel(); > > Per the above comment, we can remove this block, it will be redundant once the await is added in the appropriate place I've just remove checking isObject from emitIteratorNextWithValue. We reuse this block in `throw` case as well. >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:5001 >> emitGetById(value.get(), value.get(), propertyNames().value); > > This is correct for async generators, but it seems like it was incorrect for sync generators, which should have yielded the exact iterator result returned from whichever iterator method. > > No need to worry about it in this patch, but this is clearly a bug, worth filing: > > ``` > var O = { [Symbol.iterator]() { return this; }, next() { return { floof: 1, done: false, value: 1 }; } }; > function* g() { yield* O; } > let it = g(); > it.next(); // { done: false, value: 1; } --- should be { floof: 1, done: false, value: 1 }, same object returned from .next(); > ``` Bug is created Comment on attachment 314159 [details]
Patch
Thanks for review.I've fixed most of the comments
Comment on attachment 316209 [details] Patch Attachment 316209 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4169839 Number of test failures exceeded the failure limit. Created attachment 316213 [details]
Archive of layout-test-results from ews103 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 316209 [details] Patch Attachment 316209 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4169879 Number of test failures exceeded the failure limit. Created attachment 316215 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 316209 [details] Patch Attachment 316209 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4169847 Number of test failures exceeded the failure limit. Created attachment 316217 [details]
Archive of layout-test-results from ews115 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 316209 [details] Patch Attachment 316209 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4169860 New failing tests: jquery/deferred.html imported/w3c/web-platform-tests/streams/readable-streams/general.html jquery/css.html jquery/offset.html jquery/traversing.html jquery/attributes.html jquery/core.html jquery/dimensions.html jquery/data.html imported/w3c/web-platform-tests/streams/readable-streams/general.dedicatedworker.html jquery/event.html Created attachment 316220 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Here's a quick fix for (I think) all the new test failures. Also tries to simplify things a bit. https://gist.github.com/caitp/e47f3637adfc14908c4935c15f364301 Created attachment 316237 [details]
Patch
Fix tests
(In reply to Caitlin Potter (:caitp) from comment #113) > Here's a quick fix for (I think) all the new test failures. Also tries to > simplify things a bit. > > https://gist.github.com/caitp/e47f3637adfc14908c4935c15f364301 Thanks for help! Interesting that JSC tests worked correctly :-) Comment on attachment 316237 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316237&action=review Few more comments --- Please take another look at the earlier runtime comments I had provided, because I think there may be some duplicates here :) > Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:29 > + if (typeof syncIterator !== 'object') new @TypeError('Only objects can be wrapped by async-from-sync wrapper'); Use `@isObject()` to get the proper `Type(O) is Object` behaviour. `typeof null` is "object", after all. Also, shouldn't this be `@throwTypeError` instead of `new @TypeError`? > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:73 > + const { promiseCapability } = next; AsyncGeneratorResolve no longer does this unwrapping (https://tc39.github.io/proposal-async-iteration/#sec-asyncgeneratorresolve). I think I left this as a comment on a previous patch. > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:91 > + generator.@asyncGeneratorSuspendReason = @AsyncGeneratorSuspendReasonYield; So, I see that you're trying to do the `await` in AsyncGeneratorResolve, but there's a problem: the resolve and reject closures won't do exactly what's needed. The `@asyncGeneratorResumeNext(generator)` call on line 81 doesn't make sense, because the generator is effectively suspended for an Await (so it won't/shouldn't do the resume). Instead, the ResumeNext() needs to happen in the promise resolve/reject closures. A good way to model it: ``` function AsyncGeneratorYield(generator, value) { generator.@asyncGeneratorSuspendReason = @AsyncGeneratorYield; const onFulfilled = function(value) { @asyncGeneratorResolve(generator, value, false); return @asyncGeneratorResumeNext(generator); } @awaitValue(generator, value, onFulfilled); } ``` This will resume the generator correctly after the Awaits. It's possible the algorithm may need a slight adjustment, but I believe it's correct. > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:105 > + @asyncGeneratorReject(generator, error); this should be ```js const onRejected = function(error) { @doAsyncGeneratorBodyCall(generator, error, @GeneratorResumeModeThrow); return @asyncGeneratorResumeNext(generator); } ``` this should allow catch handlers to fire if the `await` fails. > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:182 > + return @asyncGeneratorResolve(generator, next.value, true); we need to `await` the sent value before performing @asyncGeneratorResolve --- in this case, if the `await` succeeds or fails, there's no need to perform `@doAsyncGeneratorBodyCall()` --- unlike other cases where it is needed. For this reason, the promise unwrapping can't happen within @asyncGeneratorResolve. > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:198 > +function isExectionState(generator) this looks like a typo. `isExecutingState` maybe? `isExecutionState`? Created attachment 316312 [details]
Patch
Update patch
Comment on attachment 316237 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316237&action=review >> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:29 >> + if (typeof syncIterator !== 'object') new @TypeError('Only objects can be wrapped by async-from-sync wrapper'); > > Use `@isObject()` to get the proper `Type(O) is Object` behaviour. `typeof null` is "object", after all. > > Also, shouldn't this be `@throwTypeError` instead of `new @TypeError`? OMG! I've applied last fixed to outdate patch. (I was after vacation :-( ) >> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:73 >> + const { promiseCapability } = next; > > AsyncGeneratorResolve no longer does this unwrapping (https://tc39.github.io/proposal-async-iteration/#sec-asyncgeneratorresolve). I think I left this as a comment on a previous patch. Fixed, by using correct patch >> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:91 >> + generator.@asyncGeneratorSuspendReason = @AsyncGeneratorSuspendReasonYield; > > So, I see that you're trying to do the `await` in AsyncGeneratorResolve, but there's a problem: the resolve and reject closures won't do exactly what's needed. > > The `@asyncGeneratorResumeNext(generator)` call on line 81 doesn't make sense, because the generator is effectively suspended for an Await (so it won't/shouldn't do the resume). Instead, the ResumeNext() needs to happen in the promise resolve/reject closures. > > A good way to model it: > > ``` > function AsyncGeneratorYield(generator, value) { > generator.@asyncGeneratorSuspendReason = @AsyncGeneratorYield; > > const onFulfilled = function(value) { > @asyncGeneratorResolve(generator, value, false); > > return @asyncGeneratorResumeNext(generator); > } > @awaitValue(generator, value, onFulfilled); > } > > ``` > > This will resume the generator correctly after the Awaits. It's possible the algorithm may need a slight adjustment, but I believe it's correct. Yeah, you are right, could you please revalidate if it correct in the latest patch. >> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:105 >> + @asyncGeneratorReject(generator, error); > > this should be > > ```js > const onRejected = function(error) { > @doAsyncGeneratorBodyCall(generator, error, @GeneratorResumeModeThrow); > > return @asyncGeneratorResumeNext(generator); > } > ``` > > this should allow catch handlers to fire if the `await` fails. Fixed in new patch. >> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:182 >> + return @asyncGeneratorResolve(generator, next.value, true); > > we need to `await` the sent value before performing @asyncGeneratorResolve --- in this case, if the `await` succeeds or fails, there's no need to perform `@doAsyncGeneratorBodyCall()` --- unlike other cases where it is needed. For this reason, the promise unwrapping can't happen within @asyncGeneratorResolve. Rewritted in new patch. >> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:198 >> +function isExectionState(generator) > > this looks like a typo. `isExecutingState` maybe? `isExecutionState`? Fixed Comment on attachment 316312 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316312&action=review OK --- a few more comments, I think this could start using a look from Yusuke and Saam, though. > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:36 > +function asyncGeneratorGetFirstItemFromQueue(generator) nit: rename `Get` to `Take` / `Remove`, to be clear about intent here (the queue no longer owns/references the request) > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:95 > + @asyncGeneratorResumeNext(generator); Good opportunity for a tail-call here > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:111 > + generator.@asyncGeneratorSuspendReason = @AsyncGeneratorSuspendReasonNone; Shouldn't this be @AsyncGeneratorSuspendReasonAwait? The WebInspector might eventually care about the distinction > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:174 > + @awaitValue(generator, value, onFulfilled); In this case, you need a different reject handler which does not try to re-enter the generator body > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:219 > + @asyncGeneratorReject(generator, next.value); since @asyncGeneratorReject recurses into asyncGeneratorResumeNext, it's a good opportunity for a tail-call (both here and in @asyncGeneratorReject) Arg, I left a few more comments in BytecodeGenerator.cpp, but they didn't seem to get submitted! I'll try to summarize here (at least the ones which are still in memory): 1) in emitIteratorClose, don't use the function parseMode to determine whether to perform AsyncIteratorClose or not. Ordinary for-of loops (for example) should not await the result of the .return method. 2) Don't do the `await` in `doAsyncGeneratorBodyCall` after resuming, when the state becomes completed. The spec only asks you to `await` the operand of ReturnStatements with an explicit operand (https://tc39.github.io/proposal-async-iteration/#sec-return-statement). The extra Await is technically observable, so it's better if implementations match up. Instead, this should be handled in BytecodeGenerator. Created attachment 316462 [details]
Patch
Fix latest comments
Comment on attachment 316312 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316312&action=review >> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:36 >> +function asyncGeneratorGetFirstItemFromQueue(generator) > > nit: rename `Get` to `Take` / `Remove`, to be clear about intent here (the queue no longer owns/references the request) Renamed to asyncGeneratorDeueue >> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:95 >> + @asyncGeneratorResumeNext(generator); > > Good opportunity for a tail-call here Done >> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:111 >> + generator.@asyncGeneratorSuspendReason = @AsyncGeneratorSuspendReasonNone; > > Shouldn't this be @AsyncGeneratorSuspendReasonAwait? The WebInspector might eventually care about the distinction Fixed >> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:174 >> + @awaitValue(generator, value, onFulfilled); > > In this case, you need a different reject handler which does not try to re-enter the generator body Fixed >> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:219 >> + @asyncGeneratorReject(generator, next.value); > > since @asyncGeneratorReject recurses into asyncGeneratorResumeNext, it's a good opportunity for a tail-call (both here and in @asyncGeneratorReject) Done (In reply to Caitlin Potter (:caitp) from comment #120) > Arg, I left a few more comments in BytecodeGenerator.cpp, but they didn't > seem to get submitted! I'll try to summarize here (at least the ones which > are still in memory): > > 1) in emitIteratorClose, don't use the function parseMode to determine > whether to perform AsyncIteratorClose or not. Ordinary for-of loops (for > example) should not await the result of the .return method. > > 2) Don't do the `await` in `doAsyncGeneratorBodyCall` after resuming, when > the state becomes completed. The spec only asks you to `await` the operand > of ReturnStatements with an explicit operand > (https://tc39.github.io/proposal-async-iteration/#sec-return-statement). The > extra Await is technically observable, so it's better if implementations > match up. Instead, this should be handled in BytecodeGenerator. Fixed both comments. Thanks for patience in reviewing of my patches! Created attachment 316652 [details]
Patch
Use assert instead of throwTypeError
Comment on attachment 316652 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316652&action=review I think it's starting to look pretty good. Just a couple nits. I noticed that for-await-of isn't implemented in this CL. Are you planning on adding that after? > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4858 > + RefPtr<RegisterID> iterator = emitGetIteratorByCurrentFunctionType(argument); nit: Can we do something more like: ``` RefPtr<RegisterID> iterator = parseMode() == SourceParseMode::AsyncGeneratorBodyMode ? emitGetAsyncIterator(argument) : emitGetIterator(argument); ``` where `emitGetAsyncIterator` includes the creation of the async-from-sync iterator if the @@asyncIterator method isn't found? Then you could reuse it for for-await-of loops, once those are implemented. That's sort of what I was talking about --- either have 2 separate functions for the different GetIterator algorithms, or pass some kind of flag to the function as a parameter indicating how it should work. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:781 > + void emitIteratorClose(RegisterID* iterator, const ThrowableExpressionData* node, bool doEmitAwait = false); nit: I think it would be preferred to use an enum instead of a boolean for `doEmitAwait`, but I dunno. See what proper reviewers think, I guess. As with GetIterator, I'd also be ok with a separate function implementing that algorithm (`emitAsyncIteratorClose` I guess). Up to you / other reviewers. (In reply to Caitlin Potter (:caitp) from comment #125) > Comment on attachment 316652 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=316652&action=review > > I think it's starting to look pretty good. Just a couple nits. > > I noticed that for-await-of isn't implemented in this CL. Are you planning > on adding that after? Yeah, I've tried to decrease size of the patch. There separate issue for for-await-of syntax https://bugs.webkit.org/show_bug.cgi?id=166698 > > > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4858 > > + RefPtr<RegisterID> iterator = emitGetIteratorByCurrentFunctionType(argument); > > nit: Can we do something more like: > > ``` > RefPtr<RegisterID> iterator = parseMode() == > SourceParseMode::AsyncGeneratorBodyMode ? emitGetAsyncIterator(argument) : > emitGetIterator(argument); > ``` > > where `emitGetAsyncIterator` includes the creation of the async-from-sync > iterator if the @@asyncIterator method isn't found? Then you could reuse it > for for-await-of loops, once those are implemented. That's sort of what I > was talking about --- either have 2 separate functions for the different > GetIterator algorithms, or pass some kind of flag to the function as a > parameter indicating how it should work. Oh OK. I see what you mean > > > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:781 > > + void emitIteratorClose(RegisterID* iterator, const ThrowableExpressionData* node, bool doEmitAwait = false); > > nit: I think it would be preferred to use an enum instead of a boolean for > `doEmitAwait`, but I dunno. See what proper reviewers think, I guess. As > with GetIterator, I'd also be ok with a separate function implementing that > algorithm (`emitAsyncIteratorClose` I guess). Up to you / other reviewers. Yeah, I'll check with reviewer Thanks for review! I'll try to fix soon Created attachment 316890 [details]
Patch
Fix latest comments
Comment on attachment 316652 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316652&action=review >>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4858 >>> + RefPtr<RegisterID> iterator = emitGetIteratorByCurrentFunctionType(argument); >> >> nit: Can we do something more like: >> >> ``` >> RefPtr<RegisterID> iterator = parseMode() == SourceParseMode::AsyncGeneratorBodyMode ? emitGetAsyncIterator(argument) : emitGetIterator(argument); >> ``` >> >> where `emitGetAsyncIterator` includes the creation of the async-from-sync iterator if the @@asyncIterator method isn't found? Then you could reuse it for for-await-of loops, once those are implemented. That's sort of what I was talking about --- either have 2 separate functions for the different GetIterator algorithms, or pass some kind of flag to the function as a parameter indicating how it should work. > > Oh OK. I see what you mean Fixed Created attachment 317254 [details]
Patch
Use useAsyncIterator option during prsing phase
Comment on attachment 317254 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317254&action=review Super nice! I would like to review the parser changes first. Could you spawn the parser part in a separate patch? > 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 `opcodeID == op_new_async_generator_func` case twice? > Source/JavaScriptCore/parser/Parser.cpp:529 > + if (isAsyncGeneratorFunctionParseMode(parseMode)) > + innerParseMode = SourceParseMode::AsyncGeneratorBodyMode; > + else > + innerParseMode = parseMode == SourceParseMode::AsyncArrowFunctionMode > ? SourceParseMode::AsyncArrowFunctionBodyMode > : SourceParseMode::AsyncFunctionBodyMode; Change it like if (isAsyncGeneratorFunctionParseMode(parseMode) innerParseMode = SourceParseMode::AsyncGeneratorBodyMode; else if (parseMode == SourceParseMode::AsyncArrowFunctionMode) innerParseMode = SourceParseMode::AsyncArrowFunctionBodyMode; else innerParseMode = SourceParseMode::AsyncFunctionBodyMode; > Source/JavaScriptCore/parser/Parser.cpp:2488 > + if (isAsyncGeneratorFunctionParseMode(mode)) > + innerParseMode = SourceParseMode::AsyncGeneratorBodyMode; > + else > + innerParseMode = mode == SourceParseMode::AsyncArrowFunctionMode > ? SourceParseMode::AsyncArrowFunctionBodyMode > : SourceParseMode::AsyncFunctionBodyMode; Ditto. Since this functionality is shown again, let's spawn this to static function. > Source/JavaScriptCore/parser/Parser.cpp:2821 > + if (match(OPENPAREN) || match(COLON) || match(EQUAL) || m_lexer->prevTerminator()) > + break; Nice. > Source/JavaScriptCore/parser/Parser.cpp:3927 > + failIfTrue(isGeneratorMethodParseMode(parseMode) || isAsyncMethodParseMode(parseMode), "Expected a parenthesis for argument list"); Let's use `parseMode != SourceParseMode::MethodMode` consistently. > Source/JavaScriptCore/runtime/AsyncGeneratorFunctionPrototype.h:33 > + using Bae = JSNonFinalObject; Typo, Base. > Source/JavaScriptCore/runtime/JSAsyncGeneratorFunction.cpp:40 > +const ClassInfo JSAsyncGeneratorFunction ::s_info = { "JSAsyncGeneratorFunction", &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(JSAsyncGeneratorFunction) }; Delete spaces between JSAsyncGeneratorFunction and ::s_info. > JSTests/ChangeLog:8 > + * stress/async-await-syntax.js: Bunch of tests! That's super awesome. Comment on attachment 317254 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317254&action=review This patch is huge. I've read most of it, but not all. Posting comments for now. r- for now because I found a couple bugs in builtins and Yusuke's comments. > Source/JavaScriptCore/ChangeLog:13 > + # await - wait until promise will be resolved and than continue than => then > Source/JavaScriptCore/ChangeLog:15 > + The main difference between async function and async generator that, generator that => generator is that > Source/JavaScriptCore/ChangeLog:26 > + # The behavior of yield* is modified to support > + delegation to async iterables only inside an async generator? > Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:41 > + function(result) { promiseCapability.@resolve.@call(@undefined, { done: !!done, value: result }); }, Is this !!done in the spec? I think it may be observable. > Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:61 > + promiseCapability.@resolve.@call(@undefined, {value: returnValue, done: true}); style nit: Your object literals are not consistent with having a space around the after/before "{" "}" or not. I'm not sure what our official style in JSC is, but we should probably pick something and try to stick with it. > Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:127 > + if (!@isObject(syncIterator)) @throwTypeError('Only objects can be wrapped by async-from-sync wrapper'); style nit: newline after the if(...) > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:37 > + return queue.shift(); this is wrong, you need to use private methods here otherwise you're using an untrusted "shift" method being on ArrayPrototype. > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:102 > + function asyncGeneratorYieldAwaited(result) > + { > + generator.@asyncGeneratorSuspendReason = @AsyncGeneratorSuspendReasonYield; > + @asyncGeneratorResolve(generator, result, false); > + } > + > + generator.@asyncGeneratorSuspendReason = @AsyncGeneratorSuspendReasonAwait; > + > + @awaitValue(generator, value, asyncGeneratorYieldAwaited); Why can't you just immediately resolve here? Won't we always have a regular object we need to resolve? Maybe I'm missing something. > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:114 > + const onRejected = onRejectedCallback || function(result) { @doAsyncGeneratorBodyCall(generator, result, @GeneratorResumeModeThrow); }; you only call this function with truthy values. Why the "||"? > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:137 > + if (generator.@generatorState === @AsyncGeneratorStateExecuting) { > + generator.@generatorState = @AsyncGeneratorStateCompleted; > + } style: no braces > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:155 > + if (generator.@asyncGeneratorSuspendReason === @AsyncGeneratorSuspendReasonYield) { > + return @asyncGeneratorYield(generator, value, resumeMode); > + } style: no braces > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:186 > + > + style: remove one line here please > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:211 > + } else if (state === @AsyncGeneratorStateCompleted) { > + return @asyncGeneratorResolve(generator, @undefined, true); > + } style: no braces > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:232 > + if (generator.@asyncGeneratorQueue === @undefined) > + generator.@asyncGeneratorQueue = []; Why not do this during construction? > Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:234 > + generator.@asyncGeneratorQueue.push({resumeMode, value, promiseCapability}); you need to use a private name here. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:64 > + enum EmitAwait { Yes, No }; style: enum class please > Source/JavaScriptCore/jit/JITOpcodes.cpp:1043 > + else if (opcodeID == op_new_async_generator_func_exp) > + callOperation(operationNewAsyncGeneratorFunction, dst, regT0, function); > + else { > + ASSERT(opcodeID == op_new_async_generator_func_exp); > + callOperation(operationNewAsyncGeneratorFunction, dst, regT0, function); Ditto to what Yusuke said here > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1185 > + dataLogF("Creating async generator function!\n"); style: de-indent 4 spaces > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1300 > + dispatch(4) should be: dispatch(constexpr op_new_asyc_generator_func_length) now that Keith made this possible > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1305 > + dispatch(4) ditto > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:683 > + m_asyncGeneratorFunctionPrototype->putDirectWithoutTransition(vm, vm.propertyNames->constructor, asyncGeneratorFunctionConstructor, DontEnum | ReadOnly); nit: This feels like it should be in AsyncGeneratorFunctionPrototype create. Comment on attachment 317254 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317254&action=review I've split this patches according to suggestion from reviewers. Parser part with fixed comments https://bugs.webkit.org/show_bug.cgi?id=175210 Runtime part will be uploaded after approve or land the parser part and will include all fixed comment for current patch >> Source/JavaScriptCore/parser/Parser.cpp:529 >> : SourceParseMode::AsyncFunctionBodyMode; > > Change it like > > if (isAsyncGeneratorFunctionParseMode(parseMode) > innerParseMode = SourceParseMode::AsyncGeneratorBodyMode; > else if (parseMode == SourceParseMode::AsyncArrowFunctionMode) > innerParseMode = SourceParseMode::AsyncArrowFunctionBodyMode; > else > innerParseMode = SourceParseMode::AsyncFunctionBodyMode; Done >> Source/JavaScriptCore/parser/Parser.cpp:2488 >> : SourceParseMode::AsyncFunctionBodyMode; > > Ditto. Since this functionality is shown again, let's spawn this to static function. Done >> Source/JavaScriptCore/parser/Parser.cpp:2821 >> + break; > > Nice. Oh, Thanks Katlyn Potter! She rewrote this part for me >> Source/JavaScriptCore/parser/Parser.cpp:3927 >> + failIfTrue(isGeneratorMethodParseMode(parseMode) || isAsyncMethodParseMode(parseMode), "Expected a parenthesis for argument list"); > > Let's use `parseMode != SourceParseMode::MethodMode` consistently. Done Created attachment 317344 [details]
Patch
Patch with runtime part with fixed comments
Comment on attachment 317254 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317254&action=review >> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:41 >> + function(result) { promiseCapability.@resolve.@call(@undefined, { done: !!done, value: result }); }, > > Is this !!done in the spec? I think it may be observable. Hmm, according to spec it is https://tc39.github.io/ecma262/#sec-iteratorcomplete toBoolean operation https://tc39.github.io/ecma262/#sec-toboolean. I thought it is the same as `!!`. should I replace by something? >> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:61 >> + promiseCapability.@resolve.@call(@undefined, {value: returnValue, done: true}); > > style nit: Your object literals are not consistent with having a space around the after/before "{" "}" or not. I'm not sure what our official style in JSC is, but we should probably pick something and try to stick with it. Done >> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:127 >> + if (!@isObject(syncIterator)) @throwTypeError('Only objects can be wrapped by async-from-sync wrapper'); > > style nit: newline after the if(...) Done >> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:37 >> + return queue.shift(); > > this is wrong, you need to use private methods here otherwise you're using an untrusted "shift" method being on ArrayPrototype. Done >> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:102 >> + @awaitValue(generator, value, asyncGeneratorYieldAwaited); > > Why can't you just immediately resolve here? Won't we always have a regular object we need to resolve? Maybe I'm missing something. As I remember it is to make hidden await in yield i.e.: ``` let resolver; async function * foo() { yield 1;//step 1 yield new Promise(resolve=>{ resolver = resolve;});// step 2 yield 3;//step 3 } const iter = foo(); iter.next().then(v=>print(v.value, v.done)); iter.next().then(v=>print(v.value, v.done)); iter.next().then(v=>print(v.value, v.done)); drainMicrotasks(); resolver(2); ``` Without this step we move father promise to step 3, and don't wait until promise in step 2 will be resolved >> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:114 >> + const onRejected = onRejectedCallback || function(result) { @doAsyncGeneratorBodyCall(generator, result, @GeneratorResumeModeThrow); }; > > you only call this function with truthy values. Why the "||"? Fixed >> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:137 >> + } > > style: no braces Done >> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:155 >> + } > > style: no braces Done >> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:186 >> + > > style: remove one line here please Done >> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:211 >> + } > > style: no braces Done >> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:232 >> + generator.@asyncGeneratorQueue = []; > > Why not do this during construction? Ohh, Not sure that a I know how to set empty array within Bytecode >> Source/JavaScriptCore/builtins/AsyncGeneratorPrototype.js:234 >> + generator.@asyncGeneratorQueue.push({resumeMode, value, promiseCapability}); > > you need to use a private name here. Dome |
Implement async generator that support following function async function *foo() { let t = await boo('async iteration '); yield t + 'step-1'; yield t + 'step-2'; } var f = foo(); f.next().then(({value, done}) => debug(value)); f.next().then(({value, done}) => debug(value));