Bug 174212

Summary: [JSC] Add support for instance class fields
Product: WebKit Reporter: Xan Lopez <xan.lopez>
Component: JavaScriptCoreAssignee: Caitlin Potter (:caitp) <caitp>
Status: ASSIGNED ---    
Severity: Normal CC: bugs-noreply, caitp, calvaris, chi187, dinfuehr, ews, fred.wang, guijemont, guijemont+jsc-armv7-ews, keith_miller, littledan, mark.lam, mcatanzaro, mjs, msaboff, rniwa, sbarati, ticaiolima, xan.lopez, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=194733
Bug Depends on:    
Bug Blocks: 194095, 194434, 194435, 195619    
Attachments:
Description Flags
WIP data field parsing
none
Data fields WIP
none
Patch
none
Instance class fields
ews: commit-queue-
Archive of layout-test-results from ews201 for win-future
none
instance class fields (09/05/18)
ews: commit-queue-
Archive of layout-test-results from ews201 for win-future
none
Class fields WIP
none
Class fields WIP
ews: commit-queue-
Archive of layout-test-results from ews102 for mac-sierra
none
Archive of layout-test-results from ews106 for mac-sierra-wk2
none
Archive of layout-test-results from ews112 for mac-sierra
none
Archive of layout-test-results from ews202 for win-future
none
Archive of layout-test-results from ews113 for mac-sierra
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Class fields (10/01)
ews: commit-queue-
Archive of layout-test-results from ews107 for mac-sierra-wk2
none
Class fields WIP (10/02)
none
Class fields, parser + bytecode + test changes
none
Class fields, parser + bytecode + test changes
none
Class fields, parser + bytecode + test changes
none
Class fields, parser + bytecode + test changes
none
Class fields, parser + bytecode + test changes
none
Class fields, parser + bytecode + test changes
none
Class fields, parser + bytecode + test changes
none
Placeholder baseline JIT for private field opcodes
none
Placeholder baseline JIT for private field opcodes
none
Class fields, parser + bytecode + test changes
sbarati: review-
Class fields, parser + bytecode + test changes
none
Class fields, parser + byte code + test changes
ews: commit-queue-
Class fields, parser + bytecode + test changes
none
Class fields, parser + bytecode + test changes
ews: commit-queue-
Archive of layout-test-results from ews210 for win-future
none
Class fields, parser + bytecode + test changes
ews: commit-queue-
Archive of layout-test-results from ews101 for mac-highsierra
none
Archive of layout-test-results from ews107 for mac-highsierra-wk2
none
Archive of layout-test-results from ews112 for mac-highsierra
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Class fields, parser + bytecode + test changes
ews: commit-queue-
Archive of layout-test-results from ews100 for mac-highsierra
none
Archive of layout-test-results from ews106 for mac-highsierra-wk2
none
Archive of layout-test-results from ews117 for mac-highsierra
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Archive of layout-test-results from ews213 for win-future
none
Class fields, parser + bytecode + test changes
none
Class fields, parser + bytecode + test changes
none
Class fields, parser + bytecode + test changes
ticaiolima: review?
Archive of layout-test-results from ews214 for win-future none

Description Xan Lopez 2017-07-06 11:19:19 PDT
Created attachment 314736 [details]
WIP data field parsing

Ok, a really simple/rough WIP, but I think enough to ask a couple questions. I had a larger patch with a new parseDataField() method (lots of copy/paste) that I'd run in a loop at the begining of parseClass() until it stopped parsing fields, but then realized data fields can be mixed with methods in any order, so had to scrap that. Now I just branch after parsing an identifier to check if it's a method or a field. Issues:

1) ASI is not done. Actually not quite sure how to do that, probably I'm missing something trivial.
2) I'm just using as-is the code to parse assignments. Probably needs tweaking.
3) I'm not generating any code.

My idea for the first patch would be to figure out 1) and 2) and to pass the stuff in https://github.com/tc39/test262/pull/1064
I tried a bunch of examples and they all work, including new ones like class { field; method; field; etc }
Comment 1 Caitlin Potter (:caitp) 2017-07-06 14:14:28 PDT
Comment on attachment 314736 [details]
WIP data field parsing

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

just some ideas (also, it would be good to test your implementation's interaction with the contextual keywords, the generator `*` thing, etc.

> Source/JavaScriptCore/parser/Parser.cpp:2725
>              next();

What about something like:

```
next();
if ({classFieldsEnabled} && !isGenerator && !isAsyncMethod) {
  TreeExpression initializer = 0;
  if (consume(EQUAL))
      initializer = parseAssignmentExpression(context);
  consumeOrFail(SEMICOLON);  // I think class fields require the `;` at the end, per https://tc39.github.io/proposal-class-fields/#sec-updated-syntax
  // TODO: add to the AST node, keeping track of whether the field is static or not, and its initializer.

  continue; // parse the next class element
}
```

Then we could avoid having that stuff in parseMethod where it's accessible through more paths.
Comment 2 Caitlin Potter (:caitp) 2017-07-06 14:18:00 PDT
Comment on attachment 314736 [details]
WIP data field parsing

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

>> Source/JavaScriptCore/parser/Parser.cpp:2725
>>              next();
> 
> What about something like:
> 
> ```
> next();
> if ({classFieldsEnabled} && !isGenerator && !isAsyncMethod) {
>   TreeExpression initializer = 0;
>   if (consume(EQUAL))
>       initializer = parseAssignmentExpression(context);
>   consumeOrFail(SEMICOLON);  // I think class fields require the `;` at the end, per https://tc39.github.io/proposal-class-fields/#sec-updated-syntax
>   // TODO: add to the AST node, keeping track of whether the field is static or not, and its initializer.
> 
>   continue; // parse the next class element
> }
> ```
> 
> Then we could avoid having that stuff in parseMethod where it's accessible through more paths.

well, this isn't quite right I guess, you still need the `(match(EQUAL) || match(SEMICOLON))` thing in the if statement. Should work with that though
Comment 3 Saam Barati 2017-07-06 14:18:13 PDT
(In reply to Xan Lopez from comment #0)
> Created attachment 314736 [details]
> WIP data field parsing
> 
> Ok, a really simple/rough WIP, but I think enough to ask a couple questions.
> I had a larger patch with a new parseDataField() method (lots of copy/paste)
> that I'd run in a loop at the begining of parseClass() until it stopped
> parsing fields, but then realized data fields can be mixed with methods in
> any order, so had to scrap that. Now I just branch after parsing an
> identifier to check if it's a method or a field. Issues:
> 
> 1) ASI is not done. Actually not quite sure how to do that, probably I'm
> missing something trivial.
What's ASI? Do you mean AST? If so, you'll need to hook into SyntaxChecker and ASTBuilder. The SyntaxChecker is used when JSC does a quick pass over the source code to ensure no syntax errors. ASTBuilder is used to actually build and AST. We templatize the parser over the type of TreeBuilder. This means that SyntaxChecker and ASTBuilder need to provide the same API. This often means just filling in empty stubs inside SyntaxChecker. To add AST nodes, you'll need to modify parser/Nodes.h. You'll need to add a constructor, methods that you need, etc. Then, you can hook up the ASTBuilder to call into the API you define as needed. You should be able to find other examples of this by reading code inside ASTBuilder and Parser.cpp. You'll probably need to modify class declaration/expr nodes to know about their properties. Then, when generating code for these, you'll need to hook into the byte code generator, inside bytecompiler/BytecodeGenerator.* and bytecompiler/NodesCodeGen.cpp

> 2) I'm just using as-is the code to parse assignments. Probably needs
> tweaking.
It depends what the grammar is. What does the grammar say here?

> 3) I'm not generating any code.
See above.

> 
> My idea for the first patch would be to figure out 1) and 2) and to pass the
> stuff in https://github.com/tc39/test262/pull/1064
> I tried a bunch of examples and they all work, including new ones like class
> { field; method; field; etc }
We'll need 1, 2, and 3 to land this. We need to actually generate and execute code correctly.
Comment 4 Yusuke Suzuki 2017-07-06 22:02:38 PDT
I guess ASI is Automatic Semicolon Insertion.
Comment 5 Xan Lopez 2017-08-02 11:01:30 PDT
OK, some updates.

The issue with caitp's suggestions about the parser is that they would not support STRING idents, so we still need to do something similar to what my patch does (all PropertyNames per the spec must be support, including StringLiteral).

About:

> > 2) I'm just using as-is the code to parse assignments. Probably needs
> tweaking.

> It depends what the grammar is. What does the grammar say here?

The grammar says this is just an AssignmentExpression, so I think we are fine.

About code generation:

After exploring the code I bit it seemed to me I could just add a new PropertyNode to the PropertyNodeList with the result of the assignment parsing as the second parameter. Something like:

context.createProperty(ident, initializer, PropertyNode::Constant, PropertyNode::Unknown, etc);

This seems to actually work. Of course we'd still need to figure out private and static fields, but this seems good enough to get started. Is there any obvious reason to not use PropertyNodes for data fields?

Related to that, I saw https://bugs.webkit.org/show_bug.cgi?id=174935, which seems to suggest Yuzuke is also thinking of using properties to do this if I read it right.
Comment 6 Yusuke Suzuki 2017-08-02 19:18:25 PDT
(In reply to Xan Lopez from comment #5)
> OK, some updates.
> 
> The issue with caitp's suggestions about the parser is that they would not
> support STRING idents, so we still need to do something similar to what my
> patch does (all PropertyNames per the spec must be support, including
> StringLiteral).
> 
> About:
> 
> > > 2) I'm just using as-is the code to parse assignments. Probably needs
> > tweaking.
> 
> > It depends what the grammar is. What does the grammar say here?
> 
> The grammar says this is just an AssignmentExpression, so I think we are
> fine.
> 
> About code generation:
> 
> After exploring the code I bit it seemed to me I could just add a new
> PropertyNode to the PropertyNodeList with the result of the assignment
> parsing as the second parameter. Something like:
> 
> context.createProperty(ident, initializer, PropertyNode::Constant,
> PropertyNode::Unknown, etc);
> 
> This seems to actually work. Of course we'd still need to figure out private
> and static fields, but this seems good enough to get started. Is there any
> obvious reason to not use PropertyNodes for data fields?
> 
> Related to that, I saw https://bugs.webkit.org/show_bug.cgi?id=174935, which
> seems to suggest Yuzuke is also thinking of using properties to do this if I
> read it right.

