[JSC] Generator should have internal fields

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

...

[Yusuke Suzuki]

And they should have `create_generator` / `new_generator` etc.

[Radar WebKit Bug Importer]

<rdar://problem/54736322>

[Yusuke Suzuki]

Separate async-generator and generator.

[Yusuke Suzuki]

Created attachment 378069 [details]
Patch

[Yusuke Suzuki]

Created attachment 378071 [details]
Patch

[Yusuke Suzuki]

The patch is getting larger and larger, I'll split this to InternalField access renaming patch first to make it small.

[Yusuke Suzuki]

It turned out that only changing Generator causes super complicated code. We should change generator and async-generator at once.

[Yusuke Suzuki]

AsyncGenerator thing makes the patch too larger. Do generator thing first.

[Yusuke Suzuki]

Created attachment 378267 [details]
Patch

[EWS Watchlist]

Comment on attachment 378267 [details]
Patch

Attachment 378267 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13008638

New failing tests:
stress/object-assign-fast-path.js.no-cjit-collect-continuously
stress/async-arrow-functions-lexical-super-binding.js.ftl-eager
stress/async-arrow-functions-lexical-super-binding.js.dfg-eager-no-cjit-validate
stress/async-arrow-functions-lexical-super-binding.js.ftl-eager-no-cjit
stress/async-arrow-functions-lexical-super-binding.js.bytecode-cache
stress/async-arrow-functions-lexical-super-binding.js.mini-mode
stress/object-assign-fast-path.js.bytecode-cache
stress/test-out-of-memory.js.ftl-eager-no-cjit
stress/async-arrow-functions-lexical-super-binding.js.ftl-no-cjit-b3o0
stress/async-arrow-functions-lexical-super-binding.js.no-cjit-collect-continuously
stress/async-arrow-functions-lexical-super-binding.js.no-cjit-validate-phases
stress/object-assign-fast-path.js.eager-jettison-no-cjit
stress/test-out-of-memory.js.no-cjit-collect-continuously
stress/object-assign-fast-path.js.ftl-eager-no-cjit-b3o1
stress/async-arrow-functions-lexical-binding-in-class.js.mini-mode
stress/async-arrow-functions-lexical-binding-in-class.js.ftl-no-cjit-validate-sampling-profiler
stress/object-assign-fast-path.js.no-ftl
stress/async-arrow-functions-lexical-super-binding.js.no-ftl
stress/object-assign-fast-path.js.dfg-eager-no-cjit-validate
stress/async-arrow-functions-lexical-binding-in-class.js.no-cjit-collect-continuously
stress/object-assign-fast-path.js.ftl-no-cjit-b3o0
stress/async-arrow-functions-lexical-binding-in-class.js.bytecode-cache
stress/object-assign-fast-path.js.ftl-no-cjit-no-inline-validate
stress/object-assign-fast-path.js.ftl-no-cjit-validate-sampling-profiler
stress/async-arrow-functions-lexical-binding-in-class.js.ftl-no-cjit-b3o0
stress/async-arrow-functions-lexical-binding-in-class.js.ftl-no-cjit-no-put-stack-validate
stress/object-assign-fast-path.js.mini-mode
stress/async-arrow-functions-lexical-super-binding.js.ftl-no-cjit-no-inline-validate
stress/object-assign-fast-path.js.ftl-eager-no-cjit
stress/object-assign-fast-path.js.ftl-no-cjit-small-pool
stress/async-arrow-functions-lexical-super-binding.js.default
stress/async-arrow-functions-lexical-super-binding.js.dfg-eager
stress/async-arrow-functions-lexical-binding-in-class.js.ftl-no-cjit-small-pool
stress/async-arrow-functions-lexical-binding-in-class.js.eager-jettison-no-cjit
stress/async-arrow-functions-lexical-super-binding.js.ftl-no-cjit-small-pool
stress/async-arrow-functions-lexical-super-binding.js.ftl-eager-no-cjit-b3o1
stress/object-assign-fast-path.js.ftl-eager
stress/async-arrow-functions-lexical-super-binding.js.no-llint
stress/object-assign-fast-path.js.dfg-eager
stress/async-arrow-functions-lexical-binding-in-class.js.ftl-no-cjit-no-inline-validate
stress/async-arrow-functions-lexical-binding-in-class.js.no-ftl
stress/object-assign-fast-path.js.ftl-no-cjit-no-put-stack-validate
stress/async-arrow-functions-lexical-binding-in-class.js.ftl-eager-no-cjit-b3o1
stress/object-assign-fast-path.js.no-cjit-validate-phases
stress/async-arrow-functions-lexical-super-binding.js.ftl-no-cjit-no-put-stack-validate
stress/async-arrow-functions-lexical-binding-in-class.js.no-cjit-validate-phases
stress/test-out-of-memory.js.ftl-eager
stress/async-arrow-functions-lexical-super-binding.js.ftl-no-cjit-validate-sampling-profiler
stress/object-assign-fast-path.js.default
stress/object-assign-fast-path.js.no-llint
stress/async-arrow-functions-lexical-super-binding.js.eager-jettison-no-cjit
stress/async-arrow-functions-lexical-binding-in-class.js.default
stress/async-arrow-functions-lexical-binding-in-class.js.no-llint
stress/test-out-of-memory.js.dfg-eager

