[Webkit-unassigned] [Bug 166695] [ESNext] Async iteration - Implement Async Generator

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=166695

--- Comment #59 from Caitlin Potter (:caitp) <caitp at igalia.com> ---
Comment on attachment 307013
  --> https://bugs.webkit.org/attachment.cgi?id=307013
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 `*`"

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170418/1c27c9d5/attachment-0001.html>