Let's check and follow what the actual spec says. What the attribute of the class field property?
I just introduced private symbols to implement private fields super efficiently.
But I don't investigate the spec super deeply yet :P
Comment 7 Xan Lopez 2017-08-15 06:57:40 PDT
(In reply to Yusuke Suzuki from comment #6)
> 
> Let's check and follow what the actual spec says. What the attribute of the
> class field property?
> I just introduced private symbols to implement private fields super
> efficiently.
> But I don't investigate the spec super deeply yet :P

(Sorry for taking some days to reply, I was on holidays last week)

Using https://tc39.github.io/proposal-class-fields/#prod-FieldDefinition and https://tc39.github.io/ecma262/#sec-method-definitions as references.

For the attributes:

Both methods and fields are Configurable (the latest spec for fields says they are not Configurable, but it's a mistake, see https://github.com/tc39/proposal-class-fields/issues/35) and Writable. Fields Enumerable and Methods may or may not be. So I think we should be OK on that front.

Also, 2.7 in the fields spec says we should use DefinePropertyOrThrow too, so there's at least some hint that using the Property machinery might be doable. One  obvious roadblock is that instance fields have to be initialized in [[Construct]] (for base classes) or in SuperCall for subclasses, so I guess we would need extra code to do that properly for fields.
Comment 8 Xan Lopez 2017-08-15 09:34:55 PDT
(In reply to Xan Lopez from comment #7)
> Also, 2.7 in the fields spec says we should use DefinePropertyOrThrow too,
> so there's at least some hint that using the Property machinery might be
> doable. One  obvious roadblock is that instance fields have to be
> initialized in [[Construct]] (for base classes) or in SuperCall for
> subclasses, so I guess we would need extra code to do that properly for
> fields.

Been reading the spec a bit more and I think this sort of also applies for static fields. They pretty much use the same mechanisms to be defined, only difference being the target object and when you initialize things (late in ClassDefinitionEvaluation for static, [[Constructor]]/SuperCall for instance). So again it seems we could try to change the createProperty stuff enough to accommodate these cases and it would cover both instance and static.
Comment 9 Xan Lopez 2017-11-20 05:55:58 PST
Created attachment 327359 [details]
Data fields WIP

Just for the sake of keeping track, another version of the patch. This one does more stuff, and among other things I think it deals properly with the issue of initializing field properties with the 'undefined' value (unlike the previous patch).

My main doubt now is how to pass the instance field information from the Node's code generation classes to the Bytecode generator (first FIXME in the patch), so that they can be initialized at the right time (see InitializeInstanceFields vs. InitializeStaticFields in the spec). I'll keep working on this, but suggestions are welcome.
Comment 10 Caitlin Potter (:caitp) 2018-03-16 12:25:53 PDT
Created attachment 335960 [details]
Patch
Comment 11 Build Bot 2018-03-16 12:28:27 PDT
Attachment 335960 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:649:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Caitlin Potter (:caitp) 2018-03-16 12:36:20 PDT
I've updated the previous patch over top of the recent fixes from bugs.webkit.org/show_bug.cgi?id=183523, and added handling for ASI in the parser.

I've pinged Yusuke about solving the runtime side of things better, but don't believe I've heard back from him yet.

There are two ideas I have in mind:

1) put instance-field initialization in a synthetic Function node, whose bytecode is generated while the ClassExprNode's AST is still alive (so no re-parsing necessary) --- I believe this requires some hacks to the way UnlinkedFunctionCodeBlock works).

2) add a new ParseMode for re-parsing the ClassExpression, and create an instance-field initializer function by producing a function with that ParseMode, whose source is the entire class literal (this would require a separate version of parseClass to generate the correct AST during reparsing).

both of these approaches have some other problems that need to be addressed. For computed fields and private name fields, this also requires storing the generated private symbols and names computed during ClassExprNode::emitBytecode() somewhere. v8 uses internal/synthetic Variables, but it looks like JSC's scoping system is a bit different, and I'm not sure I can use non-unique names for synthetic variables.
Comment 13 Caitlin Potter (:caitp) 2018-03-16 12:43:36 PDT
(In reply to Caitlin Potter (:caitp) from comment #12)
> I've updated the previous patch over top of the recent fixes from
> bugs.webkit.org/show_bug.cgi?id=183523, and added handling for ASI in the
> parser.
> 
> I've pinged Yusuke about solving the runtime side of things better, but
> don't believe I've heard back from him yet.
> 
> There are two ideas I have in mind:
> 
> 1) put instance-field initialization in a synthetic Function node, whose
> bytecode is generated while the ClassExprNode's AST is still alive (so no
> re-parsing necessary) --- I believe this requires some hacks to the way
> UnlinkedFunctionCodeBlock works).
> 
> 2) add a new ParseMode for re-parsing the ClassExpression, and create an
> instance-field initializer function by producing a function with that
> ParseMode, whose source is the entire class literal (this would require a
> separate version of parseClass to generate the correct AST during reparsing).
> 
> both of these approaches have some other problems that need to be addressed.
> For computed fields and private name fields, this also requires storing the
> generated private symbols and names computed during
> ClassExprNode::emitBytecode() somewhere. v8 uses internal/synthetic
> Variables, but it looks like JSC's scoping system is a bit different, and
> I'm not sure I can use non-unique names for synthetic variables.

Oh, the _other_ other idea was, during constructor code generation, if the class is flagged as having instance fields, re-parse the class again and generate instance field initialization inline in bytecode.
Comment 14 Caitlin Potter (:caitp) 2018-04-11 10:01:03 PDT
Comment on attachment 335960 [details]
Patch

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

> Source/JavaScriptCore/parser/Parser.h:1630
> +    bool autoSemiColonAfterClassField()