[Yusuke Suzuki]

Created attachment 378286 [details]
Patch

[EWS Watchlist]

Comment on attachment 378286 [details]
Patch

Attachment 378286 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13009592

New failing tests:
stress/object-assign-fast-path.js.no-cjit-collect-continuously
stress/object-assign-fast-path.js.no-ftl
stress/object-assign-fast-path.js.bytecode-cache
stress/disable-gigacage-strings.js.disable-gigacage
stress/object-assign-fast-path.js.eager-jettison-no-cjit
stress/test-out-of-memory.js.no-cjit-collect-continuously
stress/object-assign-fast-path.js.ftl-eager-no-cjit-b3o1
stress/object-assign-fast-path.js.dfg-eager-no-cjit-validate
stress/object-assign-fast-path.js.ftl-no-cjit-b3o0
stress/object-assign-fast-path.js.ftl-no-cjit-no-inline-validate
stress/object-assign-fast-path.js.ftl-no-cjit-validate-sampling-profiler
stress/object-assign-fast-path.js.mini-mode
stress/object-assign-fast-path.js.ftl-eager-no-cjit
stress/disable-gigacage-typed-arrays.js.disable-gigacage
stress/object-assign-fast-path.js.ftl-no-cjit-small-pool
stress/disable-gigacage-arrays.js.disable-gigacage
stress/test-out-of-memory.js.dfg-eager-no-cjit-validate
stress/test-out-of-memory.js.ftl-eager-no-cjit
stress/object-assign-fast-path.js.ftl-eager
stress/object-assign-fast-path.js.dfg-eager
stress/object-assign-fast-path.js.ftl-no-cjit-no-put-stack-validate
stress/object-assign-fast-path.js.no-cjit-validate-phases
stress/test-out-of-memory.js.ftl-eager
stress/object-assign-fast-path.js.default
stress/object-assign-fast-path.js.no-llint

[Yusuke Suzuki]

Created attachment 378353 [details]
Patch

JSGenerator + PolyProto support, but I'll split PolyProto support from this patch later. But this support is necessary later since generator tends to have poly proto

[Yusuke Suzuki]

Created attachment 378354 [details]
Patch

JSGenerator without PolyProto support. I would like to go with this patch first

[Yusuke Suzuki]

Created attachment 378357 [details]
Patch

[EWS Watchlist]

Comment on attachment 378357 [details]
Patch

Attachment 378357 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13016386

New failing tests:
stress/object-assign-fast-path.js.no-cjit-collect-continuously
stress/object-assign-fast-path.js.ftl-eager-no-cjit
stress/object-assign-fast-path.js.ftl-no-cjit-small-pool
stress/object-assign-fast-path.js.no-ftl
stress/object-assign-fast-path.js.ftl-no-cjit-no-put-stack-validate
stress/object-assign-fast-path.js.eager-jettison-no-cjit
stress/object-assign-fast-path.js.bytecode-cache
stress/object-assign-fast-path.js.dfg-eager-no-cjit-validate
stress/object-assign-fast-path.js.no-cjit-validate-phases
stress/object-assign-fast-path.js.dfg-eager
stress/object-assign-fast-path.js.ftl-no-cjit-b3o0
stress/object-assign-fast-path.js.default
stress/object-assign-fast-path.js.ftl-no-cjit-validate-sampling-profiler
stress/object-assign-fast-path.js.ftl-no-cjit-no-inline-validate
stress/object-assign-fast-path.js.ftl-eager
stress/object-assign-fast-path.js.no-llint
stress/object-assign-fast-path.js.ftl-eager-no-cjit-b3o1
stress/object-assign-fast-path.js.mini-mode

[Yusuke Suzuki]

Created attachment 378412 [details]
Patch

[Yusuke Suzuki]

*** Bug 151545 has been marked as a duplicate of this bug. ***

[Yusuke Suzuki]

Created attachment 378852 [details]
Patch

[Yusuke Suzuki]

Created attachment 378875 [details]
Patch

[Keith Miller]

Comment on attachment 378875 [details]
Patch

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

r=me with comments.

> Source/JavaScriptCore/ChangeLog:14
> +        If we make these structures adaptively poly-proto ones, some generators get poly-proto structures while others are not, resulting
> +        in megamorphic lookup in Generator.prototype.next. If we make all the generator's structure poly-proto ones, it makes Generator.prototype.next
> +        lookup suboptimal for now.

