Bug 201159 - [JSC] Generator should have internal fields
Summary: [JSC] Generator should have internal fields
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
: 151545 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-08-26 16:23 PDT by Yusuke Suzuki
Modified: 2019-09-17 22:02 PDT (History)
9 users (show)

See Also:


Attachments
Patch (62.85 KB, patch)
2019-09-05 02:39 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (86.55 KB, patch)
2019-09-05 03:08 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (98.39 KB, patch)
2019-09-06 20:08 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (99.65 KB, patch)
2019-09-07 01:47 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (128.77 KB, patch)
2019-09-09 00:43 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (99.65 KB, patch)
2019-09-09 00:45 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (102.33 KB, patch)
2019-09-09 02:19 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (103.09 KB, patch)
2019-09-09 15:48 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (121.35 KB, patch)
2019-09-16 03:42 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (122.34 KB, patch)
2019-09-16 11:12 PDT, Yusuke Suzuki
keith_miller: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2019-08-26 16:23:53 PDT
...
Comment 1 Yusuke Suzuki 2019-08-26 16:24:10 PDT
And they should have `create_generator` / `new_generator` etc.
Comment 2 Radar WebKit Bug Importer 2019-08-26 22:50:24 PDT
<rdar://problem/54736322>
Comment 3 Yusuke Suzuki 2019-09-05 00:24:15 PDT
Separate async-generator and generator.
Comment 4 Yusuke Suzuki 2019-09-05 02:39:20 PDT
Created attachment 378069 [details]
Patch
Comment 5 Yusuke Suzuki 2019-09-05 03:08:16 PDT
Created attachment 378071 [details]
Patch
Comment 6 Yusuke Suzuki 2019-09-05 13:00:13 PDT
The patch is getting larger and larger, I'll split this to InternalField access renaming patch first to make it small.
Comment 7 Yusuke Suzuki 2019-09-06 14:41:03 PDT
It turned out that only changing Generator causes super complicated code. We should change generator and async-generator at once.
Comment 8 Yusuke Suzuki 2019-09-06 19:56:53 PDT
AsyncGenerator thing makes the patch too larger. Do generator thing first.
Comment 9 Yusuke Suzuki 2019-09-06 20:08:41 PDT
Created attachment 378267 [details]
Patch
Comment 10 Build Bot 2019-09-06 22:29:58 PDT
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
Comment 11 Yusuke Suzuki 2019-09-07 01:47:16 PDT
Created attachment 378286 [details]
Patch
Comment 12 Build Bot 2019-09-07 04:09:31 PDT
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
Comment 13 Yusuke Suzuki 2019-09-09 00:43:31 PDT
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
Comment 14 Yusuke Suzuki 2019-09-09 00:45:52 PDT
Created attachment 378354 [details]
Patch

JSGenerator without PolyProto support. I would like to go with this patch first
Comment 15 Yusuke Suzuki 2019-09-09 02:19:02 PDT
Created attachment 378357 [details]
Patch
Comment 16 Build Bot 2019-09-09 04:40:51 PDT
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
Comment 17 Yusuke Suzuki 2019-09-09 15:48:54 PDT
Created attachment 378412 [details]
Patch
Comment 18 Yusuke Suzuki 2019-09-09 16:43:56 PDT
*** Bug 151545 has been marked as a duplicate of this bug. ***
Comment 19 Yusuke Suzuki 2019-09-16 03:42:12 PDT
Created attachment 378852 [details]
Patch
Comment 20 Yusuke Suzuki 2019-09-16 11:12:35 PDT
Created attachment 378875 [details]
Patch
Comment 21 Keith Miller 2019-09-17 12:03:24 PDT
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.
Comment 22 Ross Kirsling 2019-09-17 14:07:39 PDT
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.
Comment 23 Yusuke Suzuki 2019-09-17 19:56:05 PDT
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.
Comment 24 Yusuke Suzuki 2019-09-17 19:57:54 PDT
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.
Comment 25 Yusuke Suzuki 2019-09-17 22:02:51 PDT
Committed r250025: <https://trac.webkit.org/changeset/250025>