it turns out, this is wrong, and the proposal doesn't prescribe new ASI requirements, but instead just outlines the consequence of omitting semicolons in certain cases. delete this and allowAutomaticSemicolonAfterClassField(), replace with autoSemiColon()
Comment 15 Caitlin Potter (:caitp) 2018-04-23 10:02:54 PDT
so, my local prototype for this feature is pretty much ready (it should pass most of the test262 coverage, there are a few things that don't work yet, and room for improvement in parts of it).

it's currently split up into 5 commits, the first one based on Xan's original patch.
Comment 16 Xan Lopez 2018-07-03 02:50:53 PDT
Created attachment 344172 [details]
Instance class fields

A new patch with the work done since May. We now pass most of the in-tree class-fields-public tests, and support both public and private instance fields. See the patch message for remaining TODOs and FIXMEs.
Comment 17 Build Bot 2018-07-03 02:54:42 PDT
Attachment 344172 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:1272:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2185:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:587:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 3 in 64 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Build Bot 2018-07-03 04:53:51 PDT
Comment on attachment 344172 [details]
Instance class fields

Attachment 344172 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8422699

New failing tests:
js/Object-getOwnPropertyNames.html
js/class-syntax-semicolon.html
Comment 19 Build Bot 2018-07-03 04:54:02 PDT
Created attachment 344176 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 20 Caitlin Potter (:caitp) 2018-07-03 08:39:18 PDT
Comment on attachment 344172 [details]
Instance class fields

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

A couple notes to clean this up a little bit (not including style linter problems)

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3092
> +    RefPtr<RegisterID> initializer = emitDirectGetById(newTemporary(), constructor, propertyNames().instanceFieldInitializerSymbol);

(e.g. here)

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4173
> +            generator.emitDirectPutById(constructor.get(), generator.propertyNames().instanceFieldInitializerSymbol, instanceFieldInitializer.get(), PropertyNode::Unknown);

(and here)

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2169
> +    case ToPropertyKey: {

the initial work I did on this ToPropertyKey thing was split up so that the JIT/DFG/FTL stuff is separated, we might want to keep it to the baseline stuff only for now, unless someone decides having JIT support is worthwhile. I really just wanted to learn more about how the speculative jit and FTL lowering works in JSC.

but to justify the ToPropertyKey thing, we do fail tests without it, since we need to convert computed names to a string before they get defined on the instance (for computed instance fields).

> Source/JavaScriptCore/parser/Parser.cpp:3006
> +            [[fallthrough]];

s/[[fallthrough]]/FALLTHROUGH/ and move it after the label so it's right before the next case label

> Source/JavaScriptCore/runtime/CommonIdentifiers.h:289
> +    macro(instanceFieldInitializer)

we should move this to BuiltinNames.h (references in NodesCodegen.cpp and BytecodeGenerator.cpp need to be updated to the `builtinNames().instanceFieldInitializerPrivateName` version) --- to fix the O.getOwnPropertyNames failure.
Comment 21 Caitlin Potter (:caitp) 2018-07-03 08:47:42 PDT
the remaining failures (LayoutTests/js/class-field-syntax.html) just needs its expected.txt file updated, since the semicolon is valid in those contexts now.
Comment 22 Xan Lopez 2018-07-04 01:51:07 PDT
FWIW, we'll keep working on this, and we plan to tackle the remaining bugs/optimizations. We can wait for a review until then, and of course we can split the patch logically to make that easier. Just uploading the current WIP since it's already pretty advanced, any feedback will be welcome.
Comment 23 Xan Lopez 2018-09-05 03:05:59 PDT
Created attachment 348905 [details]
instance class fields (09/05/18)

Another big patch for comments. Most outstanding issues in the previous patch are fixed now, and we pass all the test262 tests in the tree (both -public and -private).

The commit by commit branch can be reviewed at: https://github.com/igalia/webkit/commits/class-fields
Comment 24 Build Bot 2018-09-05 03:08:43 PDT
Attachment 348905 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/VariableEnvironment.h:161:  The parameter name "identifier" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/VariableEnvironment.h:162:  The parameter name "identifier" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/VariableEnvironment.h:184:  The parameter name "identifier" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/VariableEnvironment.h:193:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1250:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1313:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1378:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/parser/VariableEnvironment.cpp:126:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/parser/VariableEnvironment.cpp:136:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:479:  Missing space around : in range-based for statement  [whitespace/colon] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:1233:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:1719:  The parameter name "ident" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 12 in 63 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 25 Build Bot 2018-09-05 05:01:47 PDT
Comment on attachment 348905 [details]
instance class fields (09/05/18)

Attachment 348905 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9100535

New failing tests:
js/class-syntax-semicolon.html
Comment 26 Build Bot 2018-09-05 05:01:59 PDT
Created attachment 348907 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 27 Xan Lopez 2018-09-26 13:42:43 PDT
Created attachment 350893 [details]
Class fields WIP
Comment 28 Build Bot 2018-09-26 13:46:19 PDT
Attachment 350893 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/VariableEnvironment.h:213:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 65 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 29 Xan Lopez 2018-09-26 13:50:30 PDT
Created attachment 350896 [details]
Class fields WIP
Comment 30 Xan Lopez 2018-09-26 14:03:33 PDT
(In reply to Xan Lopez from comment #29)
> Created attachment 350896 [details]
> Class fields WIP

This patch passes all the test262 test in the tree, as the commit says, so at this point we'd definitely welcome an initial review to see where we are at and what are the future steps to get this landed.
Comment 31 Build Bot 2018-09-26 15:20:56 PDT
Comment on attachment 350896 [details]
Class fields WIP

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

New failing tests:
jsc-layout-tests.yaml/js/script-tests/class-syntax-semicolon.js.layout-no-ftl
stress/generator-syntax.js.no-cjit-collect-continuously
stress/generator-syntax.js.ftl-no-cjit-small-pool
stress/method-name.js.no-cjit-validate-phases
stress/generator-syntax.js.no-llint
stress/method-name.js.ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/class-syntax-semicolon.js.layout-no-cjit
stress/async-await-syntax.js.no-ftl
stress/async-await-syntax.js.ftl-no-cjit-validate-sampling-profiler
stress/method-name.js.ftl-no-cjit-b3o1
stress/generator-syntax.js.ftl-no-cjit-no-put-stack-validate
stress/generator-syntax.js.ftl-no-cjit-b3o1
stress/generator-syntax.js.ftl-no-cjit-no-inline-validate
stress/generator-syntax.js.dfg-maximal-flush-validate-no-cjit
stress/method-name.js.ftl-eager-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/class-syntax-semicolon.js.layout
stress/async-await-syntax.js.ftl-no-cjit-no-inline-validate
stress/async-await-syntax.js.dfg-maximal-flush-validate-no-cjit
stress/async-await-syntax.js.dfg-eager
stress/async-await-syntax.js.ftl-eager
stress/async-await-syntax.js.no-cjit-collect-continuously
stress/async-await-syntax.js.ftl-no-cjit-small-pool
stress/async-await-syntax.js.no-llint
stress/method-name.js.dfg-eager-no-cjit-validate
stress/async-await-syntax.js.ftl-no-cjit-no-put-stack-validate
stress/async-await-syntax.js.ftl-eager-no-cjit-b3o1
stress/method-name.js.ftl-no-cjit-validate-sampling-profiler
stress/generator-syntax.js.no-cjit-validate-phases
stress/generator-syntax.js.ftl-eager
stress/async-await-syntax.js.ftl-eager-no-cjit
stress/method-name.js.no-llint
jsc-layout-tests.yaml/js/script-tests/class-syntax-semicolon.js.layout-ftl-eager-no-cjit
stress/async-await-syntax.js.no-cjit-validate-phases
stress/method-name.js.dfg-eager
stress/method-name.js.ftl-eager
stress/method-name.js.no-cjit-collect-continuously
stress/method-name.js.default
jsc-layout-tests.yaml/js/script-tests/class-syntax-semicolon.js.layout-ftl-no-cjit
stress/generator-syntax.js.dfg-eager-no-cjit-validate
stress/async-await-syntax.js.ftl-no-cjit-b3o1
stress/generator-syntax.js.dfg-eager
stress/method-name.js.ftl-no-cjit-small-pool
stress/method-name.js.dfg-maximal-flush-validate-no-cjit
stress/method-name.js.no-ftl
stress/generator-syntax.js.no-ftl
stress/generator-syntax.js.ftl-no-cjit-validate-sampling-profiler
stress/method-name.js.ftl-no-cjit-no-inline-validate
jsc-layout-tests.yaml/js/script-tests/class-syntax-semicolon.js.layout-dfg-eager-no-cjit
stress/async-await-syntax.js.default
stress/method-name.js.ftl-no-cjit-no-put-stack-validate
stress/async-await-syntax.js.dfg-eager-no-cjit-validate
jsc-layout-tests.yaml/js/script-tests/class-syntax-semicolon.js.layout-no-llint
stress/generator-syntax.js.ftl-eager-no-cjit-b3o1
stress/generator-syntax.js.default
stress/generator-syntax.js.ftl-eager-no-cjit
apiTests
Comment 32 Build Bot 2018-09-26 15:28:48 PDT
Comment on attachment 350896 [details]
Class fields WIP

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

New failing tests:
js/class-syntax-semicolon.html
Comment 33 Build Bot 2018-09-26 15:28:50 PDT
Created attachment 350917 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 34 Build Bot 2018-09-26 16:38:54 PDT
Comment on attachment 350896 [details]
Class fields WIP

Attachment 350896 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9361424

New failing tests:
js/class-syntax-semicolon.html
Comment 35 Build Bot 2018-09-26 16:38:56 PDT
Created attachment 350923 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 36 Build Bot 2018-09-26 16:45:20 PDT
Comment on attachment 350896 [details]
Class fields WIP

Attachment 350896 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9361215

New failing tests:
js/class-syntax-semicolon.html
Comment 37 Build Bot 2018-09-26 16:45:22 PDT
Created attachment 350924 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 38 Build Bot 2018-09-26 16:50:03 PDT
Comment on attachment 350896 [details]
Class fields WIP

Attachment 350896 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9361429

New failing tests:
js/class-syntax-semicolon.html
Comment 39 Build Bot 2018-09-26 16:50:15 PDT
Created attachment 350925 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 40 Build Bot 2018-09-26 18:46:02 PDT
Comment on attachment 350896 [details]
Class fields WIP

Attachment 350896 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9362553

New failing tests:
js/class-syntax-semicolon.html
Comment 41 Build Bot 2018-09-26 18:46:04 PDT
Created attachment 350935 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 42 Build Bot 2018-09-27 08:32:37 PDT
Comment on attachment 350896 [details]
Class fields WIP

Attachment 350896 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9368001

New failing tests:
js/class-syntax-semicolon.html
Comment 43 Build Bot 2018-09-27 08:32:39 PDT
Created attachment 350962 [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.13.6
Comment 44 Xan Lopez 2018-10-01 10:32:23 PDT
Created attachment 351261 [details]
Class fields (10/01)
Comment 45 Build Bot 2018-10-01 12:07:25 PDT
Comment on attachment 351261 [details]
Class fields (10/01)

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

New failing tests:
stress/async-await-syntax.js.ftl-eager-no-cjit-b3o1
stress/async-await-syntax.js.ftl-eager-no-cjit
stress/async-await-syntax.js.ftl-no-cjit-no-inline-validate
stress/async-await-syntax.js.dfg-maximal-flush-validate-no-cjit
stress/async-await-syntax.js.dfg-eager
stress/async-await-syntax.js.ftl-eager
stress/async-await-syntax.js.no-ftl
stress/async-await-syntax.js.ftl-no-cjit-validate-sampling-profiler
stress/async-await-syntax.js.no-cjit-validate-phases
stress/async-await-syntax.js.ftl-no-cjit-small-pool
stress/async-await-syntax.js.default
stress/async-await-syntax.js.no-llint
stress/async-await-syntax.js.dfg-eager-no-cjit-validate
stress/async-await-syntax.js.no-cjit-collect-continuously
stress/async-await-syntax.js.ftl-no-cjit-no-put-stack-validate
stress/async-await-syntax.js.ftl-no-cjit-b3o1
apiTests
Comment 46 Build Bot 2018-10-01 12:37:29 PDT
Comment on attachment 351261 [details]
Class fields (10/01)

Attachment 351261 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9413513

New failing tests:
media/range-extract-contents-crash.html
Comment 47 Build Bot 2018-10-01 12:37:31 PDT
Created attachment 351286 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 48 Xan Lopez 2018-10-02 01:22:28 PDT
Created attachment 351354 [details]
Class fields WIP (10/02)
Comment 49 Michael Catanzaro 2018-10-05 02:13:38 PDT
My understanding is this patch is ready to be reviewed and committed.

A couple things needed first:

 * Patch requires a ChangeLog entry created with prepare-ChangeLog
 * Attachment should not include "WIP" since then everyone looking at this bug thinks it's not ready for review yet :)
Comment 50 Daniel Ehrenberg 2018-10-06 12:08:35 PDT
A few suggestions for this patch:
- Put this new behavior behind a flag which is initially off (at least a runtime flag, and possibly also a compile-time flag if needed to avoid performance regressions); if possible, ensure that the appropriate tests run both with and without the flag.
- Include a more detailed commit message, summarizing the design of the patch, etc. It looks like the FIXMEs/TODOs are potential future optimizations, not things that would need to be fixed before landing. Is this right? If so, maybe this could be clarified in the commit message.
- Consider separating this large change into multiple small changes, to make review simpler.

We'll review this change further within Igalia before needing any upstream review. The WIP tag indicates that. If someone adventurous wants to dive into the current code, that'd still be appreciated, of course.

Xan, if you have any particular questions for a high-level design review from upstream, it'd be helpful to name those explicitly in comments here or on IRC.
Comment 51 Michael Catanzaro 2018-10-07 12:13:59 PDT
Comment on attachment 351354 [details]
Class fields WIP (10/02)

Removing r? per Dan's comment.
Comment 52 Xan Lopez 2018-10-10 02:52:14 PDT
Now that the tests are all green we are working on splitting parser+bytecode+test fixes in one patch to ease review, leaving the current optimizations aside for a second round. We of course still have the git repository with the whole series, and anyone can have a look at the full patch, but it's good if you wait for the next patch uploaded to bugzilla. We'll also address the suggestions you all made here.
Comment 53 Xan Lopez 2018-11-02 07:16:46 PDT
Created attachment 353696 [details]
Class fields, parser + bytecode + test changes

Here is the patch implementing the parser + bytecode changes for instance public/private fields. Some comments:

- As mentioned in the previous comment, we have left out the JIT/DFG changes for a follow-up patch. This should ease the review process.

- There are no runtime/compile-time flags for the feature. This is mostly to allow the bots to exercise the new code properly, but they can of course be added before landing if they are necessary.

- We have tried to explain the main design decisions and observable changes in the ChangeLog.
Comment 54 Xan Lopez 2018-11-02 07:26:09 PDT
Created attachment 353698 [details]
Class fields, parser + bytecode + test changes
Comment 55 Xan Lopez 2018-11-02 07:30:23 PDT
Created attachment 353699 [details]
Class fields, parser + bytecode + test changes
Comment 56 Xan Lopez 2018-11-05 03:50:37 PST
Created attachment 353848 [details]
Class fields, parser + bytecode + test changes
Comment 57 Xan Lopez 2018-11-05 03:57:27 PST
Created attachment 353849 [details]
Class fields, parser + bytecode + test changes
Comment 58 Dominik Inführ 2018-11-05 05:19:21 PST
Comment on attachment 353849 [details]
Class fields, parser + bytecode + test changes

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

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2635
> +    loadConstantOrVariablePayload(size, t0, CellTag, t3, .opGetPrivateFieldSlow)

This should probably be `.opPutPrivateFieldSlow`.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1366
> +        metadata.oldStructure = metadata.newStructure = metadata.offset = 0;

This also fails to build, GCC doesn't allow assigning 0 to a Structure*.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1430
> +            metadata.structure = metadata.offset = 0;

Ditto

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:1492
> +        metadata.structure = metadata.offset = 0;

Ditto.
Comment 59 Xan Lopez 2018-11-05 06:18:46 PST
Created attachment 353857 [details]
Class fields, parser + bytecode + test changes
Comment 60 Xan Lopez 2018-11-06 02:06:36 PST
Created attachment 353954 [details]
Class fields, parser + bytecode + test changes

A couple of bots took so long to process the patch that there were conflicts to apply it. Try again to see what's their status, since all the others are now green.
Comment 61 Xan Lopez 2018-11-07 05:22:24 PST
Comment on attachment 353954 [details]
Class fields, parser + bytecode + test changes

Marking as r?. Please see comment #53 for some additional comments about the patch.
Comment 62 Caitlin Potter (:caitp) 2018-12-04 08:43:23 PST
This is ready for review
Comment 63 Caio Lima 2018-12-04 11:15:03 PST
Comment on attachment 353954 [details]
Class fields, parser + bytecode + test changes

I think we either should add a feature flag or add Baseline support before landing. The problem is current Patch crashes on following program:

```
//@ runBigIntEnabled

class A {
    #a = 3;

    getV() {
        return this.#a;
    }

}

let o = new A();
for (let i = 0; i < 1000; i++) {
    o.getV();
}
```

Here is the StackTrace:

```
SHOULD NEVER BE REACHED
...webkit/Source/JavaScriptCore/jit/JIT.cpp(444) : void JSC::JIT::privateCompileMainPass()
1   0x109de1949 WTFCrash
2   0x1084d1cab WTFCrashWithInfo(int, char const*, char const*, int)
3   0x1095b6029 JSC::JIT::privateCompileMainPass()
4   0x1095b92e0 JSC::JIT::compileWithoutLinking(JSC::JITCompilationEffort)
5   0x109657755 JSC::JITWorklist::Plan::compileInThread()
6   0x109652a6a JSC::JITWorklist::Plan::compileNow(JSC::CodeBlock*, unsigned int)
7   0x109652876 JSC::JITWorklist::compileLater(JSC::CodeBlock*, unsigned int)
8   0x10969c0bf JSC::LLInt::jitCompileAndSetHeuristics(JSC::CodeBlock*, JSC::ExecState*, unsigned int)
9   0x10969b788 JSC::LLInt::entryOSR(JSC::ExecState*, JSC::Instruction const*, JSC::CodeBlock*, char const*, JSC::LLInt::EntryKind)
10  0x10969b94a llint_entry_osr_function_for_call
11  0x109686464 llint_entry
12  0x109697d95 llint_entry
13  0x109684020 vmEntryToJavaScript
14  0x10958430e JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
15  0x1095838bf JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*)
16  0x10989a555 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
17  0x108516bbb runWithOptions(GlobalObject*, CommandLine&, bool&)
18  0x1084eec2c jscmain(int, char**)::$_4::operator()(JSC::VM&, GlobalObject*, bool&) const
19  0x1084d3c54 int runJSC<jscmain(int, char**)::$_4>(CommandLine, bool, jscmain(int, char**)::$_4 const&)
20  0x1084d265f jscmain(int, char**)
21  0x1084d25be main
22  0x7fff6fc77015 start
23  0x4
```
Comment 64 Xan Lopez 2018-12-05 02:55:40 PST
Created attachment 356594 [details]
Placeholder baseline JIT for private field opcodes

Thanks Caio, this should take care of it for an initial patch where we leave off JIT/DFG. I'm uploading it as a separate patch to ease review, but it could be trivially be folded in the previous one.
Comment 65 Build Bot 2018-12-05 02:58:43 PST
Attachment 356594 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITOpcodes.cpp:1556:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITOpcodes.cpp:1556:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/JITOpcodes.cpp:1560:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 66 Xan Lopez 2018-12-05 03:37:26 PST
Created attachment 356595 [details]
Placeholder baseline JIT for private field opcodes

Fix style.
Comment 67 Saam Barati 2019-01-20 14:50:51 PST
Do you still need a review for this? Why are there two different patches?
Comment 68 Xan Lopez 2019-01-22 03:22:05 PST
(In reply to Saam Barati from comment #67)
> Do you still need a review for this? Why are there two different patches?

Indeed it does! As I said in comment #64 the second patch is just a trivial fix that can be folded easily in the main patch, but the main focus right now for us would be to get a first review of the "Class fields, parser + bytecode + test changes" patch, which is the first step to get the feature in the tree.
Comment 69 Xan Lopez 2019-01-29 02:10:25 PST
Created attachment 360454 [details]
Class fields, parser + bytecode + test changes

Patch refresh.

This includes the previous patch rebased against master (01/29/2019), plus the fix for the crash when JIT baseline kicks in, plus a few fixes that were uncovered with the recent test262 refresh.
Comment 70 Caitlin Potter (:caitp) 2019-02-01 10:53:04 PST
(In reply to Saam Barati from comment #67)
> Do you still need a review for this? Why are there two different patches?

As mentioned offline, we do still need review on this, and are under some pressure to get this upstream. It would be really good if we could get eyes taking a look at this :) The two different patches have been merged into one.

(PING: Yusuke, Keith, msaboff)
Comment 71 Caio Lima 2019-02-02 08:06:31 PST
Comment on attachment 360454 [details]
Class fields, parser + bytecode + test changes

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

Patch mostly LGTM. I have some minor comments.

> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp:46
> +static_assert(sizeof(UnlinkedFunctionExecutable) <= 176, "UnlinkedFunctionExecutable should fit in a 160-byte cell. If you increase the size of this class, consider making a size class that perfectly fits it.");

I think we should change the 160 in the message as well.

> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:199
> +    Vector<JSTextPosition> m_instanceFieldLocations;

I think this should be a FIXME. Also It is good have a link to the bug.
Comment 72 Maciej Stachowiak 2019-02-13 23:45:58 PST
Comment on attachment 360454 [details]
Class fields, parser + bytecode + test changes

Boring feedback: pleas put this behind a runtime or compile-time feature flag (per WebKit project policy for new features), so we can decouple decisions about shipping it from decisions about landing it. (I don't see a flag in this large-ish patch but maybe I missed something).
Comment 73 Saam Barati 2019-02-14 09:08:58 PST
Comment on attachment 360454 [details]
Class fields, parser + bytecode + test changes

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

> JSTests/ChangeLog:21
> +        * stress/async-await-syntax.js:

Some fly by feedback: am I reading this right that there are no new tests besides what’s in test262? If so, this patch should be adding a lot of JSC specific tests. Tests that you think stress interesting implementation details and how your patch interacts with the JIT/GC
Comment 74 Saam Barati 2019-02-14 18:47:48 PST
Comment on attachment 360454 [details]
Class fields, parser + bytecode + test changes

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

I still have to read some more code, but here is my initial feedback.

> Source/JavaScriptCore/ChangeLog:19
> +        present. The field is always loaded during class construction or
> +        super calls, but is not invoked if the value is undefined. This is
> +        similar to what the v8 engine does.

Why do we always need to load it? Seems like this is bad for perf. Shouldn't we know statically if we have this function?

Also, why are we implementing this as a function call instead of just inlining the instantiation of these fields directly? I vaguely remember a reason for it done this way, but it seems strictly less optimal than just inlining it. It would be great if using class fields isn't slower than just normally adding properties to |this|.

What scope are the fields evaluated in?

> Source/JavaScriptCore/ChangeLog:23
> +        WTF::Vector<JSTextPosition>.  These are used to reparse the class

I think it'd be awesome to find some way to inline field instantiation. It might not be super trivial. Some issues I see:
- Teaching the parser how to swap over and parse something in a different context.
- Teaching the bytecode generator how to do some scope swapping to ensure we're not evaluating code in a scope we shouldn't be.

> Source/JavaScriptCore/ChangeLog:33
> +        (although a new EvalContextType is needed to handle the use of

We don't already know this? What are the early errors? Can you enumerate them?

> Source/JavaScriptCore/ChangeLog:54
> +            For each computed class field name, `[ {expr} ] = {initializer}`:

what happens for duplicate names? We call the set twice?

> Source/JavaScriptCore/ChangeLog:64
> +            3) instance[fieldName] = value

Is this actually a Put. Or is this a defineOwnProperty?

> Source/JavaScriptCore/ChangeLog:68
> +        Although this is somewhat hacky, and should be replaced with
> +        something better, it works and is fairly straight forward to
> +        understand.

This doesn't seem that crazy.

> Source/JavaScriptCore/ChangeLog:70
> +        Finally this introduces a new opcode (op_to_property_key). There

"There" => "Previously, there"

> Source/JavaScriptCore/ChangeLog:86
> +           (Caveat: this requires looking them up in scope each time
> +           they're used --- though DFG can hopefully identify when they're
> +           reused and only emit the load once, or replace it with a constant
> +           in some cases).

The DFG will indeed CSE these. It will also constant fold them if a class is only ever evaluated once (so the scope is only written to once).

> Source/JavaScriptCore/ChangeLog:91
> +        3) avoid adding additional slow cases to get/put operations to the common
> +           cases, avoid adding new GetPutInfo complexity, etc

I guess I'm missing why this is actually needed. What about get|put_by_val doesn't work?

> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:198
> +    // TODO: only allocate this if needed.

Do we have a RareData for this class?

>> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:199
>> +    Vector<JSTextPosition> m_instanceFieldLocations;
> 
> I think this should be a FIXME. Also It is good have a link to the bug.

+1

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1946
> +                if (privateName.value.isDeclared())

What does it mean for it to not be declared? Is this computed vs not computed?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1950
> +            // FIXME: determine if private names are captured in a smarter way.

is this valid given your comment below?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2825
> +RegisterID* BytecodeGenerator::emitInstanceFieldInitializationIfNeeded(RegisterID* dst, RegisterID* constructor, const JSTextPosition& divot, const JSTextPosition& divotStart, const JSTextPosition& divotEnd)

Seems like we should know this statically. Am I missing something as to why that's not feasible? Instead of this branch being at runtime, it should be at bytecode generation time.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:734
> +    const Identifier& description = *node.name();

I'm super confused. What is this if this is supposed to be a computed field name?

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:742
> +    // TODO: add type profiling if we need it?

General comment: TODOs should be FIXMEs with a link. However, I think you're ok on this once. I don't see a reason you need to have the typeProfiler do anything here.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:746
> +    generator.liftTDZCheckIfPossible(var);

This seems wrong, but I'm not totally sure the context of what's going on here. See comment above about Identifier.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4041
> +            generator.emitSetFunctionName(value.get(), *m_ident);

An idea for tests that I suggested earlier: this is exactly the kind of thing that's good to have tests for.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4047
> +        Optional<uint32_t> optionalIndex = parseIndex(*m_ident);
> +        if (!optionalIndex)

Style nit: I like to write this in a different way:

if (Optional<uint32_t> optionalIndex = parseIndex(*m_ident))
   ... use optionalndex
else
   ... don't use optionalIndex

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4055
> +    case DefineFieldNode::PrivateName: {

Are these computed private names?

Anyways, in general, I have a question on computed private names. What are the semantics, of say:
class C {
   #["foo"] = 42
   #["foo"] = 44
}

Do I end up having two properties or one here?

Also, is there syntax to access private names in a computed fashion?

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4057
> +        generator.emitPrivateFieldAdd(generator.thisRegister(), *m_ident, value.get());

Why isn't this just put_to_scope?

> Source/JavaScriptCore/dfg/DFGCapabilities.cpp:284
> +    case op_add_private_field: // TODO: add JIT support for private field ops

Make this a "FIXME:" w/ bugzilla link

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2596
> +    # (Can we be sure there's room in the existing backing store for non-inline fields?)

What's up with this comment? Did you figure it out?

> Source/JavaScriptCore/runtime/JSScope.cpp:343
> +            auto privateNames = symbolTable->privateNames();

Why isn't this a hashtable?

> Source/JavaScriptCore/runtime/JSSymbolTableObject.h:51
> +    PrivateSymbolImpl* getPrivateSymbol(const Identifier& name)

I think describing the object model you're going for here would help me read through this code more easily. For example, I don't follow the object model for private fields yet

> Source/JavaScriptCore/runtime/JSSymbolTableObject.h:98
> +    std::unique_ptr<JSSymbolTableObjectRareData> m_rareData;

I'm confused why we need this. Why isn't this just part of the SymbolTable itself?

> Source/JavaScriptCore/runtime/SymbolTable.h:738
> +        PrivateNameSet m_privateNames;

Why isn't this just in the normal hash table? I think that'd Just Work.
Comment 75 Daniel Ehrenberg 2019-02-15 04:54:18 PST
Jumping in to answer some specification questions.

> Is this actually a Put. Or is this a defineOwnProperty?

This should be a defineOwnProperty.

> what happens for duplicate names? We call the set twice?

That's what it should do, spec-wise (whether or not it's computed)

> Are these computed private names?
> 
> Also, is there syntax to access private names in a computed fashion?

Computed private names are not supported.

> Also, why are we implementing this as a function call instead of just inlining the instantiation of these fields directly?

You can probably do that most of the time, but given the flexibility of where super() can be called (e.g., inside an eval), it's probably too hard to do 100% of the time. See https://github.com/tc39/proposal-class-fields#execution-of-initializer-expressions
Comment 76 Yusuke Suzuki 2019-02-15 10:59:33 PST
Comment on attachment 360454 [details]
Class fields, parser + bytecode + test changes

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

>> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:198
>> +    // TODO: only allocate this if needed.
> 
> Do we have a RareData for this class?

Please do this in this patch since sizeof(UnlinkedFunctionExecutable) is significantly allocated cells.
Comment 77 Caitlin Potter (:caitp) 2019-02-15 11:32:13 PST
Comment on attachment 360454 [details]
Class fields, parser + bytecode + test changes

I don't have answers to all of these questions, particularly WRT the way private names are kept in Scope / SymbolTable. I think there was a reason for everything that was done there, but there's
a good chance it can be simplified (as some of it is very old and leftover from an older iteration, etc).

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

>> Source/JavaScriptCore/ChangeLog:19
>> +        similar to what the v8 engine does.
> 
> Why do we always need to load it? Seems like this is bad for perf. Shouldn't we know statically if we have this function?
> 
> Also, why are we implementing this as a function call instead of just inlining the instantiation of these fields directly? I vaguely remember a reason for it done this way, but it seems strictly less optimal than just inlining it. It would be great if using class fields isn't slower than just normally adding properties to |this|.
> 
> What scope are the fields evaluated in?

> Why do we always need to load it? Seems like this is bad for perf. Shouldn't we know statically if we have this function?

It isn't great for perf, but it probably doesn't hurt that much, it's one extra property load per class constructor/super call.

The main reason why it's done this way currently is, when we're emitting the bytecodes for the constructor, it doesn't include
the AST for the rest of the class fields, and we haven't yet slipped information about class fields into the CodeBlock/Executable
(which currently is awkward to sneak in).

Since the information is not currently available when the constructor is generated, we do this dynamic load to avoid the problem. It
shouldn't be too difficult to make it work, though.

---

> Also, why are we implementing this as a function call instead of just inlining the instantiation of these fields directly? I vaguely remember a reason for it done this way, but it seems strictly less optimal than just inlining it. It would be great if using class fields isn't slower than just normally adding properties to |this|.

Yeah, this is similar to the previous point --- not having information about the class fields during constructor generation. The scope rules also make it a bit awkward (need a new scope whose parent is the class scope, and which sees `this` and `super`, but nothing else). I'm sure this is doable, and we have plans to do this in a followup patch, but it seemed a bit much for the first implementation.

---

> What scope are the fields evaluated in?

Essentially, it's a nested block scope under the class lexical scope, with the following bindings:

- if there is a class heritage, it can access super properties (but, super calls are illegal)
- it can access `this` (the class instance)
- `arguments` is forbidden ("It is a Syntax Error if ContainsArguments of Initializer is true." with a similar rule for arguments-in-eval-in-initializer)

>> Source/JavaScriptCore/ChangeLog:23
>> +        WTF::Vector<JSTextPosition>.  These are used to reparse the class
> 
> I think it'd be awesome to find some way to inline field instantiation. It might not be super trivial. Some issues I see:
> - Teaching the parser how to swap over and parse something in a different context.
> - Teaching the bytecode generator how to do some scope swapping to ensure we're not evaluating code in a scope we shouldn't be.

Agree --- do you see this as a blocker for landing? It seems suitable for a followup optimization. Loading the initializer function is unfortunate and may incur a very minor performance hit for ES6 classes, but I think that hit would be relatively minor, and probably too short lived to make it into a release.

>> Source/JavaScriptCore/ChangeLog:33
>> +        (although a new EvalContextType is needed to handle the use of
> 
> We don't already know this? What are the early errors? Can you enumerate them?

`arguments` is not allowed in a direct eval call within a field initializer.

>> Source/JavaScriptCore/ChangeLog:54
>> +            For each computed class field name, `[ {expr} ] = {initializer}`:
> 
> what happens for duplicate names? We call the set twice?

Yes --- If multiple computed fields become the same name during class field evaluation, then the same instance field would be defined multiple times per instance, per spec.

>> Source/JavaScriptCore/ChangeLog:64
>> +            3) instance[fieldName] = value
> 
> Is this actually a Put. Or is this a defineOwnProperty?

For non-PrivateName fields, it's specced as CreateDataPropertyOrThrow, which is essentially a Put.

>> Source/JavaScriptCore/ChangeLog:86
>> +           in some cases).
> 
> The DFG will indeed CSE these. It will also constant fold them if a class is only ever evaluated once (so the scope is only written to once).

in this patch it won't, we never really got that far with the optimization. But I agree it should be possible to do this better when we tier up.

>> Source/JavaScriptCore/ChangeLog:91
>> +           cases, avoid adding new GetPutInfo complexity, etc
> 
> I guess I'm missing why this is actually needed. What about get|put_by_val doesn't work?

It's doable, however there are a number of reasons why I originally opted to go the other route:

It looks like get/put_by_value are mainly optimized for indexed property access (at least in baseline, not sure how they do after tiering up). They also handle a lot of different cases already,
and private field support would add a number of other requirements, which would likely keep private fields permanently on the slow path, and dispatching to private field cases would have a slight impact
on other code.
Specifically, we'd need to dispatch for: private fields (distinct from existing PrivateIdentifier symbols, slightly different rules), brand checking (private methods/accessors), throwing if the field does not exist (or already exists in the DefineField case).

In addition to that, we'd probably need to wrap the private names in a JSCell, so that they can be fit in a JSValue, to avoid changing too much of the code. If we wound up using a raw UID and dispatching
early based on a flag, then it doesn't really save any code. Allocating JSCells for each of these potentially eats up a lot of heap space if classes are evaluated multiple times (as is done in some of the mixin examples I've seen from JS developers).

We are happy to change this to use the existing opcodes before or after landing, but I think there are good reasons not to.

To summarize:
  - avoid slowing down common operations by adding more dispatching
  - avoid using up extra JS heap space by putting our UIDs in a JSCell wrapper (a new one for each field for each time the class is evaluated) for compat with op_get|put_by_val
  - (very minor point) better developer ergonomics: easier to read "get_private_field #foo" vs the combination of ops that would be needed for the other cases

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4055
>> +    case DefineFieldNode::PrivateName: {
> 
> Are these computed private names?
> 
> Anyways, in general, I have a question on computed private names. What are the semantics, of say:
> class C {
>    #["foo"] = 42
>    #["foo"] = 44
> }
> 
> Do I end up having two properties or one here?
> 
> Also, is there syntax to access private names in a computed fashion?

I believe this was answered by Dan earlier, but private names are never computed.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4057
>> +        generator.emitPrivateFieldAdd(generator.thisRegister(), *m_ident, value.get());
> 
> Why isn't this just put_to_scope?

I'm not sure I understand how this would work with `put_to_scope` --- the method scope wouldn't be very useful, the class scope could be shared by multiple different instances (which might want to store different values in their private field).

>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2596
>> +    # (Can we be sure there's room in the existing backing store for non-inline fields?)
> 
> What's up with this comment? Did you figure it out?

IIRC (and it's been a while since I wrote this), `storePropertyAtVariableOffset` takes care of bailing out if the backing store is not big enough, which means we can probably get rid of the comment.
Comment 78 Xan Lopez 2019-02-18 01:39:49 PST
Just a couple extra comments:

(In reply to Saam Barati from comment #74)

> > Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:198
> > +    // TODO: only allocate this if needed.
> 
> Do we have a RareData for this class?

This was indeed planned, so we'll try to have it in the next patch already.

> 
> What does it mean for it to not be declared? Is this computed vs not
> computed?

This is meant to track whether a private name is being used in the class before it's declared.
Comment 79 Saam Barati 2019-03-11 11:22:47 PDT
(In reply to Caitlin Potter (:caitp) from comment #77)
> Comment on attachment 360454 [details]
> Class fields, parser + bytecode + test changes
> 
> I don't have answers to all of these questions, particularly WRT the way
> private names are kept in Scope / SymbolTable. I think there was a reason
> for everything that was done there, but there's
> a good chance it can be simplified (as some of it is very old and leftover
> from an older iteration, etc).
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360454&action=review
> 
> >> Source/JavaScriptCore/ChangeLog:19
> >> +        similar to what the v8 engine does.
> > 
> > Why do we always need to load it? Seems like this is bad for perf. Shouldn't we know statically if we have this function?
> > 
> > Also, why are we implementing this as a function call instead of just inlining the instantiation of these fields directly? I vaguely remember a reason for it done this way, but it seems strictly less optimal than just inlining it. It would be great if using class fields isn't slower than just normally adding properties to |this|.
> > 
> > What scope are the fields evaluated in?
> 
> > Why do we always need to load it? Seems like this is bad for perf. Shouldn't we know statically if we have this function?
> 
> It isn't great for perf, but it probably doesn't hurt that much, it's one
> extra property load per class constructor/super call.
> 
> The main reason why it's done this way currently is, when we're emitting the
> bytecodes for the constructor, it doesn't include
> the AST for the rest of the class fields, and we haven't yet slipped
> information about class fields into the CodeBlock/Executable
> (which currently is awkward to sneak in).
> 
> Since the information is not currently available when the constructor is
> generated, we do this dynamic load to avoid the problem. It
> shouldn't be too difficult to make it work, though.

I think we should make this work. I'm also not sure exactly how annoying it'll be to thread this through, but it should be a goal to not make existing classes slower to support class fields.

It's also our goal to make using class fields no slower than just doing `this.<field> = <expr>` in the constructor. But I think it's reasonable if we continue to speed this up in follow up patches.

> 
> ---
> 
> > Also, why are we implementing this as a function call instead of just inlining the instantiation of these fields directly? I vaguely remember a reason for it done this way, but it seems strictly less optimal than just inlining it. It would be great if using class fields isn't slower than just normally adding properties to |this|.
> 
> Yeah, this is similar to the previous point --- not having information about
> the class fields during constructor generation. The scope rules also make it
> a bit awkward (need a new scope whose parent is the class scope, and which
> sees `this` and `super`, but nothing else). I'm sure this is doable, and we
> have plans to do this in a followup patch, but it seemed a bit much for the
> first implementation.

This sounds reasonable to do in a followup. Can we open a bug to track doing that?

> 
> ---
> 
> > What scope are the fields evaluated in?
> 
> Essentially, it's a nested block scope under the class lexical scope, with
> the following bindings:
> 
> - if there is a class heritage, it can access super properties (but, super
> calls are illegal)
> - it can access `this` (the class instance)
> - `arguments` is forbidden ("It is a Syntax Error if ContainsArguments of
> Initializer is true." with a similar rule for
> arguments-in-eval-in-initializer)
> 
> >> Source/JavaScriptCore/ChangeLog:23
> >> +        WTF::Vector<JSTextPosition>.  These are used to reparse the class
> > 
> > I think it'd be awesome to find some way to inline field instantiation. It might not be super trivial. Some issues I see:
> > - Teaching the parser how to swap over and parse something in a different context.
> > - Teaching the bytecode generator how to do some scope swapping to ensure we're not evaluating code in a scope we shouldn't be.
> 
> Agree --- do you see this as a blocker for landing? It seems suitable for a
> followup optimization. Loading the initializer function is unfortunate and
> may incur a very minor performance hit for ES6 classes, but I think that hit
> would be relatively minor, and probably too short lived to make it into a
> release.
> 
> >> Source/JavaScriptCore/ChangeLog:33
> >> +        (although a new EvalContextType is needed to handle the use of
> > 
> > We don't already know this? What are the early errors? Can you enumerate them?
> 
> `arguments` is not allowed in a direct eval call within a field initializer.
> 
> >> Source/JavaScriptCore/ChangeLog:54
> >> +            For each computed class field name, `[ {expr} ] = {initializer}`:
> > 
> > what happens for duplicate names? We call the set twice?
> 
> Yes --- If multiple computed fields become the same name during class field
> evaluation, then the same instance field would be defined multiple times per
> instance, per spec.
> 
> >> Source/JavaScriptCore/ChangeLog:64
> >> +            3) instance[fieldName] = value
> > 
> > Is this actually a Put. Or is this a defineOwnProperty?
> 
> For non-PrivateName fields, it's specced as CreateDataPropertyOrThrow, which
> is essentially a Put.
> 
> >> Source/JavaScriptCore/ChangeLog:86
> >> +           in some cases).
> > 
> > The DFG will indeed CSE these. It will also constant fold them if a class is only ever evaluated once (so the scope is only written to once).
> 
> in this patch it won't, we never really got that far with the optimization.
> But I agree it should be possible to do this better when we tier up.
> 
> >> Source/JavaScriptCore/ChangeLog:91
> >> +           cases, avoid adding new GetPutInfo complexity, etc
> > 
> > I guess I'm missing why this is actually needed. What about get|put_by_val doesn't work?
> 
> It's doable, however there are a number of reasons why I originally opted to
> go the other route:
> 
> It looks like get/put_by_value are mainly optimized for indexed property
> access (at least in baseline, not sure how they do after tiering up). They
> also handle a lot of different cases already,
> and private field support would add a number of other requirements, which
> would likely keep private fields permanently on the slow path, and
> dispatching to private field cases would have a slight impact
> on other code.
> Specifically, we'd need to dispatch for: private fields (distinct from
> existing PrivateIdentifier symbols, slightly different rules), brand
> checking (private methods/accessors), throwing if the field does not exist
> (or already exists in the DefineField case).
> 
> In addition to that, we'd probably need to wrap the private names in a
> JSCell, so that they can be fit in a JSValue, to avoid changing too much of
> the code. If we wound up using a raw UID and dispatching
> early based on a flag, then it doesn't really save any code. Allocating
> JSCells for each of these potentially eats up a lot of heap space if classes
> are evaluated multiple times (as is done in some of the mixin examples I've
> seen from JS developers).
> 
> We are happy to change this to use the existing opcodes before or after
> landing, but I think there are good reasons not to.
> 
> To summarize:
>   - avoid slowing down common operations by adding more dispatching
>   - avoid using up extra JS heap space by putting our UIDs in a JSCell
> wrapper (a new one for each field for each time the class is evaluated) for
> compat with op_get|put_by_val
>   - (very minor point) better developer ergonomics: easier to read
> "get_private_field #foo" vs the combination of ops that would be needed for
> the other cases
Ok. Let's keep them separate for now. If we have a good reason to change this in the future we can do that then.

> 
> >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4055
> >> +    case DefineFieldNode::PrivateName: {
> > 
> > Are these computed private names?
> > 
> > Anyways, in general, I have a question on computed private names. What are the semantics, of say:
> > class C {
> >    #["foo"] = 42
> >    #["foo"] = 44
> > }
> > 
> > Do I end up having two properties or one here?
> > 
> > Also, is there syntax to access private names in a computed fashion?
> 
> I believe this was answered by Dan earlier, but private names are never
> computed.
> 
> >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4057
> >> +        generator.emitPrivateFieldAdd(generator.thisRegister(), *m_ident, value.get());
> > 
> > Why isn't this just put_to_scope?
> 
> I'm not sure I understand how this would work with `put_to_scope` --- the
> method scope wouldn't be very useful, the class scope could be shared by
> multiple different instances (which might want to store different values in
> their private field).
> 
> >> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2596
> >> +    # (Can we be sure there's room in the existing backing store for non-inline fields?)
> > 
> > What's up with this comment? Did you figure it out?
> 
> IIRC (and it's been a while since I wrote this),
> `storePropertyAtVariableOffset` takes care of bailing out if the backing
> store is not big enough, which means we can probably get rid of the comment.
Comment 80 Saam Barati 2019-03-11 11:26:16 PDT
(In reply to Saam Barati from comment #73)
> Comment on attachment 360454 [details]
> Class fields, parser + bytecode + test changes
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360454&action=review
> 
> > JSTests/ChangeLog:21
> > +        * stress/async-await-syntax.js:
> 
> Some fly by feedback: am I reading this right that there are no new tests
> besides what’s in test262? If so, this patch should be adding a lot of JSC
> specific tests. Tests that you think stress interesting implementation
> details and how your patch interacts with the JIT/GC

I don't think this has been addressed yet. We need a lot more testing of this.
Comment 81 Saam Barati 2019-03-11 11:30:38 PDT
Hi Xan, can you comment on why the JSScope change is the way it is? I don't follow if there is a reason it has to be done that way opposed to just being statically part of the scope's inline fields.
Comment 82 Saam Barati 2019-03-11 11:31:43 PDT
(In reply to Saam Barati from comment #80)
> (In reply to Saam Barati from comment #73)
> > Comment on attachment 360454 [details]
> > Class fields, parser + bytecode + test changes
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=360454&action=review
> > 
> > > JSTests/ChangeLog:21
> > > +        * stress/async-await-syntax.js:
> > 
> > Some fly by feedback: am I reading this right that there are no new tests
> > besides what’s in test262? If so, this patch should be adding a lot of JSC
> > specific tests. Tests that you think stress interesting implementation
> > details and how your patch interacts with the JIT/GC
> 
> I don't think this has been addressed yet. We need a lot more testing of
> this.

Ignore me, I just realized this is the original patch that I left comments on. I'm going to set r- for now based on my earlier feedback and Yusuke's request.
Comment 83 Xan Lopez 2019-03-12 08:08:25 PDT
(In reply to Saam Barati from comment #81)
> Hi Xan, can you comment on why the JSScope change is the way it is? I don't
> follow if there is a reason it has to be done that way opposed to just being
> statically part of the scope's inline fields.

I think you are asking about what caitp commented on in comment #77; this either was necessary in an earlier iteration of the patch, and now does not, or we forgot the details. We'll try to simplify this and either include that in the next patch or get back to you with a proper explanation of why it's needed.
Comment 84 Xan Lopez 2019-04-03 02:00:46 PDT
Created attachment 366588 [details]
Class fields, parser + bytecode + test changes

Here's a new version of the patch. The relevant changes are:

- Added a runtime flag to disable the feature.
- Added a bunch of tests, including many from V8 (found some bugs thanks to these).
- Reduced UnlinkedFunctionExecutable size to remain below the desired size limit.
- Fixed a number of bugs.
- Fixed various style issues.
- Explained some additional design decisions in the ChangeLog (especially the rationale for all the private name hashes in SymbolTable/JSSymbolTableObject/VariableEnvironment).

The patch series with all the work can be found at: https://github.com/xanlpz/webkit/tree/class-fields-fixes
Comment 85 Caio Lima 2019-04-18 13:48:56 PDT
Created attachment 367749 [details]
Class fields, parser + byte code + test changes

I'm sending this because Xan is on holidays, but most of the Patch is an effort from Xan and Caitp, with some fixes that I've made.

There is a case where the implementation is not passing the test (see below), but we think it is ready for another batch of review.

```
{
  class C {
    x = Object.freeze(this);
    c = 42;
  }
  assertThrows(() => { new C; }, TypeError);
}
```
Comment 86 Build Bot 2019-04-18 13:52:55 PDT
Attachment 367749 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:204:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 61 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 87 Build Bot 2019-04-18 16:39:20 PDT
Comment on attachment 367749 [details]
Class fields, parser + byte code + test changes

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

New failing tests:
stress/class-fields-private-out-of-line.js.ftl-eager-no-cjit
Comment 88 Xan Lopez 2019-04-24 03:27:11 PDT
Created attachment 368117 [details]
Class fields, parser + bytecode + test changes

Patch refresh. Fix a style nit, punt failing new test, rebase to ToT.
Comment 89 Caitlin Potter (:caitp) 2019-05-02 09:27:27 PDT
I've been working on inlinining field initialization (getting rid of the synthetic function), and while it's mostly straight forward to do this, there are some things that are a bit awkward.

In order to perform field initialization during bytecode generation, that AST for each field needs to be present somewhere. This should be sensible enough. The problem: The source code for the instance fields is never present when parsing the constructor, or a nested arrow function or eval() that calls super() (the places where the information is needed).

To work around this, we need to run a separate parse phase with the class source, and add the result to the AST somewhere (I have done this by adding an extra SourceElements field to ScopeNode to hold instance field initialization code).

The main difficulty is having this separate parse phase produce an AST and identifiers that share the same lifetime as the original parse phase, in order to actually use that AST.

Several ways I imagine doing this:

1) do the separate parse phase before creating the final root node (which will require passing additional parameters) --- This might be a pain to extend for other uses later on, and is a bit gross.

2) move core methods of Parser into a base class, and pass a WTF::Function/std::function to parse() to run after parsing, taking the ParserBase interface as a parameter in order to set up different SourceCode / update the lexer, etc, all while still using the same arena.

3) Add a constructor to Parser which reuses the Arena from some existing ParseArenaRoot by swapping with it (it looks like ParserArena is relatively cheap to swap), and explicitly run the extra parse phase where it's needed, and swapping again upon finishing.

So, I was wondering if anyone had any preference for those ideas, or maybe have some alternative vision for doing it, or if we should forget about inlining the field initializer function in baseline anyways, and just let optimizing phases do it.
Comment 90 Caio Lima 2019-05-02 10:24:45 PDT
(In reply to Caitlin Potter (:caitp) from comment #89)
> 1) do the separate parse phase before creating the final root node (which
> will require passing additional parameters) --- This might be a pain to
> extend for other uses later on, and is a bit gross.
> 
> 2) move core methods of Parser into a base class, and pass a
> WTF::Function/std::function to parse() to run after parsing, taking the
> ParserBase interface as a parameter in order to set up different SourceCode
> / update the lexer, etc, all while still using the same arena.
> 
> 3) Add a constructor to Parser which reuses the Arena from some existing
> ParseArenaRoot by swapping with it (it looks like ParserArena is relatively
> cheap to swap), and explicitly run the extra parse phase where it's needed,
> and swapping again upon finishing.
> 
> So, I was wondering if anyone had any preference for those ideas, or maybe
> have some alternative vision for doing it, or if we should forget about
> inlining the field initializer function in baseline anyways, and just let
> optimizing phases do it.

I don't think that we should always fallback to "field initializer function", since it will be way slower than having `consturctor() { <fields_initialization> }`, maily when there is no JIT available.
IMHO, 3) sounds good, mainly if it is cheap to swap ParseArena. 2) sounds ok as well, but it maybe can make the code a little bit more complex than 3).
Comment 91 Saam Barati 2019-05-02 11:36:54 PDT
(In reply to Caitlin Potter (:caitp) from comment #89)
> I've been working on inlinining field initialization (getting rid of the
> synthetic function), and while it's mostly straight forward to do this,
> there are some things that are a bit awkward.
> 
> In order to perform field initialization during bytecode generation, that
> AST for each field needs to be present somewhere. This should be sensible
> enough. The problem: The source code for the instance fields is never
> present when parsing the constructor, or a nested arrow function or eval()
> that calls super() (the places where the information is needed).
> 
> To work around this, we need to run a separate parse phase with the class
> source, and add the result to the AST somewhere (I have done this by adding
> an extra SourceElements field to ScopeNode to hold instance field
> initialization code).
> 
> The main difficulty is having this separate parse phase produce an AST and
> identifiers that share the same lifetime as the original parse phase, in
> order to actually use that AST.
> 
> Several ways I imagine doing this:
> 
> 1) do the separate parse phase before creating the final root node (which
> will require passing additional parameters) --- This might be a pain to
> extend for other uses later on, and is a bit gross.
> 
> 2) move core methods of Parser into a base class, and pass a
> WTF::Function/std::function to parse() to run after parsing, taking the
> ParserBase interface as a parameter in order to set up different SourceCode
> / update the lexer, etc, all while still using the same arena.
> 
> 3) Add a constructor to Parser which reuses the Arena from some existing
> ParseArenaRoot by swapping with it (it looks like ParserArena is relatively
> cheap to swap), and explicitly run the extra parse phase where it's needed,
> and swapping again upon finishing.
> 
> So, I was wondering if anyone had any preference for those ideas, or maybe
> have some alternative vision for doing it, or if we should forget about
> inlining the field initializer function in baseline anyways, and just let
> optimizing phases do it.