Nit: poly-proto ones -> poly-proto. 

while others do not,

> Source/JavaScriptCore/ChangeLog:18
> +        In this patch, we start with relatively simple way. This patch introduces JSGenerator class, and it has internal fields for generator's internal
> +        states. We extend promise-internal-field access bytecodes to access to these fields from bytecode so that Generator.prototype.next can access
> +        these fields without using megamorphic get_by_id_direct.

Nit: start with relatively simple way => start with a relatively simple solution

> Source/JavaScriptCore/ChangeLog:20
> +        And we can attach JSGeneratorType to JSGenerator so that we can efficiently implement `@isGenerator()` check in bytecode.

Nit: we can attach => we attach

> Source/JavaScriptCore/ChangeLog:27
> +        Currently, we start having op_create_generator since it could be different from create_promise when we add PolyProto support. But when adding create_async_generator-like
> +        thing, we will introduce one abstracted bytecode for both create_generator and create_async_generator to efficiently support both.

Nit: I would probably phrase this as This patch adds op_create_generator since it is distinct from op_create_promise once we add PolyProto support. In the future when we introduce some kind of op_create_async_generator we will probably share only one bytecode for both generator and async generator.

> Source/JavaScriptCore/bytecode/BytecodeIntrinsicRegistry.cpp:80
> +    m_generatorFieldState.set(m_vm, jsNumber(static_cast<unsigned>(JSGenerator::Field::State)));
> +    m_generatorFieldNext.set(m_vm, jsNumber(static_cast<unsigned>(JSGenerator::Field::Next)));
> +    m_generatorFieldThis.set(m_vm, jsNumber(static_cast<unsigned>(JSGenerator::Field::This)));
> +    m_generatorFieldFrame.set(m_vm, jsNumber(static_cast<unsigned>(JSGenerator::Field::Frame)));

Hmm, it feels like we should simplify this because we have so many of these now... I wish we could combine all the offset registries (FTLOffset, LLInt offsets, and this) into one class...

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4989
> +                            addToGraph(Phantom, callee);

Doesn't this need to be after the NewGenerator? If we exit in NewGenerator then we won't be able to recover callee correct? Callee's Phantom has already passed.

[Ross Kirsling]

Comment on attachment 378875 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSGenerator.cpp:32
> +#include "BuiltinNames.h"
> +#include "Error.h"
> +#include "JSCInlines.h"
> +#include "JSFunction.h"

This compiles without any of these four (though JSCInlines should still be required for non-unified, I believe).

> Source/JavaScriptCore/runtime/JSGenerator.h:32
> +class JSFunction;

This seems to be unused.

[Yusuke Suzuki]

Comment on attachment 378875 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/ChangeLog:14
>> +        lookup suboptimal for now.
> 
> Nit: poly-proto ones -> poly-proto. 
> 
> while others do not,

Fixed.

>> Source/JavaScriptCore/ChangeLog:18
>> +        these fields without using megamorphic get_by_id_direct.
> 
> Nit: start with relatively simple way => start with a relatively simple solution

Fixed.

>> Source/JavaScriptCore/ChangeLog:20
>> +        And we can attach JSGeneratorType to JSGenerator so that we can efficiently implement `@isGenerator()` check in bytecode.
> 
> Nit: we can attach => we attach

Fixed.

>> Source/JavaScriptCore/ChangeLog:27
>> +        thing, we will introduce one abstracted bytecode for both create_generator and create_async_generator to efficiently support both.
> 
> Nit: I would probably phrase this as This patch adds op_create_generator since it is distinct from op_create_promise once we add PolyProto support. In the future when we introduce some kind of op_create_async_generator we will probably share only one bytecode for both generator and async generator.

Fixed.

>> Source/JavaScriptCore/bytecode/BytecodeIntrinsicRegistry.cpp:80
>> +    m_generatorFieldFrame.set(m_vm, jsNumber(static_cast<unsigned>(JSGenerator::Field::Frame)));
> 
> Hmm, it feels like we should simplify this because we have so many of these now... I wish we could combine all the offset registries (FTLOffset, LLInt offsets, and this) into one class...

Sounds nice. I'll open a bug for now.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4989
>> +                            addToGraph(Phantom, callee);
> 
> Doesn't this need to be after the NewGenerator? If we exit in NewGenerator then we won't be able to recover callee correct? Callee's Phantom has already passed.

Fixed, and do the same thing for CreateThis etc.

[Yusuke Suzuki]

Comment on attachment 378875 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSGenerator.cpp:32
>> +#include "JSFunction.h"
> 
> This compiles without any of these four (though JSCInlines should still be required for non-unified, I believe).

Fixed.

>> Source/JavaScriptCore/runtime/JSGenerator.h:32
>> +class JSFunction;
> 
> This seems to be unused.

Removed.

[Yusuke Suzuki]

Committed r250025: <https://trac.webkit.org/changeset/250025>