I think we should definitely do the inlining. However, depending on what you think, I think it's a reasonable enhancement to do as a followup patch.
Comment 92 Caitlin Potter (:caitp) 2019-05-02 11:43:58 PDT
(In reply to Saam Barati from comment #91)
> I think we should definitely do the inlining. However, depending on what you
> think, I think it's a reasonable enhancement to do as a followup patch.

Do any of those designs seem like the right way to do it, or do you have a different idea?
Comment 93 Saam Barati 2019-05-02 11:59:41 PDT
(In reply to Caitlin Potter (:caitp) from comment #92)
> (In reply to Saam Barati from comment #91)
> > I think we should definitely do the inlining. However, depending on what you
> > think, I think it's a reasonable enhancement to do as a followup patch.
> 
> Do any of those designs seem like the right way to do it, or do you have a
> different idea?

We only need these field initializations when generating byte code for the constructor, right? Why not leave metadata around for the constructor function as to where each field is. When we go to reparse the constructor we can then also parse those field initializers? Or perhaps before parsing the constructor.

Maybe this is bad because it bakes in that we'll parse once with SyntaxChecker before using the ASTBuilder.

Finding a way to keep using the same arena seems like the right approach. I think an entrypoint for this thing doesn't sound crazy to me, but hopefully it wouldn't be that different than the pre-existing entry points. It'd just end up appending all these fields to some list of AST nodes.

I guess I like something like (2), but I'm not sure we really need a different class. I think we can just have an entrypoint into this type of a parse.
Comment 94 Caitlin Potter (:caitp) 2019-05-02 12:35:42 PDT
(In reply to Saam Barati from comment #93)
> (In reply to Caitlin Potter (:caitp) from comment #92)
> > (In reply to Saam Barati from comment #91)
> > > I think we should definitely do the inlining. However, depending on what you
> > > think, I think it's a reasonable enhancement to do as a followup patch.
> > 
> > Do any of those designs seem like the right way to do it, or do you have a
> > different idea?
> 
> We only need these field initializations when generating byte code for the
> constructor, right? Why not leave metadata around for the constructor
> function as to where each field is. When we go to reparse the constructor we
> can then also parse those field initializers? Or perhaps before parsing the
> constructor.

Not necessarily — It could be for an arrow function or direct eval inside the constructor.

> Maybe this is bad because it bakes in that we'll parse once with
> SyntaxChecker before using the ASTBuilder.
> 
> Finding a way to keep using the same arena seems like the right approach. I
> think an entrypoint for this thing doesn't sound crazy to me, but hopefully
> it wouldn't be that different than the pre-existing entry points. It'd just
> end up appending all these fields to some list of AST nodes.
> 
> I guess I like something like (2), but I'm not sure we really need a
> different class. I think we can just have an entrypoint into this type of a
> parse.

The reason you’d use a separate class, it would look something like this:

‘’’
rootNode = parse<NodeType>(vm, source, ..., [&](ParserBase& parser) {
  // After parsing the ParseNode root, before creating it
  initializer = parser.parseClassInstanceFields(classSource, instanceFieldPositions);
});
rootNode->setInstanceFieldInitializer(initializer);
‘’’

Having stuff in ParserBase means the callback doesn’t have to care about the lexer type.

But, it is still awkward. It’s challenging to find the right abstraction.
Comment 95 Xan Lopez 2019-05-07 09:37:42 PDT
Created attachment 369295 [details]
Class fields, parser + bytecode + test changes

New patch.
- Rebased against ToT.
- Some additional bug fixes.
Comment 96 Build Bot 2019-05-08 07:29:35 PDT
Comment on attachment 369295 [details]
Class fields, parser + bytecode + test changes

Attachment 369295 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12132977

New failing tests:
svg/repaint/remove-border-property-on-root.html
legacy-animation-engine/compositing/reflections/load-video-in-reflection.html
Comment 97 Build Bot 2019-05-08 07:29:38 PDT
Created attachment 369383 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 98 Xan Lopez 2019-06-04 02:39:42 PDT
Created attachment 371258 [details]
Class fields, parser + bytecode + test changes

New patch. Changes:

- Rebased against ToT.
- Only load and call @instanceFieldInitializer when needed.
- Fix last V8 test.
- Fix new.target and super property restrictions.
Comment 99 Build Bot 2019-06-04 04:05:49 PDT
Comment on attachment 371258 [details]
Class fields, parser + bytecode + test changes

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

New failing tests:
js/function-toString-vs-name.html
storage/indexeddb/result-request-cycle.html
inspector/console/command-line-api-getEventListeners.html
Comment 100 Build Bot 2019-06-04 04:05:51 PDT
Created attachment 371265 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 101 Build Bot 2019-06-04 04:14:20 PDT
Comment on attachment 371258 [details]
Class fields, parser + bytecode + test changes

Attachment 371258 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12371551

New failing tests:
inspector/console/command-line-api-getEventListeners.html
http/wpt/service-workers/service-worker-networkprocess-crash.html
storage/indexeddb/result-request-cycle.html
js/function-toString-vs-name.html
Comment 102 Build Bot 2019-06-04 04:14:23 PDT
Created attachment 371266 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 103 Build Bot 2019-06-04 04:36:08 PDT
Comment on attachment 371258 [details]
Class fields, parser + bytecode + test changes

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

New failing tests:
es6.yaml/es6/function_name_property_object_methods_function.js.default
stress/class-fields-harmony.js.bytecode-cache
stress/class-fields-private-cached-bytecode.js.bytecode-cache
stress/class-fields-private-use-eval.js.bytecode-cache
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-dfg-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-dfg-eager-no-cjit-validate-phases
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-no-ftl
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-ftl-no-cjit
stress/class-fields-function-name.js.bytecode-cache
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-no-llint
stress/class-fields-bytecode-cache.js.bytecode-cache
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-ftl-eager-no-cjit
apiTests
Comment 104 Build Bot 2019-06-04 04:45:09 PDT
Comment on attachment 371258 [details]
Class fields, parser + bytecode + test changes

Attachment 371258 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12371641

New failing tests:
js/function-toString-vs-name.html
storage/indexeddb/result-request-cycle.html
inspector/console/command-line-api-getEventListeners.html
Comment 105 Build Bot 2019-06-04 04:45:12 PDT
Created attachment 371267 [details]
Archive of layout-test-results from ews112 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 106 Build Bot 2019-06-04 04:52:08 PDT
Comment on attachment 371258 [details]
Class fields, parser + bytecode + test changes

Attachment 371258 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12371689

New failing tests:
storage/indexeddb/result-request-cycle.html
js/function-toString-vs-name.html
Comment 107 Build Bot 2019-06-04 04:52:11 PDT
Created attachment 371268 [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.14.5
Comment 108 Xan Lopez 2019-06-04 05:01:19 PDT
Created attachment 371270 [details]
Class fields, parser + bytecode + test changes

New patch.
- Try to fix fallout after https://bugs.webkit.org/show_bug.cgi?id=197979
Comment 109 Build Bot 2019-06-04 06:28:10 PDT
Comment on attachment 371270 [details]
Class fields, parser + bytecode + test changes

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

New failing tests:
js/function-toString-vs-name.html
storage/indexeddb/result-request-cycle.html
inspector/console/command-line-api-getEventListeners.html
Comment 110 Build Bot 2019-06-04 06:28:13 PDT
Created attachment 371272 [details]
Archive of layout-test-results from ews100 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 111 Build Bot 2019-06-04 06:36:18 PDT
Comment on attachment 371270 [details]
Class fields, parser + bytecode + test changes

Attachment 371270 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12372265

New failing tests:
inspector/console/command-line-api-getEventListeners.html
http/wpt/service-workers/service-worker-networkprocess-crash.html
storage/indexeddb/result-request-cycle.html
js/function-toString-vs-name.html
Comment 112 Build Bot 2019-06-04 06:36:21 PDT
Created attachment 371273 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 113 Build Bot 2019-06-04 06:55:45 PDT
Comment on attachment 371270 [details]
Class fields, parser + bytecode + test changes

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

New failing tests:
es6.yaml/es6/function_name_property_object_methods_function.js.default
stress/class-fields-harmony.js.bytecode-cache
stress/class-fields-private-cached-bytecode.js.bytecode-cache
stress/class-fields-private-use-eval.js.bytecode-cache
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-dfg-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-no-ftl
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-ftl-no-cjit
stress/class-fields-function-name.js.bytecode-cache
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-no-llint
stress/class-fields-bytecode-cache.js.bytecode-cache
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-ftl-eager-no-cjit
apiTests
Comment 114 Build Bot 2019-06-04 07:04:17 PDT
Comment on attachment 371270 [details]
Class fields, parser + bytecode + test changes

Attachment 371270 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12372286

New failing tests:
js/function-toString-vs-name.html
storage/indexeddb/result-request-cycle.html
inspector/console/command-line-api-getEventListeners.html
Comment 115 Build Bot 2019-06-04 07:04:20 PDT
Created attachment 371276 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 116 Build Bot 2019-06-04 07:14:04 PDT
Comment on attachment 371270 [details]
Class fields, parser + bytecode + test changes

Attachment 371270 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12372296

New failing tests:
storage/indexeddb/result-request-cycle.html
js/function-toString-vs-name.html
Comment 117 Build Bot 2019-06-04 07:14:07 PDT
Created attachment 371277 [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.14.5
Comment 118 Build Bot 2019-06-04 07:27:14 PDT
Comment on attachment 371270 [details]
Class fields, parser + bytecode + test changes

Attachment 371270 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12372416

New failing tests:
editing/style/apply-style-iframe-crash.html
editing/style/iframe-onload-crash-mac.html
js/function-toString-vs-name.html
Comment 119 Build Bot 2019-06-04 07:27:19 PDT
Created attachment 371278 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 120 jsc-armv7 EWS 2019-06-04 10:38:08 PDT
Comment on attachment 371270 [details]
Class fields, parser + bytecode + test changes

Attachment 371270 [details] did not pass jsc-armv7-ews (jsc-only):
Output: https://webkit-queues.webkit.org/results/12373585

New failing tests:
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-dfg-eager-no-cjit
es6.yaml/es6/function_name_property_object_methods_function.js.default
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/function-toString-vs-name.js.layout-no-llint
apiTests
Comment 121 Xan Lopez 2019-06-05 01:44:04 PDT
Created attachment 371383 [details]
Class fields, parser + bytecode + test changes

New patch. Try to fix previous failures in bots (code cache and toString bugs).
Comment 122 Xan Lopez 2019-06-05 02:26:03 PDT
Created attachment 371384 [details]
Class fields, parser + bytecode + test changes

Rebased again against ToT.
Comment 123 Xan Lopez 2019-06-05 04:45:59 PDT
Created attachment 371387 [details]
Class fields, parser + bytecode + test changes

New patch.

Bots seem to be green now, but merge one more fix.
Comment 124 Build Bot 2019-06-05 07:11:40 PDT
Comment on attachment 371387 [details]
Class fields, parser + bytecode + test changes

Attachment 371387 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12383797

New failing tests:
editing/style/iframe-onload-crash-mac.html
editing/style/apply-style-iframe-crash.html
Comment 125 Build Bot 2019-06-05 07:11:44 PDT
Created attachment 371392 [details]
Archive of layout-test-results from ews214 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews214  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit