Bug 174212

Summary:

[JSC] Add support for instance class fields

Product:

WebKit

Reporter:

Xan Lopez <xan.lopez>

Component:

JavaScriptCore

Assignee:

Caitlin Potter (:caitp) <caitp>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

709922234, annulen, bugs-noreply, caitp, calvaris, chi187, commit-queue, dominik.infuehr, ews-watchlist, fred.wang, guijemont, guijemont+jsc-armv7-ews, gyuyoung.kim, keith_miller, littledan, mark.lam, mike, mjs, msaboff, rakuco, rniwa, ross.kirsling, ryuan.choi, saam, sergio, ticaiolima, timothy, tzagallo, webkit-bug-importer, xan.lopez, ysuzuki

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

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

Bug Depends on:

Bug Blocks:

195619, 206174, 206663, 194095, 194434, 194435, 206431

Attachments:

Description	Flags
WIP data field parsing	none
Data fields WIP	none
Patch	none
Instance class fields	ews-watchlist: commit-queue-
Archive of layout-test-results from ews201 for win-future	none
instance class fields (09/05/18)	ews-watchlist: commit-queue-
Archive of layout-test-results from ews201 for win-future	none
Class fields WIP	none
Class fields WIP	ews-watchlist: 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-watchlist: 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	saam: review-
Class fields, parser + bytecode + test changes	none
Class fields, parser + byte code + test changes	ews-watchlist: commit-queue-
Class fields, parser + bytecode + test changes	none
Class fields, parser + bytecode + test changes	ews-watchlist: commit-queue-
Archive of layout-test-results from ews210 for win-future	none
Class fields, parser + bytecode + test changes	ews-watchlist: 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-watchlist: 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	none
Archive of layout-test-results from ews214 for win-future	none
Class fields, parser + bytecode + test changes	ews-watchlist: commit-queue-
Archive of layout-test-results from ews215 for win-future	none
Class fields, parser + bytecode + test changes	none
Archive of layout-test-results from ews210 for win-future	none
Class fields, parser + bytecode + JIT + test changes	none
Class fields, parser + bytecode + JIT + test changes	none
hopefully fix llint32_64	none
Patch for landing, hopefully	none
Archive of layout-test-results from ews215 for win-future	none
Patch for thoroughly scrutinizing	none
Archive of layout-test-results from ews212 for win-future	none
Patch for Thoroughly Scrutinizing	none
Patch for Thoroughly Scrutinizing	none
Patch for Thoroughly Scrutinizing	none
Patch for Thoroughly Scrutinizing	none
Part 1: Public Instance Fields	none
Part 2: Private Instance Fields	none
Part 1: Public Instance Fields (rev2)	none
Part 2: Private Instance Fields (rev2)	none
Part 1: Public Instance Fields (rev3)	ysuzuki: review-
Part 2: Private Instance Fields (rev3)	none
Part 1: Public Instance Fields (rev4)	none
Part 1: Public Instance Fields (rev4)	ysuzuki: review+
Part 2: Private Instance Fields (rev4)	none
Part 1: Public Instance Fields (patch for landing)	none
Part 1: Public Instance Fields (patch for landing)	none
Part 2: Private Instance Fields (rev5)	none

Xan Lopez

Reported 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 }

Attachments
WIP data field parsing (5.66 KB, patch) 2017-07-06 11:19 PDT, Xan Lopez	no flags	Details Formatted Diff Diff
Data fields WIP (23.40 KB, patch) 2017-11-20 05:55 PST, Xan Lopez	no flags	Details Formatted Diff Diff
Patch (35.87 KB, patch) 2018-03-16 12:25 PDT, Caitlin Potter (:caitp)	no flags	Details Formatted Diff Diff
Instance class fields (150.26 KB, patch) 2018-07-03 02:50 PDT, Xan Lopez	ews-watchlist: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews201 for win-future (12.85 MB, application/zip) 2018-07-03 04:54 PDT, EWS Watchlist	no flags	Details
instance class fields (09/05/18) (171.95 KB, patch) 2018-09-05 03:05 PDT, Xan Lopez	ews-watchlist: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews201 for win-future (12.82 MB, application/zip) 2018-09-05 05:01 PDT, EWS Watchlist	no flags	Details
Class fields WIP (176.05 KB, patch) 2018-09-26 13:42 PDT, Xan Lopez	no flags	Details Formatted Diff Diff
Class fields WIP (176.03 KB, patch) 2018-09-26 13:50 PDT, Xan Lopez	ews-watchlist: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews102 for mac-sierra (2.35 MB, application/zip) 2018-09-26 15:28 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.73 MB, application/zip) 2018-09-26 16:38 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews112 for mac-sierra (3.39 MB, application/zip) 2018-09-26 16:45 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews202 for win-future (12.82 MB, application/zip) 2018-09-26 16:50 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews113 for mac-sierra (3.49 MB, application/zip) 2018-09-26 18:46 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.30 MB, application/zip) 2018-09-27 08:32 PDT, EWS Watchlist	no flags	Details
Class fields (10/01) (181.54 KB, patch) 2018-10-01 10:32 PDT, Xan Lopez	ews-watchlist: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.15 MB, application/zip) 2018-10-01 12:37 PDT, EWS Watchlist	no flags	Details
Class fields WIP (10/02) (183.06 KB, patch) 2018-10-02 01:22 PDT, Xan Lopez	no flags	Details Formatted Diff Diff
Class fields, parser + bytecode + test changes (184.39 KB, patch) 2018-11-02 07:16 PDT, Xan Lopez	no flags	Details Formatted Diff Diff
Class fields, parser + bytecode + test changes (184.40 KB, patch) 2018-11-02 07:26 PDT, Xan Lopez	no flags	Details Formatted Diff Diff
Class fields, parser + bytecode + test changes (184.41 KB, patch) 2018-11-02 07:30 PDT, Xan Lopez	no flags	Details Formatted Diff Diff
Class fields, parser + bytecode + test changes (184.41 KB, patch) 2018-11-05 03:50 PST, Xan Lopez	no flags	Details Formatted Diff Diff
Class fields, parser + bytecode + test changes (184.40 KB, patch) 2018-11-05 03:57 PST, Xan Lopez	no flags	Details Formatted Diff Diff
Class fields, parser + bytecode + test changes (184.44 KB, patch) 2018-11-05 06:18 PST, Xan Lopez	no flags	Details Formatted Diff Diff
Class fields, parser + bytecode + test changes (184.39 KB, patch) 2018-11-06 02:06 PST, Xan Lopez	no flags	Details Formatted Diff Diff
Placeholder baseline JIT for private field opcodes (4.32 KB, patch) 2018-12-05 02:55 PST, Xan Lopez	no flags	Details Formatted Diff Diff
Placeholder baseline JIT for private field opcodes (4.32 KB, patch) 2018-12-05 03:37 PST, Xan Lopez	no flags	Details Formatted Diff Diff
Class fields, parser + bytecode + test changes (189.64 KB, patch) 2019-01-29 02:10 PST, Xan Lopez	saam: review-	Details Formatted Diff Diff
Class fields, parser + bytecode + test changes (232.91 KB, patch) 2019-04-03 02:00 PDT, Xan Lopez	no flags	Details Formatted Diff Diff
Class fields, parser + byte code + test changes (209.93 KB, patch) 2019-04-18 13:48 PDT, Caio Lima	ews-watchlist: commit-queue-	Details Formatted Diff Diff
Class fields, parser + bytecode + test changes (224.38 KB, patch) 2019-04-24 03:27 PDT, Xan Lopez	no flags	Details Formatted Diff Diff
Class fields, parser + bytecode + test changes (226.00 KB, patch) 2019-05-07 09:37 PDT, Xan Lopez	ews-watchlist: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews210 for win-future (13.50 MB, application/zip) 2019-05-08 07:29 PDT, EWS Watchlist	no flags	Details
Class fields, parser + bytecode + test changes (266.30 KB, patch) 2019-06-04 02:39 PDT, Xan Lopez	ews-watchlist: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews101 for mac-highsierra (3.09 MB, application/zip) 2019-06-04 04:05 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (2.83 MB, application/zip) 2019-06-04 04:14 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews112 for mac-highsierra (2.90 MB, application/zip) 2019-06-04 04:45 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.55 MB, application/zip) 2019-06-04 04:52 PDT, EWS Watchlist	no flags	Details
Class fields, parser + bytecode + test changes (266.31 KB, patch) 2019-06-04 05:01 PDT, Xan Lopez	ews-watchlist: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews100 for mac-highsierra (3.10 MB, application/zip) 2019-06-04 06:28 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.81 MB, application/zip) 2019-06-04 06:36 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews117 for mac-highsierra (2.96 MB, application/zip) 2019-06-04 07:04 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.49 MB, application/zip) 2019-06-04 07:14 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews213 for win-future (13.49 MB, application/zip) 2019-06-04 07:27 PDT, EWS Watchlist	no flags	Details
Class fields, parser + bytecode + test changes (269.29 KB, patch) 2019-06-05 01:44 PDT, Xan Lopez	no flags	Details Formatted Diff Diff
Class fields, parser + bytecode + test changes (269.26 KB, patch) 2019-06-05 02:26 PDT, Xan Lopez	no flags	Details Formatted Diff Diff
Class fields, parser + bytecode + test changes (269.23 KB, patch) 2019-06-05 04:45 PDT, Xan Lopez	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews214 for win-future (13.50 MB, application/zip) 2019-06-05 07:11 PDT, EWS Watchlist	no flags	Details
Class fields, parser + bytecode + test changes (268.82 KB, patch) 2019-06-28 06:37 PDT, Xan Lopez	ews-watchlist: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews215 for win-future (13.55 MB, application/zip) 2019-06-28 08:27 PDT, EWS Watchlist	no flags	Details
Class fields, parser + bytecode + test changes (268.88 KB, patch) 2019-06-28 12:39 PDT, Xan Lopez	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews210 for win-future (13.62 MB, application/zip) 2019-06-28 15:16 PDT, EWS Watchlist	no flags	Details
Class fields, parser + bytecode + JIT + test changes (304.35 KB, patch) 2019-10-07 13:35 PDT, Caitlin Potter (:caitp)	no flags	Details Formatted Diff Diff
Class fields, parser + bytecode + JIT + test changes (308.87 KB, patch) 2019-10-09 12:04 PDT, Caitlin Potter (:caitp)	no flags	Details Formatted Diff Diff
hopefully fix llint32_64 (304.37 KB, patch) 2019-10-09 13:25 PDT, Caitlin Potter (:caitp)	no flags	Details Formatted Diff Diff
Patch for landing, hopefully (305.20 KB, patch) 2019-10-11 08:45 PDT, Caitlin Potter (:caitp)	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews215 for win-future (14.41 MB, application/zip) 2019-10-12 05:14 PDT, EWS Watchlist	no flags	Details
Patch for thoroughly scrutinizing (306.51 KB, patch) 2019-10-21 12:09 PDT, Caitlin Potter (:caitp)	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews212 for win-future (13.79 MB, application/zip) 2019-10-21 14:32 PDT, EWS Watchlist	no flags	Details
Patch for Thoroughly Scrutinizing (309.63 KB, patch) 2019-10-23 12:34 PDT, Caitlin Potter (:caitp)	no flags	Details Formatted Diff Diff
Patch for Thoroughly Scrutinizing (316.84 KB, patch) 2019-10-30 08:22 PDT, Caitlin Potter (:caitp)	no flags	Details Formatted Diff Diff
Patch for Thoroughly Scrutinizing (306.18 KB, patch) 2019-10-30 09:04 PDT, Caitlin Potter (:caitp)	no flags	Details Formatted Diff Diff
Patch for Thoroughly Scrutinizing (306.20 KB, patch) 2019-11-13 10:22 PST, Caitlin Potter (:caitp)	no flags	Details Formatted Diff Diff
Part 1: Public Instance Fields (157.56 KB, patch) 2019-11-26 08:41 PST, Caitlin Potter (:caitp)	no flags	Details Formatted Diff Diff
Part 2: Private Instance Fields (160.33 KB, patch) 2019-11-26 08:49 PST, Caitlin Potter (:caitp)	no flags	Details Formatted Diff Diff
Part 1: Public Instance Fields (rev2) (156.08 KB, patch) 2019-12-05 11:37 PST, Caitlin Potter (:caitp)	no flags	Details Formatted Diff Diff
Part 2: Private Instance Fields (rev2) (160.32 KB, patch) 2019-12-05 11:48 PST, Caitlin Potter (:caitp)	no flags	Details Formatted Diff Diff
Part 1: Public Instance Fields (rev3) (164.31 KB, patch) 2019-12-12 07:59 PST, Caitlin Potter (:caitp)	ysuzuki: review-	Details Formatted Diff Diff
Part 2: Private Instance Fields (rev3) (160.16 KB, patch) 2019-12-12 08:20 PST, Caitlin Potter (:caitp)	no flags	Details Formatted Diff Diff
Part 1: Public Instance Fields (rev4) (160.55 KB, patch) 2020-01-10 11:34 PST, Caitlin Potter (:caitp)	no flags	Details Formatted Diff Diff
Part 1: Public Instance Fields (rev4) (160.43 KB, patch) 2020-01-10 12:06 PST, Caitlin Potter (:caitp)	ysuzuki: review+	Details Formatted Diff Diff
Part 2: Private Instance Fields (rev4) (160.42 KB, patch) 2020-01-10 12:17 PST, Caitlin Potter (:caitp)	no flags	Details Formatted Diff Diff
Part 1: Public Instance Fields (patch for landing) (160.94 KB, patch) 2020-01-15 12:15 PST, Caitlin Potter (:caitp)	no flags	Details Formatted Diff Diff
Part 1: Public Instance Fields (patch for landing) (160.94 KB, patch) 2020-01-15 12:51 PST, Caitlin Potter (:caitp)	no flags	Details Formatted Diff Diff
Part 2: Private Instance Fields (rev5) (168.21 KB, patch) 2020-01-16 09:59 PST, Caitlin Potter (:caitp)	no flags	Details Formatted Diff Diff
Show Obsolete (74) View All Add attachment proposed patch, testcase, etc.

Caitlin Potter (:caitp)

Comment 1 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.

Caitlin Potter (:caitp)

Comment 2 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

Saam Barati

Comment 3 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.

Yusuke Suzuki

Comment 4 2017-07-06 22:02:38 PDT

I guess ASI is Automatic Semicolon Insertion.

Xan Lopez

Comment 5 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.

Yusuke Suzuki

Comment 6 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

Xan Lopez

Comment 7 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.

Xan Lopez

Comment 8 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.

Xan Lopez

Comment 9 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.

Caitlin Potter (:caitp)

Comment 10 2018-03-16 12:25:53 PDT

Created attachment 335960 [details] Patch

EWS Watchlist

Comment 11 2018-03-16 12:28:27 PDT Comment hidden (obsolete)

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.

Caitlin Potter (:caitp)

Comment 12 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.

Caitlin Potter (:caitp)

Comment 13 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.

Caitlin Potter (:caitp)

Comment 14 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()

Caitlin Potter (:caitp)

Comment 15 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.

Xan Lopez

Comment 16 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.

EWS Watchlist

Comment 17 2018-07-03 02:54:42 PDT Comment hidden (obsolete)

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.

EWS Watchlist

Comment 18 2018-07-03 04:53:51 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 19 2018-07-03 04:54:02 PDT Comment hidden (obsolete)

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

Caitlin Potter (:caitp)

Comment 20 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.

Caitlin Potter (:caitp)

Comment 21 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.

Xan Lopez

Comment 22 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.

Xan Lopez

Comment 23 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

EWS Watchlist

Comment 24 2018-09-05 03:08:43 PDT Comment hidden (obsolete)

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.

EWS Watchlist

Comment 25 2018-09-05 05:01:47 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 26 2018-09-05 05:01:59 PDT Comment hidden (obsolete)

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

Xan Lopez

Comment 27 2018-09-26 13:42:43 PDT

Created attachment 350893 [details] Class fields WIP

EWS Watchlist

Comment 28 2018-09-26 13:46:19 PDT Comment hidden (obsolete)

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.

Xan Lopez

Comment 29 2018-09-26 13:50:30 PDT

Created attachment 350896 [details] Class fields WIP

Xan Lopez

Comment 30 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.

EWS Watchlist

Comment 31 2018-09-26 15:20:56 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 32 2018-09-26 15:28:48 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 33 2018-09-26 15:28:50 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 34 2018-09-26 16:38:54 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 35 2018-09-26 16:38:56 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 36 2018-09-26 16:45:20 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 37 2018-09-26 16:45:22 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 38 2018-09-26 16:50:03 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 39 2018-09-26 16:50:15 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 40 2018-09-26 18:46:02 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 41 2018-09-26 18:46:04 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 42 2018-09-27 08:32:37 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 43 2018-09-27 08:32:39 PDT Comment hidden (obsolete)

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

Xan Lopez

Comment 44 2018-10-01 10:32:23 PDT

Created attachment 351261 [details] Class fields (10/01)

EWS Watchlist

Comment 45 2018-10-01 12:07:25 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 46 2018-10-01 12:37:29 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 47 2018-10-01 12:37:31 PDT Comment hidden (obsolete)

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

Xan Lopez

Comment 48 2018-10-02 01:22:28 PDT

Created attachment 351354 [details] Class fields WIP (10/02)

Michael Catanzaro

Comment 49 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 :)

Daniel Ehrenberg

Comment 50 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.

Michael Catanzaro

Comment 51 2018-10-07 12:13:59 PDT

Comment on attachment 351354 [details] Class fields WIP (10/02) Removing r? per Dan's comment.

Xan Lopez

Comment 52 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.

Xan Lopez

Comment 53 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.

Xan Lopez

Comment 54 2018-11-02 07:26:09 PDT

Created attachment 353698 [details] Class fields, parser + bytecode + test changes

Xan Lopez

Comment 55 2018-11-02 07:30:23 PDT

Created attachment 353699 [details] Class fields, parser + bytecode + test changes

Xan Lopez

Comment 56 2018-11-05 03:50:37 PST

Created attachment 353848 [details] Class fields, parser + bytecode + test changes

Xan Lopez

Comment 57 2018-11-05 03:57:27 PST

Created attachment 353849 [details] Class fields, parser + bytecode + test changes

Dominik Inführ

Comment 58 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.

Xan Lopez

Comment 59 2018-11-05 06:18:46 PST

Created attachment 353857 [details] Class fields, parser + bytecode + test changes

Xan Lopez

Comment 60 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.

Xan Lopez

Comment 61 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.

Caitlin Potter (:caitp)

Comment 62 2018-12-04 08:43:23 PST

This is ready for review

Caio Lima

Comment 63 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 ```

Xan Lopez

Comment 64 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.

EWS Watchlist

Comment 65 2018-12-05 02:58:43 PST Comment hidden (obsolete)

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.

Xan Lopez

Comment 66 2018-12-05 03:37:26 PST

Created attachment 356595 [details] Placeholder baseline JIT for private field opcodes Fix style.

Saam Barati

Comment 67 2019-01-20 14:50:51 PST

Do you still need a review for this? Why are there two different patches?

Xan Lopez

Comment 68 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.

Xan Lopez

Comment 69 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.

Caitlin Potter (:caitp)

Comment 70 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)

Caio Lima

Comment 71 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.

Maciej Stachowiak

Comment 72 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).

Saam Barati

Comment 73 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

Saam Barati

Comment 74 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.

Daniel Ehrenberg

Comment 75 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

Yusuke Suzuki

Comment 76 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.

Caitlin Potter (:caitp)

Comment 77 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.

Xan Lopez

Comment 78 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.

Saam Barati

Comment 79 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.

Saam Barati

Comment 80 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.

Saam Barati

Comment 81 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.

Saam Barati

Comment 82 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.

Xan Lopez

Comment 83 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.

Xan Lopez

Comment 84 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

Caio Lima

Comment 85 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); } ```

EWS Watchlist

Comment 86 2019-04-18 13:52:55 PDT Comment hidden (obsolete)

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.

EWS Watchlist

Comment 87 2019-04-18 16:39:20 PDT Comment hidden (obsolete)

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

Xan Lopez

Comment 88 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.

Caitlin Potter (:caitp)

Comment 89 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.

Caio Lima

Comment 90 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).

Saam Barati

Comment 91 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.

Caitlin Potter (:caitp)

Comment 92 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?

Saam Barati

Comment 93 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.

Caitlin Potter (:caitp)

Comment 94 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.

Xan Lopez

Comment 95 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.

EWS Watchlist

Comment 96 2019-05-08 07:29:35 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 97 2019-05-08 07:29:38 PDT Comment hidden (obsolete)

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

Xan Lopez

Comment 98 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.

EWS Watchlist

Comment 99 2019-06-04 04:05:49 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 100 2019-06-04 04:05:51 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 101 2019-06-04 04:14:20 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 102 2019-06-04 04:14:23 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 103 2019-06-04 04:36:08 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 104 2019-06-04 04:45:09 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 105 2019-06-04 04:45:12 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 106 2019-06-04 04:52:08 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 107 2019-06-04 04:52:11 PDT Comment hidden (obsolete)

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

Xan Lopez

Comment 108 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

EWS Watchlist

Comment 109 2019-06-04 06:28:10 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 110 2019-06-04 06:28:13 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 111 2019-06-04 06:36:18 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 112 2019-06-04 06:36:21 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 113 2019-06-04 06:55:45 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 114 2019-06-04 07:04:17 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 115 2019-06-04 07:04:20 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 116 2019-06-04 07:14:04 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 117 2019-06-04 07:14:07 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 118 2019-06-04 07:27:14 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 119 2019-06-04 07:27:19 PDT Comment hidden (obsolete)

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

jsc-armv7 EWS

Comment 120 2019-06-04 10:38:08 PDT Comment hidden (obsolete)

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

Xan Lopez

Comment 121 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).

Xan Lopez

Comment 122 2019-06-05 02:26:03 PDT

Created attachment 371384 [details] Class fields, parser + bytecode + test changes Rebased again against ToT.

Xan Lopez

Comment 123 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.

EWS Watchlist

Comment 124 2019-06-05 07:11:40 PDT Comment hidden (obsolete)

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

EWS Watchlist

Comment 125 2019-06-05 07:11:44 PDT Comment hidden (obsolete)

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

Xan Lopez

Comment 126 2019-06-28 06:37:51 PDT

Created attachment 373112 [details] Class fields, parser + bytecode + test changes New patch: - Rebased against ToT (28/06/19)

EWS Watchlist

Comment 127 2019-06-28 08:27:18 PDT Comment hidden (obsolete)

Comment on attachment 373112 [details] Class fields, parser + bytecode + test changes Attachment 373112 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12603104 New failing tests: editing/style/iframe-onload-crash-mac.html editing/style/apply-style-iframe-crash.html

EWS Watchlist

Comment 128 2019-06-28 08:27:21 PDT Comment hidden (obsolete)

Created attachment 373117 [details] Archive of layout-test-results from ews215 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews215 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

EWS Watchlist

Comment 129 2019-06-28 08:42:10 PDT Comment hidden (obsolete)

Comment on attachment 373112 [details] Class fields, parser + bytecode + test changes Attachment 373112 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12603024 New failing tests: stress/regress-181388.js.bytecode-cache stress/super-property-access-object-literal-to-this.js.bytecode-cache stress/modules-syntax-error.js.bytecode-cache stress/initialize_functions_after_arguments.js.bytecode-cache stress/regress-185896.js.bytecode-cache stress/get-by-id-direct.js.bytecode-cache stress/lexical-let-not-strict-mode.js.bytecode-cache stress/closure-recursive-tail-call-infinite-loop.js.bytecode-cache stress/module-namespace-access-poly.js.bytecode-cache stress/arith-nodes-abstract-interpreter-untypeduse.js.bytecode-cache microbenchmarks/captured-assignments.js.bytecode-cache stress/class-syntax-tdz-in-eval.js.bytecode-cache stress/proxy-get-prototype-of.js.bytecode-cache stress/proxy-all-the-parameters.js.bytecode-cache stress/reserved-word-with-escape.js.bytecode-cache microbenchmarks/raytrace-with-empty-try-catch.js.bytecode-cache microbenchmarks/eval-cached.js.bytecode-cache stress/activation-sink-default-value-tdz-error.js.bytecode-cache stress/sampling-profiler-microtasks.js.bytecode-cache stress/activation-sink.js.bytecode-cache stress/arrowfunction-activation-sink-osrexit.js.bytecode-cache stress/class-fields-private-use-eval.js.bytecode-cache stress/value-div-ai-rule.js.bytecode-cache stress/object-number-properties.js.bytecode-cache stress/rest-parameter-basics.js.bytecode-cache microbenchmarks/arrowfunciton-reference-arguments.js.bytecode-cache stress/eval-func-decl-in-frozen-global.js.bytecode-cache stress/async-iteration-syntax.js.bytecode-cache microbenchmarks/generate-multiple-llint-entrypoints.js.bytecode-cache stress/type-check-hoisting-phase-hoist-check-structure-on-tdz-this-value.js.bytecode-cache stress/proxy-revoke.js.bytecode-cache stress/symbol-tostringtag.js.bytecode-cache stress/for-in-invalidation-for-any-write.js.bytecode-cache stress/eval-in-arrow-function.js.bytecode-cache stress/dfg-object-proto-accessor.js.bytecode-cache stress/proxy-basic.js.bytecode-cache microbenchmarks/getter-no-activation.js.bytecode-cache stress/sloppy-mode-hoist-arguments-function-non-simple-parameter-list.js.bytecode-cache stress/raise-error-in-iterator-close.js.bytecode-cache microbenchmarks/data-view-accesses-2.js.bytecode-cache stress/constant-closure-var-with-dynamic-invalidation.js.bytecode-cache stress/function-with-defaults-inlining.js.bytecode-cache stress/object-is-fold-to-compare-eq-ptr.js.bytecode-cache stress/async-await-mozilla.js.bytecode-cache stress/code-cache-incorrect-caching.js.bytecode-cache stress/istypedarrayview-intrinsic.js.bytecode-cache sunspider-1.0/3d-raytrace.js.bytecode-cache microbenchmarks/large-int-captured.js.bytecode-cache stress/arrowfunction-lexical-bind-supercall-4.js.bytecode-cache stress/activation-sink-osrexit-default-value-tdz-error.js.bytecode-cache stress/arrowfunction-lexical-bind-arguments-non-strict-1.js.bytecode-cache stress/spec-empty-flows-through-cell-checks.js.bytecode-cache stress/typedarray-sort.js.bytecode-cache microbenchmarks/singleton-scope.js.bytecode-cache stress/tricky-infinite-loop-that-uses-captured-variables.js.bytecode-cache stress/heap-allocator-allocates-incorrect-size-for-activation.js.bytecode-cache stress/class-fields-harmony.js.bytecode-cache stress/proxy-is-extensible.js.bytecode-cache stress/string-raw.js.bytecode-cache microbenchmarks/is-not-cell-speculation-for-empty-value.js.bytecode-cache stress/arrowfunction-tdz-4.js.bytecode-cache stress/generator-name.js.bytecode-cache stress/domjit-exception-ic.js.bytecode-cache stress/phantom-new-array-buffer-forward-varargs.js.bytecode-cache stress/adhoc-setter-frame-should-emit-setlocal-again.js.bytecode-cache stress/proxy-own-keys.js.bytecode-cache stress/generator-eval-this.js.bytecode-cache stress/constant-folding-phase-insert-check-handle-varargs.js.bytecode-cache stress/class-method-does-not-declare-variable-to-upper-scope.js.bytecode-cache stress/proxy-with-private-symbols.js.bytecode-cache stress/to-number-throws-correct-exception.js.bytecode-cache stress/eval-func-decl-block-scoping-reassign.js.bytecode-cache stress/value-add-on-double-array-with-holes.js.bytecode-cache stress/arrowfunction-run-10-2.js.bytecode-cache stress/arrowfunction-lexical-this-sinking-osrexit.js.bytecode-cache stress/allocation-sinking-puthint-control-flow.js.bytecode-cache stress/put-indexed-getter-setter.js.bytecode-cache stress/dataview-jit-bounds-checks.js.bytecode-cache stress/object-rest-deconstruct.js.bytecode-cache stress/arrowfunction-lexical-bind-newtarget.js.bytecode-cache stress/string-repeat-edge-cases.js.bytecode-cache stress/typedarray-find.js.bytecode-cache stress/regress-153486.js.bytecode-cache microbenchmarks/infer-one-time-closure-ten-vars.js.bytecode-cache stress/sorting-boolean-result-comparator.js.bytecode-cache stress/generator-fib-ftl-and-string.js.bytecode-cache stress/function-configurable-properties.js.bytecode-cache stress/create-subclass-structure-may-throw-exception-when-getting-prototype.js.bytecode-cache stress/function-with-defaults-non-inlining.js.bytecode-cache stress/proxy-get-own-property.js.bytecode-cache microbenchmarks/sorting-benchmark.js.bytecode-cache stress/error-messages-for-in-operator-should-not-crash.js.bytecode-cache stress/template-literal-line-terminators.js.bytecode-cache stress/create-error-out-of-memory-rope-string.js.bytecode-cache stress/template-literal.js.bytecode-cache stress/proxy-instanceof.js.bytecode-cache stress/for-in-of-const.js.bytecode-cache stress/activation-sink-osrexit-default-value.js.bytecode-cache stress/typedarray-configure-index.js.bytecode-cache stress/rest-parameter-is-destructuring.js.bytecode-cache mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-baseline stress/yield-label-generator.js.bytecode-cache stress/compare-number-and-other.js.bytecode-cache sunspider-1.0/string-tagcloud.js.bytecode-cache stress/spread-capture-rest.js.bytecode-cache stress/ai-create-this-to-new-object-fire.js.bytecode-cache stress/scoped-arguments-out-of-bounds-change-structure.js.bytecode-cache stress/materialize-activation-referenced-from-phantom-function.js.bytecode-cache stress/force-exit-then-eval-dfg.js.bytecode-cache stress/dataview-jit-set.js.bytecode-cache stress/eval-func-decl-in-eval-within-block-with-let.js.bytecode-cache microbenchmarks/infer-one-time-closure.js.bytecode-cache microbenchmarks/infer-closure-const-then-reenter.js.bytecode-cache stress/infinite-loop-that-uses-captured-variables-but-they-do-not-escape.js.bytecode-cache stress/array-fill-put-by-val.js.bytecode-cache mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-dfg-eager-no-cjit-validate-phases stress/regexp-syntax-error-invalid-flags.js.bytecode-cache stress/rest-elements.js.bytecode-cache stress/inline-closure-call.js.bytecode-cache microbenchmarks/string-rope-with-object.js.bytecode-cache stress/proxy-underscore-proto.js.bytecode-cache stress/async-arrow-functions-lexical-this-binding.js.bytecode-cache stress/proxy-delete.js.bytecode-cache stress/proxy-json.js.bytecode-cache stress/module-namespace-access-non-constant.js.bytecode-cache stress/tagged-templates.js.bytecode-cache stress/cloned-arguments-should-visit-callee-during-gc.js.bytecode-cache stress/module-namespace-access.js.bytecode-cache stress/relaxed-line-terminators-in-string.js.bytecode-cache stress/proxy-returning-incomplete-property-descriptor.js.bytecode-cache stress/IIFE-function-name-captured.js.bytecode-cache stress/arity-mismatch-arguments-length.js.bytecode-cache stress/reflect-set-receiver-proxy-set.js.bytecode-cache stress/generator-function-declaration-sinking-osrexit.js.bytecode-cache stress/direct-eval-in-object-literal-methods.js.bytecode-cache stress/do-eval-virtual-call-correctly.js.bytecode-cache stress/tdz-this-in-try-catch.js.bytecode-cache microbenchmarks/emscripten-cube2hash.js.bytecode-cache stress/async-iteration-yield-star-interface.js.bytecode-cache stress/tricky-inferred-infinite-loop-that-uses-captured-variables-and-creates-the-activation-outside-the-loop.js.bytecode-cache stress/generic-arguments-correct-delete-behavior.js.bytecode-cache stress/trailing-comma-in-patterns.js.bytecode-cache stress/lexical-let-and-with-statement.js.bytecode-cache stress/yield-star-throw-continue.js.bytecode-cache stress/completion-value.js.bytecode-cache stress/catch-parameter-syntax.js.bytecode-cache microbenchmarks/scoped-arguments-possibly-overridden-length.js.bytecode-cache stress/scoped-then-direct-arguments-get-by-val-in-baseline.js.bytecode-cache stress/catch-parameter-destructuring.js.bytecode-cache stress/promise-finally.js.bytecode-cache stress/class-syntax-no-tdz-in-eval.js.bytecode-cache microbenchmarks/sink-huge-activation.js.bytecode-cache microbenchmarks/function-with-eval.js.bytecode-cache stress/const-not-strict-mode.js.bytecode-cache stress/domjit-exception.js.bytecode-cache stress/get-declared-unpassed-argument-in-scoped-arguments.js.bytecode-cache stress/singleton-scope-then-overwrite.js.bytecode-cache stress/activation-sink-osrexit.js.bytecode-cache stress/spread-correct-global-object-on-exception.js.bytecode-cache stress/string-object-put-by-index.js.bytecode-cache stress/eval-func-decl-block-with-var-and-remove.js.bytecode-cache stress/string-compare.js.bytecode-cache stress/regress-182419.js.bytecode-cache stress/generator-return-before-first-call.js.bytecode-cache stress/typed-array-canonical-numeric-index-string.js.bytecode-cache stress/new-target.js.bytecode-cache stress/operation-in-throw-error.js.bytecode-cache stress/create-list-from-array-like-not-object.js.bytecode-cache microbenchmarks/simple-poly-call-nested.js.bytecode-cache stress/eval-that-is-not-eval.js.bytecode-cache stress/var-injection-cache-invalidation.js.bytecode-cache microbenchmarks/generator-sunspider-access-nsieve.js.bytecode-cache stress/regress-170661.js.bytecode-cache stress/phantom-spread-forward-varargs.js.bytecode-cache stress/dont-crash-when-hoist-check-structure-on-tdz.js.bytecode-cache microbenchmarks/set-for-each.js.bytecode-cache stress/for-in-array-mode.js.bytecode-cache stress/typedarray-set.js.bytecode-cache stress/string-prototype-scopes.js.bytecode-cache stress/infer-uninitialized-closure-var.js.bytecode-cache microbenchmarks/raytrace-with-try-catch.js.bytecode-cache stress/proxy-of-a-proxy.js.bytecode-cache stress/native-error-properties.js.bytecode-cache stress/new-target-syntax-errors.js.bytecode-cache stress/forward-varargs-double-new-array-buffer.js.bytecode-cache stress/obj-rest-destructuring-order.js.bytecode-cache stress/async-arrow-functions-lexical-arguments-binding.js.bytecode-cache stress/goofy-function-reentry-incorrect-inference.js.bytecode-cache stress/yield-named-variable-generator.js.bytecode-cache stress/eval-script-contains-null-character.js.bytecode-cache stress/jit-gracefully-handle-double-constants-in-math-operators.js.bytecode-cache stress/async-generator-assertion.js.bytecode-cache stress/regress-168546.js.bytecode-cache stress/yield-out-of-generator.js.bytecode-cache stress/typedarray-of.js.bytecode-cache stress/regexp-matches-array-bad-time.js.bytecode-cache microbenchmarks/map-for-each.js.bytecode-cache stress/generator-with-this-strict.js.bytecode-cache stress/dataview-jit-unaligned-accesses.js.bytecode-cache stress/get-prototype-create-this-effectful.js.bytecode-cache stress/unscopables.js.bytecode-cache microbenchmarks/eval-code-ftl.js.bytecode-cache stress/catch-set-argument-speculation-failure.js.bytecode-cache stress/generator-arguments.js.bytecode-cache stress/array-message-passing.js.bytecode-cache stress/function-reentry-infer-on-self.js.bytecode-cache stress/async-function-declaration-sinking-put.js.bytecode-cache stress/import-syntax.js.bytecode-cache stress/class-syntax-super-in-eval.js.bytecode-cache stress/generator-with-this.js.bytecode-cache stress/typedarray-view-string-properties-neutered.js.bytecode-cache stress/eval-func-decl-in-if.js.bytecode-cache stress/arrow-function-syntax.js.bytecode-cache stress/arrowfunction-bound.js.bytecode-cache stress/global-environment-does-not-trap-unscopables.js.bytecode-cache stress/import-from-eval.js.bytecode-cache stress/const-loop-semantics.js.bytecode-cache stress/eval-func-decl-block-with-var-sinthesize.js.bytecode-cache stress/sink-object-past-invalid-check-sneaky.js.bytecode-cache stress/disposable-call-site-index-with-call-and-this.js.bytecode-cache stress/sampling-profiler-bound-function-name.js.bytecode-cache stress/activation-sink-default-value.js.bytecode-cache stress/global-lexical-binding-epoch-should-be-correct-one.js.bytecode-cache stress/put-direct-index-broken-2.js.bytecode-cache microbenchmarks/call-or-not-call.js.bytecode-cache stress/sampling-profiler-deep-stack.js.bytecode-cache stress/check-structure-ir-ensures-empty-does-not-flow-through.js.bytecode-cache stress/math-pow-stable-results.js.bytecode-cache stress/clobberize-needs-to-model-spread-effects.js.bytecode-cache stress/typedarray-indexOf.js.bytecode-cache stress/generator-arguments-from-function.js.bytecode-cache stress/iterator-field-order.js.bytecode-cache stress/typedarray-bad-getter.js.bytecode-cache stress/block-scope-redeclarations.js.bytecode-cache microbenchmarks/Float32Array-matrix-mult.js.bytecode-cache microbenchmarks/arrowfunciton-direct-arguments.js.bytecode-cache stress/compare-eq-should-use-known-other-use.js.bytecode-cache stress/async-await-basic.js.bytecode-cache stress/ai-needs-to-model-spreads-effects.js.bytecode-cache stress/arith-profile-for-negate-can-see-non-number-due-to-dfg-osr-exit-profiling.js.bytecode-cache microbenchmarks/data-view-accesses.js.bytecode-cache stress/arrowfunction-lexical-this-activation-sink.js.bytecode-cache stress/arrowfunction-call.js.bytecode-cache stress/typedarray-every.js.bytecode-cache stress/regress-187074.js.bytecode-cache stress/to-object-intrinsic-null-or-undefined-edge.js.bytecode-cache sunspider-1.0/date-format-tofte.js.bytecode-cache stress/for-in-capture-string-loop-var.js.bytecode-cache stress/async-arrow-functions-lexical-new.target-binding.js.bytecode-cache microbenchmarks/infer-closure-const-then-mov.js.bytecode-cache stress/proxy-set.js.bytecode-cache stress/super-property-access-exceptions.js.bytecode-cache stress/async-function-declaration-sinking-osrexit.js.bytecode-cache stress/tagged-templates-raw-strings.js.bytecode-cache stress/generator-fib-ftl-and-array.js.bytecode-cache stress/proxy-prevent-extensions.js.bytecode-cache stress/generator-return.js.bytecode-cache stress/sink-object-past-invalid-check.js.bytecode-cache stress/arrow-functions-as-default-parameter-values.js.bytecode-cache stress/typedarray-lastIndexOf.js.bytecode-cache stress/dead-access-to-captured-variable-preceded-by-a-live-store.js.bytecode-cache stress/default-value-parsing-should-propagate-error.js.bytecode-cache stress/arrowfunction-lexical-bind-this-8.js.bytecode-cache stress/tagged-templates-in-function-in-direct-eval.js.bytecode-cache stress/force-exit-then-eval.js.bytecode-cache stress/freeze-and-seal-should-prevent-extensions.js.bytecode-cache microbenchmarks/simple-activation-demo.js.bytecode-cache stress/arguments-iterator.js.bytecode-cache stress/iterator-return-beyond-multiple-iteration-scopes.js.bytecode-cache stress/async-iteration-basic.js.bytecode-cache stress/arrowfunction-lexical-this-sinking-put.js.bytecode-cache stress/destructuring-rest-element.js.bytecode-cache stress/class_elements.js.bytecode-cache stress/yield-reserved-word.js.bytecode-cache stress/class-fields-stress-instance.js.bytecode-cache microbenchmarks/polyvariant-get-by-id-shorter-tower.js.bytecode-cache stress/captured-arguments-variable.js.bytecode-cache stress/arrowfunction-tdz-3.js.bytecode-cache stress/es6-default-parameters.js.bytecode-cache stress/regexp-replace-proxy.js.bytecode-cache stress/arrowfunction-lexical-bind-this-2.js.bytecode-cache stress/eval-func-decl-in-block-scope-and-bind-to-top-eval-scope.js.bytecode-cache stress/destructuring-assignment-syntax.js.bytecode-cache stress/stress-cleared-calllinkinfo.js.bytecode-cache stress/lexical-let-tdz.js.bytecode-cache stress/proxy-has-property.js.bytecode-cache stress/arrowfunction-activation-sink.js.bytecode-cache stress/to-this-before-arrow-function-closes-over-this-that-starts-as-lexical-environment.js.bytecode-cache stress/typedarray-forEach.js.bytecode-cache stress/typedarray-reduceRight.js.bytecode-cache stress/generator-frame-empty.js.bytecode-cache stress/yield-label.js.bytecode-cache stress/type-of-functions-and-objects.js.bytecode-cache sunspider-1.0/date-format-xparb.js.bytecode-cache stress/module-namespace-access-transitive-exports.js.bytecode-cache stress/put-local-conservative.js.bytecode-cache stress/const-exception-handling.js.bytecode-cache stress/async-iteration-yield-star.js.bytecode-cache microbenchmarks/eval-code-ftl-reentry.js.bytecode-cache stress/class-syntax-definition-semantics.js.bytecode-cache microbenchmarks/implicit-bigswitch-indirect-symbol.js.bytecode-cache stress/generator-with-new-target.js.bytecode-cache stress/class-derived-from-null.js.bytecode-cache stress/arrowfunction-lexical-bind-arguments-non-strict-2.js.bytecode-cache stress/tricky-infinite-loop-that-uses-captured-variables-and-creates-the-activation-outside-the-loop.js.bytecode-cache stress/infinite-loop-that-uses-captured-variables-with-osr-entry.js.bytecode-cache stress/async-await-module-reserved-word.js.bytecode-cache stress/generator-reduced-save-point-put-to-scope.js.bytecode-cache stress/const-semantics.js.bytecode-cache stress/super-property-access.js.bytecode-cache stress/object-entries.js.bytecode-cache stress/ftl-tail-call-throw-exception-from-slow-path-recover-stack-values.js.bytecode-cache stress/arrowfunction-run-10000-2.js.bytecode-cache stress/global-import-function-should-return-a-promise-when-clearing-exceptions.js.bytecode-cache stress/async-iteration-yield-promise.js.bytecode-cache stress/catch-variables-under-tdz.js.bytecode-cache stress/generator-transfer-register-beyond-mutiple-yields.js.bytecode-cache microbenchmarks/large-int.js.bytecode-cache stress/typedarray-filter.js.bytecode-cache stress/super-in-lexical-scope.js.bytecode-cache stress/rest-parameter-various-types.js.bytecode-cache stress/bug-165091.js.bytecode-cache stress/ordinary-set-exceptions.js.bytecode-cache stress/class-fields-private-out-of-line.js.bytecode-cache stress/generator-function-expression-sinking-osrexit.js.bytecode-cache microbenchmarks/create-lots-of-functions.js.bytecode-cache stress/generator-syntax.js.bytecode-cache stress/exception-dfg-not-strict-equal.js.bytecode-cache stress/array-species-functions.js.bytecode-cache stress/to-this-on-constant-lexical-environment.js.bytecode-cache stress/arrowfunction-lexical-bind-supercall-1.js.bytecode-cache stress/function-constructor-semantics.js.bytecode-cache stress/object-assign-correctness.js.bytecode-cache stress/dfg-reflect-get-prototype-of.js.bytecode-cache stress/async-function-expression-sinking-put.js.bytecode-cache stress/inferred-types-regex-matches-array.js.bytecode-cache stress/generator-type-check.js.bytecode-cache stress/generator-this.js.bytecode-cache microbenchmarks/eval-not-eval-compute-args.js.bytecode-cache stress/regexp-last-index-writable.js.bytecode-cache stress/destructuring-assignment-require-object-coercible.js.bytecode-cache stress/spread-calling.js.bytecode-cache microbenchmarks/fold-double-to-int.js.bytecode-cache stress/infinite-loop-that-uses-captured-variables-before-throwing.js.bytecode-cache stress/const-and-with-statement.js.bytecode-cache stress/async-await-syntax.js.bytecode-cache stress/tagged-templates-identity.js.bytecode-cache microbenchmarks/generator-fib.js.bytecode-cache stress/eval-func-decl-block-with-remove.js.bytecode-cache stress/typed-array-put-by-val-profiling.js.bytecode-cache stress/jsc-read.js.bytecode-cache stress/arrowfunction-lexical-bind-this-7.js.bytecode-cache stress/arrowfunction-lexical-bind-this-4.js.bytecode-cache stress/arith-sub-on-double-array-with-holes.js.bytecode-cache stress/proxy-with-statement.js.bytecode-cache stress/typedarray-reduce.js.bytecode-cache stress/get-array-length-concurrently-change-mode.js.bytecode-cache stress/arith-add-on-double-array-with-holes.js.bytecode-cache stress/typedarray-copyWithin.js.bytecode-cache stress/poly-call-exit.js.bytecode-cache stress/typedarray-slice.js.bytecode-cache stress/generator-throw-before-first-call.js.bytecode-cache stress/lexical-let-exception-handling.js.bytecode-cache stress/proxy-stack-overflow-exceptions.js.bytecode-cache stress/parse-int-intrinsic.js.bytecode-cache stress/simple-module.mjs.bytecode-cache stress/pow-evaluation-order.js.bytecode-cache stress/create-subclass-structure-might-throw.js.bytecode-cache sunspider-1.0/string-unpack-code.js.bytecode-cache stress/generator-fib-ftl-and-object.js.bytecode-cache stress/marked-argument-buffer.js.bytecode-cache microbenchmarks/infer-closure-const-then-mov-no-inline.js.bytecode-cache stress/eval-func-decl-in-eval-within-with-scope.js.bytecode-cache stress/proxy-construct.js.bytecode-cache stress/arrowfunction-run-10000-1.js.bytecode-cache stress/modules-syntax-error-with-names.js.bytecode-cache stress/arrowfunction-lexical-bind-supercall-2.js.bytecode-cache stress/global-object-proto-getter.js.bytecode-cache stress/optional-catch-binding-syntax.js.bytecode-cache stress/typeof-symbol.js.bytecode-cache stress/arrowfunction-lexical-this-sinking-no-double-allocate.js.bytecode-cache stress/block-scoped-function-declarations.js.bytecode-cache stress/get-by-offset-should-use-correct-child.js.bytecode-cache stress/v8-regexp-strict.js.bytecode-cache stress/typedarray-subarray.js.bytecode-cache microbenchmarks/polyvariant-get-by-id-tower.js.bytecode-cache stress/generator-function-expression-sinking-put.js.bytecode-cache stress/typedarray-getter-on-self.js.bytecode-cache stress/regress-151324.js.bytecode-cache stress/yield-and-line-terminator.js.bytecode-cache stress/spread-non-array.js.bytecode-cache stress/import-basic.js.bytecode-cache stress/dont-validate-stack-offset-in-b3-because-it-might-be-guarded-by-control-flow.js.bytecode-cache stress/re-execute-error-module.js.bytecode-cache stress/async-iteration-async-from-sync.js.bytecode-cache stress/dataview-typedarray-toindex.js.bytecode-cache stress/const-tdz.js.bytecode-cache stress/arrowfunction-lexical-bind-this-1.js.bytecode-cache stress/v8-splay-strict.js.bytecode-cache stress/arrowfunction-lexical-this-activation-sink-osrexit.js.bytecode-cache microbenchmarks/eval-not-eval-compute.js.bytecode-cache stress/array-copywithin.js.bytecode-cache stress/generator-function-declaration-sinking-put.js.bytecode-cache stress/big-wasm-memory-grow.js.bytecode-cache microbenchmarks/scoped-arguments-length.js.bytecode-cache stress/inline-call-to-recursive-tail-call.js.bytecode-cache stress/class-syntax-block-scoping.js.bytecode-cache stress/regexp-replace-double-watchpoint.js.bytecode-cache stress/values-unscopables.js.bytecode-cache stress/call-apply-exponential-bytecode-size.js.bytecode-cache stress/to-property-key-correctness.js.bytecode-cache microbenchmarks/large-int-neg.js.bytecode-cache microbenchmarks/Int16Array-bubble-sort.js.bytecode-cache stress/source-origin.js.bytecode-cache stress/arrow-function-token-is-not-keyword.js.bytecode-cache stress/arrowfunction-lexical-bind-this-6.js.bytecode-cache stress/eval-cached.js.bytecode-cache stress/reflect-set-with-global-proxy.js.bytecode-cache stress/reflect-construct.js.bytecode-cache stress/lexical-scoping-break-continue.js.bytecode-cache stress/typedarray-some.js.bytecode-cache stress/generator-with-super.js.bytecode-cache stress/tagged-template-object-collect.js.bytecode-cache stress/tagged-templates-in-direct-eval-should-not-produce-same-site-object.js.bytecode-cache stress/inferred-infinite-loop-that-uses-captured-variables.js.bytecode-cache stress/lexical-let-global-not-captured-variables.js.bytecode-cache stress/trailing-comma-in-function-parameters.js.bytecode-cache stress/catch-clause-should-be-under-tdz4.js.bytecode-cache stress/let-lexical-binding-shadowing-global-properties-and-eval-injection.js.bytecode-cache stress/activation-test-loop.js.bytecode-cache stress/capture-escape-and-throw.js.bytecode-cache stress/string-replace-proxy.js.bytecode-cache stress/cached-prototype-setter.js.bytecode-cache stress/dead-get-closure-var.js.bytecode-cache stress/call-apply-builtin-functions-dont-use-iterators.js.bytecode-cache stress/eval-func-decl-in-global.js.bytecode-cache stress/value-mod-ai-rule.js.bytecode-cache stress/regress-169445.js.bytecode-cache microbenchmarks/infer-one-time-closure-two-vars.js.bytecode-cache stress/super-property-access-to-this.js.bytecode-cache stress/proxy-array-prototype-methods.js.bytecode-cache stress/typedarray-includes.js.bytecode-cache stress/typedarray-functions-with-neutered.js.bytecode-cache stress/arrowfunction-tdz-2.js.bytecode-cache stress/tagged-template-tdz.js.bytecode-cache stress/module-namespace-access-change.js.bytecode-cache stress/infinite-loop-that-uses-captured-variables.js.bytecode-cache stress/parameter-scoping.js.bytecode-cache stress/tagged-templates-template-object.js.bytecode-cache stress/string-prototype-scopes-strict.js.bytecode-cache stress/generator-and-super.js.bytecode-cache stress/proxy-class.js.bytecode-cache stress/proxy-call.js.bytecode-cache stress/licm-should-handle-if-a-hoist-causes-a-provable-osr-exit.js.bytecode-cache stress/async-iteration-for-await-of-syntax.js.bytecode-cache stress/for-in-tests.js.bytecode-cache stress/obj-spread-order.js.bytecode-cache stress/verify-bytecode-generator-cached-variables-under-tdz.js.bytecode-cache stress/dfg-object-proto-getter.js.bytecode-cache stress/singleton-scope-then-realloc-and-overwrite.js.bytecode-cache stress/typedarray-constructor.js.bytecode-cache stress/arrowfunction-lexical-bind-superproperty.js.bytecode-cache microbenchmarks/infer-closure-const-then-put-to-scope-no-inline.js.bytecode-cache stress/r238510-bad-loop.js.bytecode-cache stress/class-syntax-tdz.js.bytecode-cache microbenchmarks/v8-regexp-search.js.bytecode-cache stress/phantom-new-array-buffer-forward-varargs2.js.bytecode-cache stress/exit-then-eval.js.bytecode-cache stress/array-from-with-iterator.js.bytecode-cache v8-v6/v8-regexp.js.bytecode-cache stress/regress-173053.js.bytecode-cache stress/op-negate-inline-cache.js.bytecode-cache stress/proxy-define-own-property.js.bytecode-cache stress/dfg-object-prototype-of.js.bytecode-cache stress/rest-parameter-having-a-bad-time.js.bytecode-cache stress/scoped-arguments-test.js.bytecode-cache stress/array-find-does-not-lookup-twice.js.bytecode-cache stress/generator-class-methods.js.bytecode-cache stress/regress-192626.js.bytecode-cache stress/async-iteration-for-await-of.js.bytecode-cache stress/lexical-let-loop-semantics.js.bytecode-cache microbenchmarks/infer-closure-const-then-put-to-scope.js.bytecode-cache microbenchmarks/inline-get-scoped-var.js.bytecode-cache stress/global-lexical-var-injection.js.bytecode-cache stress/eval-func-decl-with-let-const-class.js.bytecode-cache stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.bytecode-cache stress/arrowfunction-activation-sink-osrexit-default-value-tdz-error.js.bytecode-cache stress/custom-iterators.js.bytecode-cache stress/arrowfunction-lexical-bind-arguments-strict.js.bytecode-cache stress/pow-expects-update-expression-on-lhs.js.bytecode-cache stress/sloppy-mode-function-hoisting.js.bytecode-cache stress/variable-under-tdz-eval-tricky.js.bytecode-cache stress/reflect-set-proxy-set.js.bytecode-cache stress/function-name-scope.js.bytecode-cache stress/scoped-arguments-array-length.js.bytecode-cache stress/template-string-tags-eval.js.bytecode-cache stress/spread-multi-layers.js.bytecode-cache stress/const-lexical-binding-shadowing-global-properties-and-eval-injection.js.bytecode-cache stress/v8-parameter-scoping.js.bytecode-cache stress/proxy-get-set-correct-receiver.js.bytecode-cache stress/arrowfunction-activation-sink-osrexit-default-value.js.bytecode-cache stress/object-assign-changing-properties.js.bytecode-cache stress/lift-template-literal.js.bytecode-cache stress/closure-recursive-tail-call.js.bytecode-cache stress/computed-accessor-parsing.js.bytecode-cache stress/duplicate-computed-accessors.js.bytecode-cache stress/object-from-entries.js.bytecode-cache stress/tagged-templates-syntax.js.bytecode-cache stress/with.js.bytecode-cache stress/IIFE-es6-default-parameters.js.bytecode-cache stress/generator-class-methods-syntax.js.bytecode-cache stress/arrowfunction-lexical-bind-this-3.js.bytecode-cache stress/poly-call-exit-this.js.bytecode-cache stress/async-arrow-functions-lexical-binding-in-class.js.bytecode-cache stress/arrowfunction-tdz-1.js.bytecode-cache stress/regress-191579.js.bytecode-cache stress/get-from-scope-dynamic-onto-proxy.js.bytecode-cache stress/rest-parameter-and-default-arguments.js.bytecode-cache stress/arrowfunction-run-10-1.js.bytecode-cache stress/spread-non-varargs.js.bytecode-cache stress/class-expression-generates-environment.js.bytecode-cache stress/typedarray-map.js.bytecode-cache stress/get-by-id-strict-nested-arguments.js.bytecode-cache stress/template-literal-syntax.js.bytecode-cache microbenchmarks/slow-ternaries.js.bytecode-cache stress/constructor-with-return.js.bytecode-cache stress/arrowfunction-lexical-bind-this-5.js.bytecode-cache stress/map-cse-correctness.js.bytecode-cache v8-v6/v8-splay.js.bytecode-cache stress/tricky-indirectly-inferred-infinite-loop-that-uses-captured-variables-and-creates-the-activation-outside-the-loop.js.bytecode-cache microbenchmarks/scoped-arguments-overridden-length.js.bytecode-cache stress/super-get-by-id.js.bytecode-cache stress/for-in-invalidate-context-weird-assignments.js.bytecode-cache microbenchmarks/infer-one-time-deep-closure.js.bytecode-cache stress/object-assign-fast-path.js.bytecode-cache stress/regress-187373.js.bytecode-cache stress/gc-should-reap-dead-watchpoints.js.bytecode-cache stress/put-dynamic-var-strict-and-sloppy.js.bytecode-cache stress/arity-mismatch-rest.js.bytecode-cache v8-v6/v8-earley-boyer.js.bytecode-cache stress/generator-throw.js.bytecode-cache stress/exception-in-to-property-key-should-be-handled-early.js.bytecode-cache stress/pow-stable-results.js.bytecode-cache stress/object-values.js.bytecode-cache stress/JSON-parse-should-cache-array-lengths.js.bytecode-cache microbenchmarks/eval-compute.js.bytecode-cache stress/scoped-arguments-check-array.js.bytecode-cache stress/regress-159954.js.bytecode-cache stress/arith-mul-with-constants.js.bytecode-cache stress/spread-forward-varargs-rest-parameter-change-iterator-protocol.js.bytecode-cache stress/string-prototype-methods-endsWith-startsWith-includes-correctness.js.bytecode-cache stress/dfg-to-string-on-string-object-does-not-gc.js.bytecode-cache stress/method-name.js.bytecode-cache stress/typedarray-fill.js.bytecode-cache stress/eval-and-super.js.bytecode-cache stress/big-wasm-memory-grow-no-max.js.bytecode-cache stress/big-wasm-memory.js.bytecode-cache stress/eval-func-decl-within-eval-without-reassign-to-let.js.bytecode-cache stress/arrowfunction-lexical-bind-supercall-3.js.bytecode-cache stress/generator-yield-star.js.bytecode-cache microbenchmarks/object-keys-map-values.js.bytecode-cache stress/async-arrow-function-in-class-heritage.js.bytecode-cache stress/typedarray-from.js.bytecode-cache stress/proxy-set-prototype-of.js.bytecode-cache stress/put-by-id-build-list-order-recurse.js.bytecode-cache stress/lexical-let-semantics.js.bytecode-cache stress/array-reverse-proxy.js.bytecode-cache stress/generator-function-name.js.bytecode-cache stress/object-values-changing-properties.js.bytecode-cache stress/async-arrow-functions-lexical-super-binding.js.bytecode-cache microbenchmarks/Int16Array-bubble-sort-with-byteLength.js.bytecode-cache stress/reflect-set.js.bytecode-cache stress/regress-163748.js.bytecode-cache stress/many-nested-functions-parser-stack-overflow.js.bytecode-cache stress/regress-158437.js.bytecode-cache stress/singleton-scope-then-realloc.js.bytecode-cache stress/super-property-access-tdz.js.bytecode-cache stress/assignment-in-function-call-bracket-node.js.bytecode-cache microbenchmarks/try-catch-get-by-val-scoped-arguments.js.bytecode-cache stress/get-by-id-direct-getter.js.bytecode-cache microbenchmarks/infer-closure-const-then-reenter-no-inline.js.bytecode-cache stress/dead-access-to-captured-variable-preceded-by-a-live-store-in-function-with-multiple-basic-blocks.js.bytecode-cache stress/promise-species-functions.js.bytecode-cache stress/variable-named-eval-under-tdz.js.bytecode-cache stress/ensure-proper-strict-mode-parsing.js.bytecode-cache stress/spread-optimized-properly.js.bytecode-cache stress/typed-array-get-by-val-profiling.js.bytecode-cache stress/typedarray-findIndex.js.bytecode-cache stress/regress-190693.js.bytecode-cache stress/destructuring-assignment-accepts-iterables.js.bytecode-cache stress/v8-earley-boyer-strict.js.bytecode-cache stress/class-syntax-double-constructor.js.bytecode-cache stress/constructing-builtin-functions-with-getter-prototype-should-only-call-getter-once-per-new-2.js.bytecode-cache stress/dfg-tail-calls.js.bytecode-cache stress/regexp-match-proxy.js.bytecode-cache stress/proxy-with-unbalanced-getter-setter.js.bytecode-cache stress/generator-fib-ftl.js.bytecode-cache stress/regexp-prototype-tostring.js.bytecode-cache stress/async-function-expression-sinking-osrexit.js.bytecode-cache stress/async-await-reserved-word.js.bytecode-cache stress/sink-object-past-invalid-check-sneakier.js.bytecode-cache stress/object-spread.js.bytecode-cache stress/regexp-matches-array-slow-put.js.bytecode-cache microbenchmarks/apply-not-apply.js.bytecode-cache stress/eval-func-decl-within-eval-with-reassign-to-var.js.bytecode-cache stress/super-property-access-object-literal-to-this-2.js.bytecode-cache stress/eval-func-decl-in-global-of-eval.js.bytecode-cache stress/yield-named-variable.js.bytecode-cache apiTests

Xan Lopez

Comment 130 2019-06-28 12:39:05 PDT

Created attachment 373142 [details] Class fields, parser + bytecode + test changes New patch. Try to fix bytecode cache crashes.

EWS Watchlist

Comment 131 2019-06-28 15:16:13 PDT

Comment on attachment 373142 [details] Class fields, parser + bytecode + test changes Attachment 373142 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12606254 New failing tests: editing/style/apply-style-iframe-crash.html editing/style/iframe-onload-crash-mac.html

EWS Watchlist

Comment 132 2019-06-28 15:16:18 PDT

Created attachment 373153 [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

Saam Barati

Comment 133 2019-07-26 09:04:40 PDT

Comment on attachment 373142 [details] Class fields, parser + bytecode + test changes View in context: https://bugs.webkit.org/attachment.cgi?id=373142&action=review > JSTests/stress/class-fields-harmony.js:3 > +// Copyright 2017 the V8 project authors. All rights reserved. > +// Use of this source code is governed by a BSD-style license that can be > +// found in the LICENSE file. Please include their license here instead of found in the LICENSE file > Source/JavaScriptCore/ChangeLog:65 > + Private fields are stored in a separate hash inside > + VariableEnvironment instead of extending the > + VariableEnvironmentEntry Traits. This is done for several reasons: > + > + 1) Make private names first class in scopes (different from the > + string naming mechanics). > + 2) Easily determine if a scope is a PrivateNameEnvironment by the > + presence of the private name's hash. > + 3) In a future patch for private methods also allow to easily > + determine if a private name access is happening from a field or > + from an accessor/method. > + > + Slightly related to the previous, there are also private name > + hashes in both SymbolTable and JSSymbolTableObject. The main > + reason for this is that while SymbolTables are shared between > + multiple evaluations of the same scope, JSSymbolTableObjects are > + not, since they are newly created during evaluation. We thus store > + the string form of the private names in SymbolTable, and the > + actual SymbolImpl in each JSSymbolTableObject. This matches the > + expected semantics of private names, where two instances of the > + same class with a private field should not collide. This part of this implementation has been nagging at me. I think I've found a nicer way to accomplish the same thing. Let me know if you think it works. Right now, let's say a class C has N private fields. We generate N new symbols each time we create that class C. This is so if we create two C's from the text of C, they don't have visibility into each other. However, instead of creating this new scope mechanism, instead, I think we can just index into a normal scope object using a private symbol per class C. So anytime we create a class C, we give it a symbol specific to it. Then, all instances have a field keyed off that symbol, and that property that exists at that symbol key points to a normal scope (and we throw if that access returns undefined since we're now dealing with a bad instance). So, if we have code like "this.#foo", we will emit something like: ``` let symbol = C.@symbolForPrivateFields; let scope = this[symbol]; if (!scope) throw new Error("..."); result = get_from_scope(scope, "foo") ``` I think this achieves the same scoping mechanism and the same property that two unique Cs can't see each other's fields. Also, it means we have no new scoping related byte codes. Does my analysis that sound right to y'all? Are there any downsides to doing it this way over what's in this patch?

Caio Lima

Comment 134 2019-07-26 10:13:15 PDT

(In reply to Saam Barati from comment #133) > Comment on attachment 373142 [details] > Class fields, parser + bytecode + test changes > > View in context: > https://bugs.webkit.org/attachment.cgi?id=373142&action=review > > > JSTests/stress/class-fields-harmony.js:3 > > +// Copyright 2017 the V8 project authors. All rights reserved. > > +// Use of this source code is governed by a BSD-style license that can be > > +// found in the LICENSE file. > > Please include their license here instead of found in the LICENSE file > > > Source/JavaScriptCore/ChangeLog:65 > > + Private fields are stored in a separate hash inside > > + VariableEnvironment instead of extending the > > + VariableEnvironmentEntry Traits. This is done for several reasons: > > + > > + 1) Make private names first class in scopes (different from the > > + string naming mechanics). > > + 2) Easily determine if a scope is a PrivateNameEnvironment by the > > + presence of the private name's hash. > > + 3) In a future patch for private methods also allow to easily > > + determine if a private name access is happening from a field or > > + from an accessor/method. > > + > > + Slightly related to the previous, there are also private name > > + hashes in both SymbolTable and JSSymbolTableObject. The main > > + reason for this is that while SymbolTables are shared between > > + multiple evaluations of the same scope, JSSymbolTableObjects are > > + not, since they are newly created during evaluation. We thus store > > + the string form of the private names in SymbolTable, and the > > + actual SymbolImpl in each JSSymbolTableObject. This matches the > > + expected semantics of private names, where two instances of the > > + same class with a private field should not collide. > > This part of this implementation has been nagging at me. I think I've found > a nicer way to accomplish the same thing. Let me know if you think it works. I'm wondering if what has been nagging at you is the change we are doing into SymbolTable and JSSymbolTableObject or if it also includes VariableEnvironment changes. I'm asking that because we are using VariableEnvironment changes to proper emit code to access private methods/accessors on other patches. This is required because we need to know at compile time if a private name is referring to a field, a method or an accessor, since the access to each one is different. > Right now, let's say a class C has N private fields. We generate N new > symbols each time we create that class C. This is so if we create two C's > from the text of C, they don't have visibility into each other. However, > instead of creating this new scope mechanism, instead, I think we can just > index into a normal scope object using a private symbol per class C. > > So anytime we create a class C, we give it a symbol specific to it. Then, > all instances have a field keyed off that symbol, and that property that > exists at that symbol key points to a normal scope (and we throw if that > access returns undefined since we're now dealing with a bad instance). > > So, if we have code like "this.#foo", we will emit something like: > ``` > let symbol = C.@symbolForPrivateFields; > let scope = this[symbol]; > if (!scope) throw new Error("..."); > result = get_from_scope(scope, "foo") > ``` > > I think this achieves the same scoping mechanism and the same property that > two unique Cs can't see each other's fields. Also, it means we have no new > scoping related byte codes. IIUC, your idea is to store private fields into a separate scope instead of into Object, right? > Does my analysis that sound right to y'all? > > Are there any downsides to doing it this way over what's in this patch? Personally, I like this idea but I need to think a little bit more about this. One comment that I have is the case where `this` can be a Proxy. When such situation happens, we can't use `this[symbol]` because it won't work when we try to get private fields installed in a Proxy (see test https://github.com/tc39/test262/blob/master/test/language/statements/class/elements/privatefield-on-proxy.js). In such case, we will need a kind of `get_by_val_direct` or something like that to properly access the private field. Beside that, I think this is an approach that is worth it trying.

Saam Barati

Comment 135 2019-07-26 10:26:36 PDT

(In reply to Caio Lima from comment #134) > (In reply to Saam Barati from comment #133) > > Comment on attachment 373142 [details] > > Class fields, parser + bytecode + test changes > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=373142&action=review > > > > > JSTests/stress/class-fields-harmony.js:3 > > > +// Copyright 2017 the V8 project authors. All rights reserved. > > > +// Use of this source code is governed by a BSD-style license that can be > > > +// found in the LICENSE file. > > > > Please include their license here instead of found in the LICENSE file > > > > > Source/JavaScriptCore/ChangeLog:65 > > > + Private fields are stored in a separate hash inside > > > + VariableEnvironment instead of extending the > > > + VariableEnvironmentEntry Traits. This is done for several reasons: > > > + > > > + 1) Make private names first class in scopes (different from the > > > + string naming mechanics). > > > + 2) Easily determine if a scope is a PrivateNameEnvironment by the > > > + presence of the private name's hash. > > > + 3) In a future patch for private methods also allow to easily > > > + determine if a private name access is happening from a field or > > > + from an accessor/method. > > > + > > > + Slightly related to the previous, there are also private name > > > + hashes in both SymbolTable and JSSymbolTableObject. The main > > > + reason for this is that while SymbolTables are shared between > > > + multiple evaluations of the same scope, JSSymbolTableObjects are > > > + not, since they are newly created during evaluation. We thus store > > > + the string form of the private names in SymbolTable, and the > > > + actual SymbolImpl in each JSSymbolTableObject. This matches the > > > + expected semantics of private names, where two instances of the > > > + same class with a private field should not collide. > > > > This part of this implementation has been nagging at me. I think I've found > > a nicer way to accomplish the same thing. Let me know if you think it works. > > I'm wondering if what has been nagging at you is the change we are doing > into SymbolTable and JSSymbolTableObject or if it also includes > VariableEnvironment changes. I'm asking that because we are using > VariableEnvironment changes to proper emit code to access private > methods/accessors on other patches. This is required because we need to know > at compile time if a private name is referring to a field, a method or an > accessor, since the access to each one is different. it’s just more the the fact that we didn’t map it onto existing primitives. I think this proposal gives us that. > > > Right now, let's say a class C has N private fields. We generate N new > > symbols each time we create that class C. This is so if we create two C's > > from the text of C, they don't have visibility into each other. However, > > instead of creating this new scope mechanism, instead, I think we can just > > index into a normal scope object using a private symbol per class C. > > > > So anytime we create a class C, we give it a symbol specific to it. Then, > > all instances have a field keyed off that symbol, and that property that > > exists at that symbol key points to a normal scope (and we throw if that > > access returns undefined since we're now dealing with a bad instance). > > > > So, if we have code like "this.#foo", we will emit something like: > > ``` > > let symbol = C.@symbolForPrivateFields; > > let scope = this[symbol]; > > if (!scope) throw new Error("..."); > > result = get_from_scope(scope, "foo") > > ``` > > > > I think this achieves the same scoping mechanism and the same property that > > two unique Cs can't see each other's fields. Also, it means we have no new > > scoping related byte codes. > > IIUC, your idea is to store private fields into a separate scope instead of > into Object, right? > > > Does my analysis that sound right to y'all? > > > > Are there any downsides to doing it this way over what's in this patch? > > Personally, I like this idea but I need to think a little bit more about > this. One comment that I have is the case where `this` can be a Proxy. When > such situation happens, we can't use `this[symbol]` because it won't work > when we try to get private fields installed in a Proxy (see test > https://github.com/tc39/test262/blob/master/test/language/statements/class/ > elements/privatefield-on-proxy.js). In such case, we will need a kind of > `get_by_val_direct` or something like that to properly access the private > field. Beside that, I think this is an approach that is worth it trying.

Caitlin Potter (:caitp)

Comment 136 2019-07-31 07:53:29 PDT

(In reply to Saam Barati from comment #133) > Comment on attachment 373142 [details] > Class fields, parser + bytecode + test changes > > View in context: > https://bugs.webkit.org/attachment.cgi?id=373142&action=review > > > JSTests/stress/class-fields-harmony.js:3 > > +// Copyright 2017 the V8 project authors. All rights reserved. > > +// Use of this source code is governed by a BSD-style license that can be > > +// found in the LICENSE file. > > Please include their license here instead of found in the LICENSE file > > > Source/JavaScriptCore/ChangeLog:65 > > + Private fields are stored in a separate hash inside > > + VariableEnvironment instead of extending the > > + VariableEnvironmentEntry Traits. This is done for several reasons: > > + > > + 1) Make private names first class in scopes (different from the > > + string naming mechanics). > > + 2) Easily determine if a scope is a PrivateNameEnvironment by the > > + presence of the private name's hash. > > + 3) In a future patch for private methods also allow to easily > > + determine if a private name access is happening from a field or > > + from an accessor/method. > > + > > + Slightly related to the previous, there are also private name > > + hashes in both SymbolTable and JSSymbolTableObject. The main > > + reason for this is that while SymbolTables are shared between > > + multiple evaluations of the same scope, JSSymbolTableObjects are > > + not, since they are newly created during evaluation. We thus store > > + the string form of the private names in SymbolTable, and the > > + actual SymbolImpl in each JSSymbolTableObject. This matches the > > + expected semantics of private names, where two instances of the > > + same class with a private field should not collide. > > This part of this implementation has been nagging at me. I think I've found > a nicer way to accomplish the same thing. Let me know if you think it works. > > Right now, let's say a class C has N private fields. We generate N new > symbols each time we create that class C. This is so if we create two C's > from the text of C, they don't have visibility into each other. However, > instead of creating this new scope mechanism, instead, I think we can just > index into a normal scope object using a private symbol per class C. > > So anytime we create a class C, we give it a symbol specific to it. Then, > all instances have a field keyed off that symbol, and that property that > exists at that symbol key points to a normal scope (and we throw if that > access returns undefined since we're now dealing with a bad instance). > > So, if we have code like "this.#foo", we will emit something like: > ``` > let symbol = C.@symbolForPrivateFields; > let scope = this[symbol]; > if (!scope) throw new Error("..."); > result = get_from_scope(scope, "foo") > ``` > > I think this achieves the same scoping mechanism and the same property that > two unique Cs can't see each other's fields. Also, it means we have no new > scoping related byte codes. > > Does my analysis that sound right to y'all? > > Are there any downsides to doing it this way over what's in this patch? This seems very similar to what is already done, except that you're changing where the private name is loaded (outside of the get/put op rather than inside it) --- I'm fine with doing this, no problem there. It also means allocating a special scope for private names, separate from the class lexical scope --- then scope inheritance gets a bit harder to track for nested scopes. For get/put operations, we need either new opcodes or new get/put flags (or both if we go with the get_by_val_direct/put_by_val_direct idea) to add the extra private field semantics (throwing at runtime if field isn't defined yet, distinction between "put" and "define", etc) Is it important that we do this before landing? This will take up some time to implement, and we've been trying to get this landed for many months now. From my POV, the changes are fine, but because it will take some time, I'd prefer to add these as a followup, and call it an optimization.

Caitlin Potter (:caitp)

Comment 137 2019-08-28 10:32:11 PDT

Ping --- asking again, can we initially land a version without that change first? We are working on a number of these features in parallel, and it would be easiest to make this change afterwords, so that there isn't a need to go back and change the parallel features.

Caitlin Potter (:caitp)

Comment 138 2019-09-03 09:44:49 PDT

Comment on attachment 373142 [details] Class fields, parser + bytecode + test changes View in context: https://bugs.webkit.org/attachment.cgi?id=373142&action=review >> Source/JavaScriptCore/ChangeLog:65 >> + same class with a private field should not collide. > > This part of this implementation has been nagging at me. I think I've found a nicer way to accomplish the same thing. Let me know if you think it works. > > Right now, let's say a class C has N private fields. We generate N new symbols each time we create that class C. This is so if we create two C's from the text of C, they don't have visibility into each other. However, instead of creating this new scope mechanism, instead, I think we can just index into a normal scope object using a private symbol per class C. > > So anytime we create a class C, we give it a symbol specific to it. Then, all instances have a field keyed off that symbol, and that property that exists at that symbol key points to a normal scope (and we throw if that access returns undefined since we're now dealing with a bad instance). > > So, if we have code like "this.#foo", we will emit something like: > ``` > let symbol = C.@symbolForPrivateFields; > let scope = this[symbol]; > if (!scope) throw new Error("..."); > result = get_from_scope(scope, "foo") > ``` > > I think this achieves the same scoping mechanism and the same property that two unique Cs can't see each other's fields. Also, it means we have no new scoping related byte codes. > > Does my analysis that sound right to y'all? > > Are there any downsides to doing it this way over what's in this patch? I'm trying to work out when we allocate the scope. In a situation like ``` class Base { #x = 1; C = class Child extends Base { #y = 2; xy() { return this.#x + this.#y; } } } (new Base.C).xy(); ``` What is the order of operations here? Is `#x` in `this[Base.@symbolForPrivateFields]` and `#y` in `this[Child.@symbolForPrivateFields]`? Or the same scope? how much metadata do we need to store to know which scope to look in when using nested PrivateNameEnvironments? I keep running into confusing cases with this model that are hard to explain/work through, and because of that I don't really like the approach, even if it can take advantage of some stuff that already exists.

Caitlin Potter (:caitp)

Comment 139 2019-10-07 13:35:15 PDT

Created attachment 380353 [details] Class fields, parser + bytecode + JIT + test changes This version includes our version of the changes requested by Saam --- see changelog for details

Caio Lima

Comment 140 2019-10-08 06:24:43 PDT

Comment on attachment 380353 [details] Class fields, parser + bytecode + JIT + test changes View in context: https://bugs.webkit.org/attachment.cgi?id=380353&action=review LGTM overall. I've left some comments into ChangeLog. > Source/JavaScriptCore/ChangeLog:15 > + a private name #instanceFieldInitializer is conditionally added to I think it is better use `@instanceFieldInitializer`. > Source/JavaScriptCore/ChangeLog:19 > + similar to what the v8 engine does. This is not true anymore. We only emit the call to this function if class contains fields. > Source/JavaScriptCore/ChangeLog:65 > + same class with a private field should not collide. From new implementation, I think this is also not true anymore, since we are storing PrivateSymbols into class scope, as explained below. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1999 > + // TODO: only do this if there is an eval() within a nested scope --- otherwise it isn't needed. nit: turn this TODO into a FIXME. > Source/JavaScriptCore/runtime/ModuleProgramExecutable.h:1 > + nit: remove this.

Caitlin Potter (:caitp)

Comment 141 2019-10-09 12:04:08 PDT

Created attachment 380552 [details] Class fields, parser + bytecode + JIT + test changes

Caitlin Potter (:caitp)

Comment 142 2019-10-09 12:26:11 PDT

Comment on attachment 380552 [details] Class fields, parser + bytecode + JIT + test changes View in context: https://bugs.webkit.org/attachment.cgi?id=380552&action=review just taking some notes for if I upload a new version tomorrow... > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:-2245 > - bool hasCapturedVariables = false; renaming `hasCapturedVariables` to `needsToPopScope` is not needed, can be removed from diff > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3243 > +bool BytecodeGenerator::shouldEmitSetFunctionName(ExpressionNode* valueNode) There are alternative ways to do this, like having a private `emitSetFunctionNameIfNeeded()` template which takes a lambda that returns a RegisterID*, and having public versions for RegisterID* and const Identifier& respectively --- but it doesn't matter much. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4503 > + return emitLoadDerivedConstructorFromArrowFunctionLexicalEnvironment(); it's not obvious that this works for eval() --- it might be worth renaming the "ArrowFunctionLexicalEnvironment" to something else --- but not here. > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:-786 > - RegisterID* ret; all the DotAccessor changes make things a lot more confusing to me :( Any thoughts on this approach? > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1544 > +getByValOp(get_by_val_direct, OpGetByValDirect, _slow_path_get_by_val_direct, macro(a, b) end) do we need OSR exit points for these? I noticed putByVal didn't add one, but I don't see why the non-direct version would but not the direct version. > JSTests/ChangeLog:21 > + * stress/async-await-syntax.js: TODO: remove JS spamy symbols from change list

Caitlin Potter (:caitp)

Comment 143 2019-10-09 13:25:24 PDT

Created attachment 380560 [details] hopefully fix llint32_64

Caitlin Potter (:caitp)

Comment 144 2019-10-11 08:45:49 PDT

Created attachment 380757 [details] Patch for landing, hopefully This last one implements the changes from https://github.com/tc39/proposal-class-fields/issues/263

EWS Watchlist

Comment 145 2019-10-12 05:14:35 PDT

Comment on attachment 380757 [details] Patch for landing, hopefully Attachment 380757 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/13122969 New failing tests: http/wpt/resource-timing/rt-resources-per-worker.html

EWS Watchlist

Comment 146 2019-10-12 05:14:40 PDT

Created attachment 380826 [details] Archive of layout-test-results from ews215 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews215 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

Ross Kirsling

Comment 147 2019-10-15 05:33:26 PDT

Comment on attachment 380757 [details] Patch for landing, hopefully View in context: https://bugs.webkit.org/attachment.cgi?id=380757&action=review I'm not in a position to be the ultimate reviewer on this, but here are some things I noticed! (BTW: "Patch for landing" is usually what we call a resubmission of a patch that received "r+ with comments" which is now being sent directly to the commit queue. In particular, it's the default description when using `webkit-patch land-safely`.) > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3268 > + emitSetFunctionNameIfNeededImpl(valueNode, value, [=]() { return emitLoad(newTemporary(), ident); }); Seems like we probably don't want to capture by value here. > Source/JavaScriptCore/parser/Lexer.cpp:-2413 > - // Hashbang is only permitted at the start of the source text. This comment might be worth preserving. > Source/JavaScriptCore/parser/NodeConstructors.h:256 > + , m_isPrivate(!!(type & Private)) The !! shouldn't be required (here and below). > Source/JavaScriptCore/parser/Nodes.h:2253 > + enum Type { Name, PrivateName, ComputedName }; I think this ought to be an enum class. > Source/JavaScriptCore/parser/Parser.cpp:2988 > + failIfTrue(classScope->declareLexicalVariable(ident, true) != DeclarationResult::Valid, "Failed to declare lexical variable for computed class field. FIXME: This should never happen!"); Hmm, if this should never happen, it should probably be an assertion and not a user-facing error. > Source/JavaScriptCore/parser/Parser.cpp:3119 > + m_token.m_type = EOFTOK; // Hack to make parsing valid Can you elaborate? Seems surprising since EOF *would* be an error here, hehe. > Source/JavaScriptCore/parser/Parser.h:1233 > + ASSERT(i < m_scopeStack.size()); I know this is mimicking stuff above, but is this assertion even hittable? `i` needed to have been positive in order to enter the loop... > Source/JavaScriptCore/parser/Parser.h:1269 > + ASSERT(i < m_scopeStack.size() && m_scopeStack.size()); Similarly, I think the latter conjunct is redundant here. > Source/JavaScriptCore/parser/VariableEnvironment.h:127 > + VariableEnvironment() > + { } Style: this can be on one line > Source/JavaScriptCore/parser/VariableEnvironment.h:132 > + { } Style (here and right below): each brace gets its own line (like in NodeConstructors.h) > Source/JavaScriptCore/parser/VariableEnvironment.h:240 > + RareData() > + { } > + RareData(RareData&& other) > + : m_privateNames(WTFMove(other.m_privateNames)) > + { } Style: ditto to the previous two > Source/JavaScriptCore/runtime/JSSymbolTableObject.h:65 > - > + You could probably drop this file since it's just whitespace changes. > JSTests/stress/class-fields-harmony.js:6 > +// Copyright 2017 the V8 project authors. All rights reserved. > +// Use of this source code is governed by a BSD-style license that can be > +// found in the LICENSE file. I think if we import a test file wholesale like this, we at least need to insert the license contents here.

Caitlin Potter (:caitp)

Comment 148 2019-10-21 12:09:07 PDT

Created attachment 381427 [details] Patch for thoroughly scrutinizing

Caitlin Potter (:caitp)

Comment 149 2019-10-21 12:12:12 PDT

Comment on attachment 380757 [details] Patch for landing, hopefully View in context: https://bugs.webkit.org/attachment.cgi?id=380757&action=review I think I've taken care of all of these comments, hopefully the new patch description will be seen as more deserving of a look. In the meantime, still working on improving JIT and tier-up support for the new operations. >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3268 >> + emitSetFunctionNameIfNeededImpl(valueNode, value, [=]() { return emitLoad(newTemporary(), ident); }); > > Seems like we probably don't want to capture by value here. Done > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4411 > + RefPtr<RegisterID> scope = generator.emitResolveScope(generator.newTemporary(), var); op_resolve_scope seems to not be doing our work properly here. We want to dynamically resolve the correct lexical scope, but end up using the global object. I don't think this was always the case, since it causes some previously passing tests to fail. I am having trouble finding exactly what was changed that might have broken this, though. These variables are treated as GlobalProperty variables, and should not be. I'm considering options to fix this, but any suggestions would be welcome. >> Source/JavaScriptCore/parser/NodeConstructors.h:256 >> + , m_isPrivate(!!(type & Private)) > > The !! shouldn't be required (here and below). Opted to just get rid of the field >> Source/JavaScriptCore/parser/Nodes.h:2253 >> + enum Type { Name, PrivateName, ComputedName }; > > I think this ought to be an enum class. Done >> Source/JavaScriptCore/parser/Parser.cpp:2988 >> + failIfTrue(classScope->declareLexicalVariable(ident, true) != DeclarationResult::Valid, "Failed to declare lexical variable for computed class field. FIXME: This should never happen!"); > > Hmm, if this should never happen, it should probably be an assertion and not a user-facing error. Agreed, done >> Source/JavaScriptCore/parser/Parser.cpp:3119 >> + m_token.m_type = EOFTOK; // Hack to make parsing valid > > Can you elaborate? Seems surprising since EOF *would* be an error here, hehe. So, this function is called by `parseInner()` --- parseInner normally expects the entire SourceCode to be consumed, but in this case, we take the entire class SourceCode but only parse little chunks of it (the Syntax is already guaranteed to be valid, but the entire thing isn't consumed). So because of this, we cheat and trick parseInner into believing the entire SourceCode was consumed, and parsing wasn't aborted at any point. Anyways, I've added a more detailed explanation/comment of why this is done. >> Source/JavaScriptCore/parser/Parser.h:1233 >> + ASSERT(i < m_scopeStack.size()); > > I know this is mimicking stuff above, but is this assertion even hittable? `i` needed to have been positive in order to enter the loop... Removed the assert >> Source/JavaScriptCore/parser/Parser.h:1269 >> + ASSERT(i < m_scopeStack.size() && m_scopeStack.size()); > > Similarly, I think the latter conjunct is redundant here. Thanks, removed >> Source/JavaScriptCore/parser/VariableEnvironment.h:127 >> + { } > > Style: this can be on one line Done >> Source/JavaScriptCore/parser/VariableEnvironment.h:132 >> + { } > > Style (here and right below): each brace gets its own line (like in NodeConstructors.h) Done >> Source/JavaScriptCore/parser/VariableEnvironment.h:240 >> + { } > > Style: ditto to the previous two Done >> Source/JavaScriptCore/runtime/JSSymbolTableObject.h:65 >> + > > You could probably drop this file since it's just whitespace changes. Done >> JSTests/stress/class-fields-harmony.js:6 >> +// found in the LICENSE file. > > I think if we import a test file wholesale like this, we at least need to insert the license contents here. Done

EWS Watchlist

Comment 150 2019-10-21 14:32:41 PDT

Comment on attachment 381427 [details] Patch for thoroughly scrutinizing Attachment 381427 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/13159825 New failing tests: http/tests/misc/repeat-open-cancel.html

EWS Watchlist

Comment 151 2019-10-21 14:32:45 PDT

Created attachment 381457 [details] Archive of layout-test-results from ews212 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews212 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

Caitlin Potter (:caitp)

Comment 152 2019-10-23 12:34:08 PDT

Created attachment 381711 [details] Patch for Thoroughly Scrutinizing Updated with all the JSGlobalObject*/CallFrame* instead of ExecState* stuff

Caio Lima

Comment 153 2019-10-29 12:38:26 PDT

Comment on attachment 381711 [details] Patch for Thoroughly Scrutinizing View in context: https://bugs.webkit.org/attachment.cgi?id=381711&action=review I left some comments, majority of them are minor changes. > Source/JavaScriptCore/ChangeLog:96 > + a Symbol object with a no-descriptor PrivateSymbolImpl. It would be very informative here a piece of bytecode of class evaluation. > Source/JavaScriptCore/ChangeLog:101 > + 'get_by_val_direct'. I think it is worth it mention that we don't fully support JIT of `get_by_val_direct`. > Source/JavaScriptCore/bytecode/PutByValFlags.h:42 > + PutByValPrivateFieldPut = PutByValPrivateName, What's the main reason for having this alias here? Maybe it is unecessary. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:-781 > - Oops. I think this was not intentional. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1145 > + classFieldsInitializer = m_codeBlock->classFieldsInitializer(); I think we have tests to cover this, right? A test calling `super()` from an inner function would verify this path. > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:764 > + auto length = node.isPrivate() ? description.length() : 1; Is there any case where a computed field node can be a private? Maybe I'm missing something here. > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:854 > + // TODO: only load the PrivateSymbol once for unary ++ / -- nit: TODOs should be a FIXME with a bug to resolve it. > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:883 > + // TODO: only load the PrivateSymbol once for unary ++ / -- Ditto. > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:906 > + return emitPutProperty(generator, base, value, thisValue); Can we use use a default value of `emitPutProperty` as `nullptr` here? > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4398 > + generator.emitSetFunctionNameIfNeeded(m_assign, value.get(), *m_ident); We should have tests covering this. > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4461 > + bool hasInstanceFields = this->hasInstanceFields(); I think it would be better have something like `ClassFieldsInitializer classFieldInitializer = this->hasInstanceFields() ? ClassFieldsInitializer::Needed : ClassFieldsInitializer::NotNeeded` here. > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4468 > + if (hasInstanceFields) here: `metadata->setClassFieldsInitializer(classFieldInitializer);` > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4474 > and here only use `classFieldInitializer`. I personally don't like this cast from boolean to `ClassFieldsInitializer`, since it makes obscure the result this cast. > Source/JavaScriptCore/jit/JITOperations.cpp:746 > + Why do we have 2 different functions that are doing almost exactly the same thing? I think we should refactor this to re-use code. > Source/JavaScriptCore/parser/Parser.cpp:841 > + failIfTrue(match(PRIVATENAME), "Cannot parse variable declaration"); I think we should have better error message here, like `PrivateNames can't be used to declare variables`. > Source/JavaScriptCore/parser/Parser.cpp:1298 > + failIfTrue(match(PRIVATENAME), "Cannot parse this destructuring pattern"); Ditto. > JSTests/stress/class-fields-function-name.js:17 > +assert(a.baz.name == undefined); We should add private names as well. > JSTests/test262/config.yaml:-19 > - - class-fields-private I think we need to figure out a way to enable `useClassFields` flag when we don't skip those. Otherwise, we will fail Test262 bot.

Caitlin Potter (:caitp)

Comment 154 2019-10-29 13:31:03 PDT

Comment on attachment 381711 [details] Patch for Thoroughly Scrutinizing View in context: https://bugs.webkit.org/attachment.cgi?id=381711&action=review >> Source/JavaScriptCore/bytecode/PutByValFlags.h:42 >> + PutByValPrivateFieldPut = PutByValPrivateName, > > What's the main reason for having this alias here? Maybe it is unecessary. It documents the uses of the other flags, so that a reader will know why they exist. It puts intent forward >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1145 >> + classFieldsInitializer = m_codeBlock->classFieldsInitializer(); > > I think we have tests to cover this, right? A test calling `super()` from an inner function would verify this path. class-fields-harmony.js exercises this path >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:764 >> + auto length = node.isPrivate() ? description.length() : 1; > > Is there any case where a computed field node can be a private? Maybe I'm missing something here. looks vestigial, removed >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:854 >> + // TODO: only load the PrivateSymbol once for unary ++ / -- > > nit: TODOs should be a FIXME with a bug to resolve it. comment looks vestigial, removed >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:883 >> + // TODO: only load the PrivateSymbol once for unary ++ / -- > > Ditto. ditto >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:906 >> + return emitPutProperty(generator, base, value, thisValue); > > Can we use use a default value of `emitPutProperty` as `nullptr` here? The reason this is written this way is so ReadModifyDotNode case initialize `thisValue` in the get operation, and reuse it in the put operation. I prefer to not use default-null pointers, they get confusing. Technically, the design isn't fully needed, since currently `emitPutProperty` never needs to initialize `thisValue` --- but I prefer it if they work consistently, that makes the code overall easier to understand, even if it is slightly unnecessary for the put case. This could be rewritten to use pointers and avoid overloads, but again, I don't think it's worth it. What might be a bit nicer is renaming the version with `thisValue` to `emit###PropertyWithThisValue` or something, so that it's easier to see that the overloaded version is being used.I dunno if anybody agrees with that assessment, I am sure someone is uneasy about the way `thisValue` is initialized by `emitGetProperty()`. WDYT? >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4398 >> + generator.emitSetFunctionNameIfNeeded(m_assign, value.get(), *m_ident); > > We should have tests covering this. class-fields-harmony.js has some coverage of this >> Source/JavaScriptCore/jit/JITOperations.cpp:746 >> + > > Why do we have 2 different functions that are doing almost exactly the same thing? I think we should refactor this to re-use code. Because we are trying to fit into the design of the existing IC code, which makes it tricky to do things like add additional parameters. Of course there could be a 3rd impl function, probably a template function to avoid having ternary statements all over the place --- I don't think it would really be an improvement >> Source/JavaScriptCore/parser/Parser.cpp:841 >> + failIfTrue(match(PRIVATENAME), "Cannot parse variable declaration"); > > I think we should have better error message here, like `PrivateNames can't be used to declare variables`. ack >> Source/JavaScriptCore/parser/Parser.cpp:1298 >> + failIfTrue(match(PRIVATENAME), "Cannot parse this destructuring pattern"); > > Ditto. I'm not 100% sure this error is ever visible anyways, don't we try to reparse as an object literal if it fails? >> JSTests/stress/class-fields-function-name.js:17 >> +assert(a.baz.name == undefined); > > We should add private names as well. ack >> JSTests/test262/config.yaml:-19 >> - - class-fields-private > > I think we need to figure out a way to enable `useClassFields` flag when we don't skip those. Otherwise, we will fail Test262 bot. On our Igalia EWS we should be able to change the test run command to `JSC_useClassFields=1 test262-runner ...` easily enough, afaik. Or supply the flag some other way. Not something we can or should worry about in this bug, though.

Caio Lima

Comment 155 2019-10-29 13:52:46 PDT

Comment on attachment 381711 [details] Patch for Thoroughly Scrutinizing View in context: https://bugs.webkit.org/attachment.cgi?id=381711&action=review >>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:906 >>> + return emitPutProperty(generator, base, value, thisValue); >> >> Can we use use a default value of `emitPutProperty` as `nullptr` here? > > The reason this is written this way is so ReadModifyDotNode case initialize `thisValue` in the get operation, and reuse it in the put operation. I prefer to not use default-null pointers, they get confusing. Technically, the design isn't fully needed, since currently `emitPutProperty` never needs to initialize `thisValue` --- but I prefer it if they work consistently, that makes the code overall easier to understand, even if it is slightly unnecessary for the put case. > > This could be rewritten to use pointers and avoid overloads, but again, I don't think it's worth it. What might be a bit nicer is renaming the version with `thisValue` to `emit###PropertyWithThisValue` or something, so that it's easier to see that the overloaded version is being used.I dunno if anybody agrees with that assessment, I am sure someone is uneasy about the way `thisValue` is initialized by `emitGetProperty()`. WDYT? I do t have a strong opinion here. I’m biased to follow the same approach we have to emitGetById that will use “with_this” version of “thisValue” is present. I’m ok leaving the code as it is if the definition of “thisValue” is deterministic. Can you confirm that we will never have junk value into “thisValue”? >>> JSTests/test262/config.yaml:-19 >>> - - class-fields-private >> >> I think we need to figure out a way to enable `useClassFields` flag when we don't skip those. Otherwise, we will fail Test262 bot. > > On our Igalia EWS we should be able to change the test run command to `JSC_useClassFields=1 test262-runner ...` easily enough, afaik. Or supply the flag some other way. Not something we can or should worry about in this bug, though. But I’m not sure if we are able to set the flag to Apple Test262 bots like https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20Test262%20%28Tests%29.

Ross Kirsling

Comment 156 2019-10-29 13:59:47 PDT

As of r251588, test262/config.yaml has a mapping from test262 features to JSC feature flags. :)

Caitlin Potter (:caitp)

Comment 157 2019-10-30 08:22:37 PDT

Created attachment 382314 [details] Patch for Thoroughly Scrutinizing Added test262 config.yaml changes and addressed a few more nits.

Caitlin Potter (:caitp)

Comment 158 2019-10-30 08:24:00 PDT

Comment on attachment 382314 [details] Patch for Thoroughly Scrutinizing View in context: https://bugs.webkit.org/attachment.cgi?id=382314&action=review > Source/ThirdParty/ANGLE/ChangeLog:3 > + [JSC] Add support for instance class fields I'm not sure where this got pulled in from, so... guess I'll make another one.

Caitlin Potter (:caitp)

Comment 159 2019-10-30 09:04:28 PDT

Created attachment 382318 [details] Patch for Thoroughly Scrutinizing Added test262 config.yaml changes and addressed a few more nits, and hopefully fixed the previous issue o_o.

Caitlin Potter (:caitp)

Comment 160 2019-11-13 10:22:43 PST

Created attachment 383468 [details] Patch for Thoroughly Scrutinizing Periodic rebase --- Changed to use the LinkTimeConstants mechanic from https://bugs.webkit.org/show_bug.cgi?id=153792 which probably generates marginally better bytecode now.

Caitlin Potter (:caitp)

Comment 161 2019-11-26 08:41:00 PST

Created attachment 384358 [details] Part 1: Public Instance Fields

Caitlin Potter (:caitp)

Comment 162 2019-11-26 08:49:06 PST

Created attachment 384359 [details] Part 2: Private Instance Fields

Caitlin Potter (:caitp)

Comment 163 2019-11-26 08:52:25 PST

The public field and private field parts of the patch have been split apart (as requested by Yusuke offline). I'm hoping this will help with reviewing a bit, but I'd still prefer it if the changes can be signed off on and landed in a single patch.

Caio Lima

Comment 164 2019-11-26 12:19:44 PST

Comment on attachment 384358 [details] Part 1: Public Instance Fields View in context: https://bugs.webkit.org/attachment.cgi?id=384358&action=review LGTM overall. I left some comments. > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4319 > + Variable var = generator.variable(*m_ident); Looking to this code and seeing `m_ident` being used on computed names is counter intuitive IMHO. I think a comment here explaining how we use the `m_ident` would be very valuable. > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1958 > + bbaeq JSCell::m_type[t0], SymbolType, .opToPropertyKeySlow Is there a reason why we always fallback to slow path on String case? > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2098 > + bbaeq JSCell::m_type[t0], SymbolType, .opToPropertyKeySlow ditto > Source/JavaScriptCore/runtime/JSCJSValueInlines.h:702 > + JSValue primitive = toPrimitive(globalObject, PreferString); I think it is good have a test to cover the case of `PreferString`. We can have a computed property with both `valueOf` and `toString` methods and assert that `toString` is called and `valueOf` is never called.

Caitlin Potter (:caitp)

Comment 165 2019-12-02 08:01:42 PST

Comment on attachment 384358 [details] Part 1: Public Instance Fields View in context: https://bugs.webkit.org/attachment.cgi?id=384358&action=review >> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1958 >> + bbaeq JSCell::m_type[t0], SymbolType, .opToPropertyKeySlow > > Is there a reason why we always fallback to slow path on String case? We don't. CellType is 0, StringType is 1, and SymbolType is 2. The idea is "Branch if the source cell's JSType > SymbolType" (inferring that, if everything is working properly, the source is a String or Symbol and not a misc JSCell). Admittedly, since this is bbaeq, we're also taking the slow case for Symbols. I had fixed this to use bba in llint64, but forgot to do that here.

Caitlin Potter (:caitp)

Comment 166 2019-12-02 08:01:43 PST

Caitlin Potter (:caitp)

Comment 167 2019-12-05 11:37:39 PST

Created attachment 384927 [details] Part 1: Public Instance Fields (rev2) Addressed some of Caio's comments

Caitlin Potter (:caitp)

Comment 168 2019-12-05 11:48:55 PST

Created attachment 384930 [details] Part 2: Private Instance Fields (rev2) Rebased since last week

Ross Kirsling

Comment 169 2019-12-09 17:49:13 PST

Comment on attachment 384927 [details] Part 1: Public Instance Fields (rev2) View in context: https://bugs.webkit.org/attachment.cgi?id=384927&action=review A couple of nits from me. > Source/JavaScriptCore/bytecode/ExecutableInfo.h:34 > +enum class ClassFieldsInitializer : uint8_t { NotNeeded, Needed }; Hmm, this name really is kind of surprising for a type that's effectively a boolean...though I guess SuperBinding is like this too. Adding, say, `Requirement` to the end admittedly might be kind of long, but there'd also be precedent for leaning into the booleanness and doing something like NeedsClassFieldInitializer { No, Yes }. > Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp:61 > + const auto& instanceFieldLocations = optionalInstanceFieldLocations ? *optionalInstanceFieldLocations : Vector<JSTextPosition>(); Style nit: I think there's preference for using optional.value() instead of *optional.

Ross Kirsling

Comment 170 2019-12-09 22:50:48 PST

Comment on attachment 384930 [details] Part 2: Private Instance Fields (rev2) View in context: https://bugs.webkit.org/attachment.cgi?id=384930&action=review > Source/JavaScriptCore/ChangeLog:34 > + put_by_val_direct <receiver, propably 'this'>, <symbol>, <value>, <PutByValPrivateName> `propably` may not be a word, but I kind of want it to be. :P > Source/JavaScriptCore/bytecode/PutByValFlags.h:33 > +enum PutByValFlags : int32_t { I want to say `enum class PutByValFlags : uint8_t`, but I see PutByIdFlags is like this too... > Source/JavaScriptCore/parser/Lexer.cpp:943 > + bool isPrivateIdentifier = m_current == '@' && m_parsingBuiltinFunction; I think "private" was the wrong word for this one to begin with -- how about isBuiltinName? > Source/JavaScriptCore/parser/Nodes.h:184 > + virtual bool isPrivateLocation() const { return false; } Hmm, is it worth adding this to ExpressionNodes across the board if it's just `isDotAccessorNode() && isPrivateName()`? (I don't mean that it hurts anything so much as it feels weirdly non-orthogonal; it's not true of *all* DotAccessorNodes and it *cannot* be true of other location types.) > Source/JavaScriptCore/parser/Parser.cpp:4793 > +bool Parser<LexerType>::addPrivateNameUsedIfNeeded(const Identifier* ident) I think this means "add in a used state" but it's kind of hard to parse. I was also surprised that the return value here indicates not whether we needed to do work, but whether we ran into trouble on the way (meaning that the "if needed" is wrapped in an "if possible"). Perhaps `ensurePrivateNameUsed` would cover both points? I don't believe "add" lends any information, but "ensure" indicates what we might fail to do. > Source/JavaScriptCore/parser/VariableEnvironment.h:91 > +struct PrivateNameEntry { Augh, it's a shame that this whole thing is basically just OptionSet<PrivateNameTraits>. I suppose it'd be tricky to update one without the other though... > Source/JavaScriptCore/runtime/JSScope.cpp:344 > + for (auto end = privateNames.end(), iter = privateNames.begin(); iter != end; ++iter) Any reason this couldn't be a range-based for loop? > Source/JavaScriptCore/runtime/SymbolTable.cpp:188 > + for (; iter != end; ++iter) Ditto.

Yusuke Suzuki

Comment 171 2019-12-10 02:04:03 PST

Comment on attachment 384927 [details] Part 1: Public Instance Fields (rev2) View in context: https://bugs.webkit.org/attachment.cgi?id=384927&action=review Thanks for splitting. It is easy to review to me. First, I would like to ask you a design question. > Source/JavaScriptCore/ChangeLog:16 > + In summary, class fields are initialized by a synthetic JSFunction. In its unlinked state, > + the UnlinkedFunctionExecutable for the function includes an ordered list of JSTokenLocations > + pointing to the start of each class field in the class. Each of these fields are parsed and > + included as DefineFieldNodes, which implement the appropriate DefineField behaviour in the > + proposal. This synthetic function is only created, and only loaded, if there are class fields > + present. Why not initializing fields in the constructor? Creating a new JSFunction and calling it is (1) slow, and (2) memory consuming. Can you clarify the reason in ChangeLog? > Source/JavaScriptCore/dfg/DFGCapabilities.cpp:295 > + case op_to_property_key: Can you add DFG/FTL implementation too? > Source/JavaScriptCore/jit/JITOpcodes.cpp:1612 > + addSlowCase(jump()); Can you implement the fast path for StringType and SymbolType? > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1966 > + bba JSCell::m_type[t0], SymbolType, .opToPropertyKeySlow We also have `CellType` and which can be used for any other use cases. Can you do, loadb JSCell::m_type[t0], t2 beq t2, SymbolType, .done bneq t2, StringType, .slow .done: return (t1, t0) ... > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2099 > + btqnz t0, notCellMask, .opToPropertyKeySlow > + bbaeq JSCell::m_type[t0], SymbolType, .opToPropertyKeySlow > + return(t0) Ditto > Source/JavaScriptCore/runtime/JSCJSValueInlines.h:694 > +ALWAYS_INLINE JSValue JSValue::toPropertyKey(JSGlobalObject* globalObject, JSValueTag) const Let's rename it to `toPropertyKeyValue` instead of using JSValueTag, since it is difficult to know the difference between `Identifier JSValue::toPropertyKey`.

Caitlin Potter (:caitp)

Comment 172 2019-12-10 05:00:33 PST

Comment on attachment 384927 [details] Part 1: Public Instance Fields (rev2) View in context: https://bugs.webkit.org/attachment.cgi?id=384927&action=review Thanks very much, I’ll get to addressing these points in a few hours. >> Source/JavaScriptCore/ChangeLog:16 >> + present. > > Why not initializing fields in the constructor? Creating a new JSFunction and calling it is (1) slow, and (2) memory consuming. > Can you clarify the reason in ChangeLog? I’ll get to the rest of this later today, but: It’s non-trivial to do this. 1) in either case, we still need to do the weird “go back and parse the fields again”, but now the fields AST lives in a different parser arena, and fixing that is a bit tricky. Beyond that, it only really makes sense to do this for base classes. For extended classes, this might need to be done in the constructors, or in a nested arrow or eval. Beyond that, the scope of initialization is slightly different from the scope of the constructor, with no access to “arguments”, and no access to variables within the constructor’s body. Because of the complexity, it wasn’t considered worthwhile to spend time on this initially. It might be something worth doing later. I believe this is explained in the unified patch ChangeLog, and I still think the unified patch should be the one which is landed eventually, but O’ll add some points about this here shortly.

Caitlin Potter (:caitp)

Comment 173 2019-12-12 07:59:35 PST

Created attachment 385494 [details] Part 1: Public Instance Fields (rev3)

Caitlin Potter (:caitp)

Comment 174 2019-12-12 08:20:55 PST

Created attachment 385496 [details] Part 2: Private Instance Fields (rev3)

Caitlin Potter (:caitp)

Comment 175 2019-12-12 08:23:56 PST

Comment on attachment 384927 [details] Part 1: Public Instance Fields (rev2) View in context: https://bugs.webkit.org/attachment.cgi?id=384927&action=review >> Source/JavaScriptCore/bytecode/ExecutableInfo.h:34 >> +enum class ClassFieldsInitializer : uint8_t { NotNeeded, Needed }; > > Hmm, this name really is kind of surprising for a type that's effectively a boolean...though I guess SuperBinding is like this too. > Adding, say, `Requirement` to the end admittedly might be kind of long, but there'd also be precedent for leaning into the booleanness and doing something like NeedsClassFieldInitializer { No, Yes }. I've gone ahead and changed it to `NeedsClassFieldInitializer` >> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp:61 >> + const auto& instanceFieldLocations = optionalInstanceFieldLocations ? *optionalInstanceFieldLocations : Vector<JSTextPosition>(); > > Style nit: I think there's preference for using optional.value() instead of *optional. Done >> Source/JavaScriptCore/dfg/DFGCapabilities.cpp:295 >> + case op_to_property_key: > > Can you add DFG/FTL implementation too? This is in progress (https://github.com/caitp/webkit/commit/a761d5559156023adb1680aaef48e5c6fba35a36), I'll get some help with this next week --- but I don't think this should be a blocker for landing, ditto for the other new opcode. It's much easier to add DFG/FTL support separately, and not having them in the initial commit shouldn't slow down any existing code AFAIK. >> Source/JavaScriptCore/jit/JITOpcodes.cpp:1612 >> + addSlowCase(jump()); > > Can you implement the fast path for StringType and SymbolType? Done >> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1966 >> + bba JSCell::m_type[t0], SymbolType, .opToPropertyKeySlow > > We also have `CellType` and which can be used for any other use cases. > Can you do, > > loadb JSCell::m_type[t0], t2 > beq t2, SymbolType, .done > bneq t2, StringType, .slow > .done: > return (t1, t0) > ... Done >> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2099 >> + return(t0) > > Ditto Done ditto >> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:694 >> +ALWAYS_INLINE JSValue JSValue::toPropertyKey(JSGlobalObject* globalObject, JSValueTag) const > > Let's rename it to `toPropertyKeyValue` instead of using JSValueTag, since it is difficult to know the difference between `Identifier JSValue::toPropertyKey`. Done

Caitlin Potter (:caitp)

Comment 176 2019-12-12 08:24:18 PST

Comment on attachment 384930 [details] Part 2: Private Instance Fields (rev2) View in context: https://bugs.webkit.org/attachment.cgi?id=384930&action=review >> Source/JavaScriptCore/ChangeLog:34 >> + put_by_val_direct <receiver, propably 'this'>, <symbol>, <value>, <PutByValPrivateName> > > `propably` may not be a word, but I kind of want it to be. :P I've corrected the typo. >> Source/JavaScriptCore/bytecode/PutByValFlags.h:33 >> +enum PutByValFlags : int32_t { > > I want to say `enum class PutByValFlags : uint8_t`, but I see PutByIdFlags is like this too... I agree, but it is bitfield-ish and that's always annoying with enum classes, it's probably simpler to stick with the approach used in PutById because of that. At least, that was my rationale >> Source/JavaScriptCore/parser/Lexer.cpp:943 >> + bool isPrivateIdentifier = m_current == '@' && m_parsingBuiltinFunction; > > I think "private" was the wrong word for this one to begin with -- how about isBuiltinName? Sure, done >> Source/JavaScriptCore/parser/Nodes.h:184 >> + virtual bool isPrivateLocation() const { return false; } > > Hmm, is it worth adding this to ExpressionNodes across the board if it's just `isDotAccessorNode() && isPrivateName()`? > > (I don't mean that it hurts anything so much as it feels weirdly non-orthogonal; it's not true of *all* DotAccessorNodes and it *cannot* be true of other location types.) I don't mind removing it, but it gets a little bit wordier. The spec proposal has a similar abstraction (IsPrivateReference) that this maps directly to, so having it exist in the same way it. If you'd prefer to change it in favor of the wordier version, I have no problem with that. >> Source/JavaScriptCore/parser/Parser.cpp:4793 >> +bool Parser<LexerType>::addPrivateNameUsedIfNeeded(const Identifier* ident) > > I think this means "add in a used state" but it's kind of hard to parse. > > I was also surprised that the return value here indicates not whether we needed to do work, but whether we ran into trouble on the way (meaning that the "if needed" is wrapped in an "if possible"). > > Perhaps `ensurePrivateNameUsed` would cover both points? I don't believe "add" lends any information, but "ensure" indicates what we might fail to do. Sounds good to me, done >> Source/JavaScriptCore/runtime/JSScope.cpp:344 >> + for (auto end = privateNames.end(), iter = privateNames.begin(); iter != end; ++iter) > > Any reason this couldn't be a range-based for loop? Done

Caio Lima

Comment 177 2019-12-17 06:27:12 PST

(In reply to Caitlin Potter (:caitp) from comment #175) > Comment on attachment 384927 [details] > Part 1: Public Instance Fields (rev2) > > >> Source/JavaScriptCore/dfg/DFGCapabilities.cpp:295 > >> + case op_to_property_key: > > > > Can you add DFG/FTL implementation too? > > This is in progress > (https://github.com/caitp/webkit/commit/ > a761d5559156023adb1680aaef48e5c6fba35a36), I'll get some help with this next > week --- but I don't think this should be a blocker for landing, ditto for > the other new opcode. It's much easier to add DFG/FTL support separately, > and not having them in the initial commit shouldn't slow down any existing > code AFAIK. I agree with Caitlin here. I think it would be better handling DFG/FTL of `to_property_key` in a separate patch. IMHO, including DFG/FTL will make it harder to be reviewed and it is very likely that it will make this patch wait longer. BTW, there is a new version of the patch since it was reviewed last week. Could anybody take a look on it? I'm personally interested in get some feedback from private fields implementation, since it is the same foundation we are using to implement private methods and accessors.

Caio Lima

Comment 178 2019-12-17 07:38:56 PST

Comment on attachment 385494 [details] Part 1: Public Instance Fields (rev3) View in context: https://bugs.webkit.org/attachment.cgi?id=385494&action=review > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:740 > + RefPtr<RegisterID> propertyExpr; I think it would be very valuable explain here why a computed field has a `node.name()`. It is counter intuitive. > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:750 > + generator.emitProfileType(propertyName, var, position(), JSTextPosition(-1, position().offset + 1, -1)); I don't see we using this on DFG so far. Is this profiling still necessary? > Source/JavaScriptCore/parser/Parser.cpp:4792 > + semanticFailIfFalse(currentScope()->isFunction() || isFunctionEvalContextType || isClassFieldInitializer, "new.target is only valid inside functions"); Do we have test case covering this? > Source/JavaScriptCore/parser/Parser.cpp:4795 > + semanticFailIfFalse(!closestOrdinaryFunctionScope->isGlobalCodeScope() || isFunctionEvalContextType || isClassFieldInitializer, "new.target is not valid inside arrow functions in global code"); Ditto.

Yusuke Suzuki

Comment 179 2020-01-09 16:41:51 PST

Comment on attachment 385494 [details] Part 1: Public Instance Fields (rev3) View in context: https://bugs.webkit.org/attachment.cgi?id=385494&action=review Second round of review. Looks nice overall! But putting r- since we need that sizeof() of critical classes must be the same. > Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:420 > + unsigned m_needsClassFieldInitializer : 1; Can you ensure this does not increase the sizeof(UnlinkedCodeBlock)? This is strong requirement. > Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:222 > + Optional<Vector<JSTextPosition>> instanceFieldLocations() const > + { > + if (m_rareData) > + return m_rareData->m_instanceFieldLocations; > + return WTF::nullopt; > + } It is copying Vector<>. Can you just return a pointer to `Vector<JSTextPosition>` instead? > Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:259 > + unsigned m_needsClassFieldInitializer : 1; I think this makes sizeof(UnlinkedFunctionExecutable) larger, and it immediately consumes super very large size of memory in some website including Gmail. Please put this flag without increasing sizeof(UnlinkedFunctionExecutable). This is strong requirement. I think you can put this after m_isGeneratedFromCache. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3459 > + dst = tempDestination(dst); Should not accept nullptr here. Caller of this function should ensure it. Just making this as OpToPropertyKey::emit(this, dst, src); return dst; > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:962 > + func = generator.emitLoadDerivedConstructor(); Let's avoid generator.emitLoadDerivedConstructor if instance-fields do not exist. > Source/JavaScriptCore/dfg/DFGCapabilities.cpp:295 > + case op_to_property_key: File FIXME for DFG handling for this. We need DFG and FTL implementation when enabling class-fields. > Source/JavaScriptCore/jit/JITOpcodes.cpp:364 > + int dst = bytecode.m_dst.offset(); > + int src = bytecode.m_src.offset(); Use VirtualRegister. > Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:350 > + int dst = bytecode.m_dst.offset(); > + int src = bytecode.m_src.offset(); Ditto. > Source/JavaScriptCore/parser/Nodes.h:2239 > + void emitBytecode(BytecodeGenerator&, RegisterID* destination = 0) override; Use `= nullptr` for new code. > Source/JavaScriptCore/parser/Parser.cpp:3049 > + const Identifier* ident = 0; Use `nullptr` for new code. > Source/JavaScriptCore/runtime/CodeCache.h:293 > bool isStrictMode = rootNode->features() & StrictModeFeature; I think using if constexpr is easier to read than using needsClassFieldInitializer(executable). NeedsClassFieldInitializer needsClassFieldInitializer = NeedsClassFieldInitializer::No; if constexpr (std::is_same_v<ExecutableType, DirectEvalExecutable>) needsClassFieldInitializer = executable->needsClassFieldInitializer(); > Source/JavaScriptCore/runtime/DirectEvalExecutable.cpp:73 > : EvalExecutable(globalObject, source, inStrictContext, derivedContextType, isArrowFunctionContext, evalContextType) Let's propagate needsClassFieldInitializer to EvalExecutable and initialize it correctly in initializer-list, instead of initializing it inside the constructor.

Caitlin Potter (:caitp)

Comment 180 2020-01-10 11:34:14 PST

Created attachment 387356 [details] Part 1: Public Instance Fields (rev4)

Caitlin Potter (:caitp)

Comment 181 2020-01-10 11:35:18 PST

Comment on attachment 385494 [details] Part 1: Public Instance Fields (rev3) View in context: https://bugs.webkit.org/attachment.cgi?id=385494&action=review I believe I've addressed each of these in the latest patch. >> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:420 >> + unsigned m_needsClassFieldInitializer : 1; > > Can you ensure this does not increase the sizeof(UnlinkedCodeBlock)? This is strong requirement. done >> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:222 >> + } > > It is copying Vector<>. Can you just return a pointer to `Vector<JSTextPosition>` instead? It is, but it's also pretty rare. We weren't sure what would be preferred, since Optional is more elegant and there's some precedent for using it :) I guess we'll switch back to using the pointer. Done >> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:259 >> + unsigned m_needsClassFieldInitializer : 1; > > I think this makes sizeof(UnlinkedFunctionExecutable) larger, and it immediately consumes super very large size of memory in some website including Gmail. Please put this flag without increasing sizeof(UnlinkedFunctionExecutable). > This is strong requirement. I think you can put this after m_isGeneratedFromCache. Yep, it looks like that very narrowly fits. Done. >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3459 >> + dst = tempDestination(dst); > > Should not accept nullptr here. Caller of this function should ensure it. Just making this as > > OpToPropertyKey::emit(this, dst, src); > return dst; Done >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:740 >> + RefPtr<RegisterID> propertyExpr; > > I think it would be very valuable explain here why a computed field has a `node.name()`. It is counter intuitive. I've added a comment, and I believe this is also mentioned in the Changelog >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:750 >> + generator.emitProfileType(propertyName, var, position(), JSTextPosition(-1, position().offset + 1, -1)); > > I don't see we using this on DFG so far. Is this profiling still necessary? Removed it and the DFG/FTL tests [not included in this patch] still pass, done >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:962 >> + func = generator.emitLoadDerivedConstructor(); > > Let's avoid generator.emitLoadDerivedConstructor if instance-fields do not exist. Done >> Source/JavaScriptCore/dfg/DFGCapabilities.cpp:295 >> + case op_to_property_key: > > File FIXME for DFG handling for this. We need DFG and FTL implementation when enabling class-fields. Done >> Source/JavaScriptCore/jit/JITOpcodes.cpp:364 >> + int src = bytecode.m_src.offset(); > > Use VirtualRegister. Done >> Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:350 >> + int src = bytecode.m_src.offset(); > > Ditto. Done >> Source/JavaScriptCore/parser/Nodes.h:2239 >> + void emitBytecode(BytecodeGenerator&, RegisterID* destination = 0) override; > > Use `= nullptr` for new code. done >> Source/JavaScriptCore/parser/Parser.cpp:3049 >> + const Identifier* ident = 0; > > Use `nullptr` for new code. Done >> Source/JavaScriptCore/parser/Parser.cpp:4792 >> + semanticFailIfFalse(currentScope()->isFunction() || isFunctionEvalContextType || isClassFieldInitializer, "new.target is only valid inside functions"); > > Do we have test case covering this? It's covered in JSTests/stress/class-fields-harmony.js (line 403) >> Source/JavaScriptCore/parser/Parser.cpp:4795 >> + semanticFailIfFalse(!closestOrdinaryFunctionScope->isGlobalCodeScope() || isFunctionEvalContextType || isClassFieldInitializer, "new.target is not valid inside arrow functions in global code"); > > Ditto. This is also included in the class-fields-harmony.js test (line 373) >> Source/JavaScriptCore/runtime/CodeCache.h:293 >> bool isStrictMode = rootNode->features() & StrictModeFeature; > > I think using if constexpr is easier to read than using needsClassFieldInitializer(executable). > > NeedsClassFieldInitializer needsClassFieldInitializer = NeedsClassFieldInitializer::No; > if constexpr (std::is_same_v<ExecutableType, DirectEvalExecutable>) > needsClassFieldInitializer = executable->needsClassFieldInitializer(); Done >> Source/JavaScriptCore/runtime/DirectEvalExecutable.cpp:73 >> : EvalExecutable(globalObject, source, inStrictContext, derivedContextType, isArrowFunctionContext, evalContextType) > > Let's propagate needsClassFieldInitializer to EvalExecutable and initialize it correctly in initializer-list, instead of initializing it inside the constructor. Done

Caitlin Potter (:caitp)

Comment 182 2020-01-10 12:06:46 PST

Created attachment 387361 [details] Part 1: Public Instance Fields (rev4)

Caitlin Potter (:caitp)

Comment 183 2020-01-10 12:17:07 PST

Created attachment 387363 [details] Part 2: Private Instance Fields (rev4)

Yusuke Suzuki

Comment 184 2020-01-10 17:02:01 PST

Comment on attachment 387361 [details] Part 1: Public Instance Fields (rev4) View in context: https://bugs.webkit.org/attachment.cgi?id=387361&action=review r=me with comments. Nice. > Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:86 > + if (info.needsClassFieldInitializer() == NeedsClassFieldInitializer::Yes) { > + createRareDataIfNecessary(); > + m_rareData->m_needsClassFieldInitializer = static_cast<unsigned>(NeedsClassFieldInitializer::Yes); > + } Nice > Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:217 > + Vector<JSTextPosition>* instanceFieldLocations() const Yeah, elegant code is nice. But I love performance benefit, so if I need to take one of either, I always take performance benefit :P > Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:228 > + ensureRareData().m_instanceFieldLocations = instanceFieldLocations; Let's do `WTFMove(instanceFieldLocations)` to avoid copying. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1134 > + if (constructorKind() == ConstructorKind::Extends || isDerivedConstructorContext()) { > newDerivedContextType = DerivedContextType::DerivedConstructorContext; > + needsClassFieldInitializer = m_codeBlock->needsClassFieldInitializer(); > + } This code means that, FunctionMetadataNode::needsClassFieldInitializer could be wrong (Even if it is saying NO, it depends on the upper CodeBlock's value). So, I don't think FunctionMetadataNode::needsClassFieldInitializer's name is correct. Please rename FunctionMetadataNode::needsClassFieldInitializer to appropriate one, and ensure it is appropriately set. For example, FunctionMetadataNode::isConstructorAndNeedsClassFieldInitializer And UnlinkedCodeBlock::needsClassFieldInitializer > Source/JavaScriptCore/parser/Nodes.cpp:333 > +bool PropertyListNode::shouldCreateLexicalScopeForClass(PropertyListNode* list) Add FIXME+url to avoid this behavior. While collecting property lists, I think we can calculate it. > Source/JavaScriptCore/parser/Nodes.cpp:345 > +bool PropertyListNode::hasInstanceFields() const Ditto. > Source/JavaScriptCore/parser/Parser.cpp:2964 > + } else if (Options::useClassFields() && !match(OPENPAREN) && tag == ClassElementTag::Instance && parseMode == SourceParseMode::MethodMode && !isGetter && !isSetter) { `!isGetter && !isSetter` is not necessary since we have `if (isGetter || isSetter) {` Put ASSERT(!isGetter && !isSetter) inside brace.

Yusuke Suzuki

Comment 185 2020-01-10 17:03:45 PST

(In reply to Yusuke Suzuki from comment #184) > Comment on attachment 387361 [details] > Part 1: Public Instance Fields (rev4) > > View in context: > https://bugs.webkit.org/attachment.cgi?id=387361&action=review > > r=me with comments. Nice. > > > Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:86 > > + if (info.needsClassFieldInitializer() == NeedsClassFieldInitializer::Yes) { > > + createRareDataIfNecessary(); > > + m_rareData->m_needsClassFieldInitializer = static_cast<unsigned>(NeedsClassFieldInitializer::Yes); > > + } > > Nice > > > Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:217 > > + Vector<JSTextPosition>* instanceFieldLocations() const > > Yeah, elegant code is nice. But I love performance benefit, so if I need to > take one of either, I always take performance benefit :P > > > Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:228 > > + ensureRareData().m_instanceFieldLocations = instanceFieldLocations; > > Let's do `WTFMove(instanceFieldLocations)` to avoid copying. > > > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1134 > > + if (constructorKind() == ConstructorKind::Extends || isDerivedConstructorContext()) { > > newDerivedContextType = DerivedContextType::DerivedConstructorContext; > > + needsClassFieldInitializer = m_codeBlock->needsClassFieldInitializer(); > > + } > > This code means that, FunctionMetadataNode::needsClassFieldInitializer could > be wrong (Even if it is saying NO, it depends on the upper CodeBlock's > value). > So, I don't think FunctionMetadataNode::needsClassFieldInitializer's name is > correct. > Please rename FunctionMetadataNode::needsClassFieldInitializer to > appropriate one, and ensure it is appropriately set. > For example, > > FunctionMetadataNode::isConstructorAndNeedsClassFieldInitializer > And > UnlinkedCodeBlock::needsClassFieldInitializer > > > Source/JavaScriptCore/parser/Nodes.cpp:333 > > +bool PropertyListNode::shouldCreateLexicalScopeForClass(PropertyListNode* list) > > Add FIXME+url to avoid this behavior. While collecting property lists, I > think we can calculate it. > > > Source/JavaScriptCore/parser/Nodes.cpp:345 > > +bool PropertyListNode::hasInstanceFields() const > > Ditto. > > > Source/JavaScriptCore/parser/Parser.cpp:2964 > > + } else if (Options::useClassFields() && !match(OPENPAREN) && tag == ClassElementTag::Instance && parseMode == SourceParseMode::MethodMode && !isGetter && !isSetter) { > > `!isGetter && !isSetter` is not necessary since we have `if (isGetter || > isSetter) {` > > Put ASSERT(!isGetter && !isSetter) inside brace. And before landing, please check jsc-armv7's new failure.

Caitlin Potter (:caitp)

Comment 186 2020-01-13 13:00:50 PST

Comment on attachment 387361 [details] Part 1: Public Instance Fields (rev4) View in context: https://bugs.webkit.org/attachment.cgi?id=387361&action=review > And before landing, please check jsc-armv7's new failure. I'm a bit stumped on the class-fields-harmony.js failure in the arm7 run. >>> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:86 >>> + } >> >> Nice > > And before landing, please check jsc-armv7's new failure. Just for the record, I've been looking into this for the past few hours, but haven't been able to reproduce the failure using https://github.com/caitp/webkit/commit/afef4270fa6e2d140b75a074742e95f383424727. Unfortunately, checking on the latest master branch has been difficult. Our buildbox cross compiler is failing to build with or without my patch applied. In the case of the class-fields-harmony.js.default failure on the bot, the failure in question appears to not use any of the newly introduced opcodes, but it's difficult to verify this at the moment. >> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:228 >> + ensureRareData().m_instanceFieldLocations = instanceFieldLocations; > > Let's do `WTFMove(instanceFieldLocations)` to avoid copying. Done (when next version is uploaded) >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1134 >> + } > > This code means that, FunctionMetadataNode::needsClassFieldInitializer could be wrong (Even if it is saying NO, it depends on the upper CodeBlock's value). > So, I don't think FunctionMetadataNode::needsClassFieldInitializer's name is correct. > Please rename FunctionMetadataNode::needsClassFieldInitializer to appropriate one, and ensure it is appropriately set. > For example, > > FunctionMetadataNode::isConstructorAndNeedsClassFieldInitializer > And > UnlinkedCodeBlock::needsClassFieldInitializer What this is really saying is, "this code is either a constructor which will need to initialize class fields, or is an arrow function nested within a derived class constructor which will need to initialize instance fields after `super()`" It is potentially a false-positive, if there is no super-call within the arrow function, but it's not possible to be a false negative, AFAIK. There might be a better name, like "needsClassFieldInitializeDuringConstructOrSuperCall" or something. But it's a bit wordy (and not that informative/helpful, IMHO). Do you think a comment would help explain this better? >> Source/JavaScriptCore/parser/Parser.cpp:2964 >> + } else if (Options::useClassFields() && !match(OPENPAREN) && tag == ClassElementTag::Instance && parseMode == SourceParseMode::MethodMode && !isGetter && !isSetter) { > > `!isGetter && !isSetter` is not necessary since we have `if (isGetter || isSetter) {` > > Put ASSERT(!isGetter && !isSetter) inside brace. Done

Caitlin Potter (:caitp)

Comment 187 2020-01-13 13:05:03 PST

(In reply to Caitlin Potter (:caitp) from comment #186) > Comment on attachment 387361 [details] > Part 1: Public Instance Fields (rev4) > > View in context: > https://bugs.webkit.org/attachment.cgi?id=387361&action=review > > > And before landing, please check jsc-armv7's new failure. > > I'm a bit stumped on the class-fields-harmony.js failure in the arm7 run. > > >>> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:86 > >>> + } > >> > >> Nice > > > > And before landing, please check jsc-armv7's new failure. > > Just for the record, I've been looking into this for the past few hours, but > haven't been able to reproduce the failure using > https://github.com/caitp/webkit/commit/ > afef4270fa6e2d140b75a074742e95f383424727. > > Unfortunately, checking on the latest master branch has been difficult. Our > buildbox cross compiler is failing to build with or without my patch applied. > > In the case of the class-fields-harmony.js.default failure on the bot, the > failure in question appears to not use any of the newly introduced opcodes, > but it's difficult to verify this at the moment. This might not have come across as clearly as intended. That commit builds using the ARM cross compiler toolchain, and runs the test without fail (does not seem to be flaky). The patch on the bug is that commit rebased over master and with the changelog added on. However, when the diff is applied over the latest master branch, the build fails, so I can't comment on whether the test is broken there or not.

Yusuke Suzuki

Comment 188 2020-01-13 13:06:19 PST

Comment on attachment 387361 [details] Part 1: Public Instance Fields (rev4) View in context: https://bugs.webkit.org/attachment.cgi?id=387361&action=review >>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1134 >>> + } >> >> This code means that, FunctionMetadataNode::needsClassFieldInitializer could be wrong (Even if it is saying NO, it depends on the upper CodeBlock's value). >> So, I don't think FunctionMetadataNode::needsClassFieldInitializer's name is correct. >> Please rename FunctionMetadataNode::needsClassFieldInitializer to appropriate one, and ensure it is appropriately set. >> For example, >> >> FunctionMetadataNode::isConstructorAndNeedsClassFieldInitializer >> And >> UnlinkedCodeBlock::needsClassFieldInitializer > > What this is really saying is, "this code is either a constructor which will need to initialize class fields, or is an arrow function nested within a derived class constructor which will need to initialize instance fields after `super()`" > > It is potentially a false-positive, if there is no super-call within the arrow function, but it's not possible to be a false negative, AFAIK. > > There might be a better name, like "needsClassFieldInitializeDuringConstructOrSuperCall" or something. But it's a bit wordy (and not that informative/helpful, IMHO). Do you think a comment would help explain this better? I think we should use `needsClassFieldInitializeDuringConstructOrSuperCall` for FunctionMetadataNode, because `needsClassFieldInitializer` name is incorrectt for FunctionMetadataNode :)

Yusuke Suzuki

Comment 189 2020-01-13 13:07:52 PST

Comment on attachment 387361 [details] Part 1: Public Instance Fields (rev4) View in context: https://bugs.webkit.org/attachment.cgi?id=387361&action=review >>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1134 >>>> + } >>> >>> This code means that, FunctionMetadataNode::needsClassFieldInitializer could be wrong (Even if it is saying NO, it depends on the upper CodeBlock's value). >>> So, I don't think FunctionMetadataNode::needsClassFieldInitializer's name is correct. >>> Please rename FunctionMetadataNode::needsClassFieldInitializer to appropriate one, and ensure it is appropriately set. >>> For example, >>> >>> FunctionMetadataNode::isConstructorAndNeedsClassFieldInitializer >>> And >>> UnlinkedCodeBlock::needsClassFieldInitializer >> >> What this is really saying is, "this code is either a constructor which will need to initialize class fields, or is an arrow function nested within a derived class constructor which will need to initialize instance fields after `super()`" >> >> It is potentially a false-positive, if there is no super-call within the arrow function, but it's not possible to be a false negative, AFAIK. >> >> There might be a better name, like "needsClassFieldInitializeDuringConstructOrSuperCall" or something. But it's a bit wordy (and not that informative/helpful, IMHO). Do you think a comment would help explain this better? > > I think we should use `needsClassFieldInitializeDuringConstructOrSuperCall` for FunctionMetadataNode, because `needsClassFieldInitializer` name is incorrectt for FunctionMetadataNode :) Ah, no, needsClassFieldInitializeDuringConstructOrSuperCall FunctionMetadataNode is also wrong. If it is false, don't we need class-field-initializattion? No. So this name is incorrect. I think FunctionMetadataNode::isConstructorAndNeedsClassFieldInitializer is better name. And keep UnlinkedCodeBlock::needsClassFieldInitializer's name as is.

Caitlin Potter (:caitp)

Comment 190 2020-01-15 12:15:18 PST

Created attachment 387818 [details] Part 1: Public Instance Fields (patch for landing)

Caitlin Potter (:caitp)

Comment 191 2020-01-15 12:17:24 PST

Comment on attachment 387361 [details] Part 1: Public Instance Fields (rev4) View in context: https://bugs.webkit.org/attachment.cgi?id=387361&action=review Last patch is ready to go, I'll submit when EWS is finished. >>>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1134 >>>>> + } >>>> >>>> This code means that, FunctionMetadataNode::needsClassFieldInitializer could be wrong (Even if it is saying NO, it depends on the upper CodeBlock's value). >>>> So, I don't think FunctionMetadataNode::needsClassFieldInitializer's name is correct. >>>> Please rename FunctionMetadataNode::needsClassFieldInitializer to appropriate one, and ensure it is appropriately set. >>>> For example, >>>> >>>> FunctionMetadataNode::isConstructorAndNeedsClassFieldInitializer >>>> And >>>> UnlinkedCodeBlock::needsClassFieldInitializer >>> >>> What this is really saying is, "this code is either a constructor which will need to initialize class fields, or is an arrow function nested within a derived class constructor which will need to initialize instance fields after `super()`" >>> >>> It is potentially a false-positive, if there is no super-call within the arrow function, but it's not possible to be a false negative, AFAIK. >>> >>> There might be a better name, like "needsClassFieldInitializeDuringConstructOrSuperCall" or something. But it's a bit wordy (and not that informative/helpful, IMHO). Do you think a comment would help explain this better? >> >> I think we should use `needsClassFieldInitializeDuringConstructOrSuperCall` for FunctionMetadataNode, because `needsClassFieldInitializer` name is incorrectt for FunctionMetadataNode :) > > Ah, no, needsClassFieldInitializeDuringConstructOrSuperCall FunctionMetadataNode is also wrong. If it is false, don't we need class-field-initializattion? No. So this name is incorrect. > I think FunctionMetadataNode::isConstructorAndNeedsClassFieldInitializer is better name. And keep UnlinkedCodeBlock::needsClassFieldInitializer's name as is. OK, per the discussion on IRC, I have come to agree and have made this change. I've opted to go with a boolean rather than an enum, in order to make conversion between this and the enum slightly simpler.

Caitlin Potter (:caitp)

Comment 192 2020-01-15 12:51:50 PST

Created attachment 387827 [details] Part 1: Public Instance Fields (patch for landing)

Ross Kirsling

Comment 193 2020-01-15 15:13:30 PST

(Weird. Your cq+ didn't actually cause the patch to be sent to the commit queue somehow. I thought it might be because r? was still set, but clearing that didn't help...)

Ross Kirsling

Comment 194 2020-01-15 15:14:32 PST

(In reply to Ross Kirsling from comment #193) > (Weird. Your cq+ didn't actually cause the patch to be sent to the commit > queue somehow. I thought it might be because r? was still set, but clearing > that didn't help...) ...or wait, yes it did! Guess it just took a moment.

WebKit Commit Bot

Comment 195 2020-01-15 16:09:08 PST

The commit-queue encountered the following flaky tests while processing attachment 387827 [details]: editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org) The commit-queue is continuing to process your patch.

WebKit Commit Bot

Comment 196 2020-01-15 16:09:55 PST

Comment on attachment 387827 [details] Part 1: Public Instance Fields (patch for landing) Clearing flags on attachment: 387827 Committed r254653: <https://trac.webkit.org/changeset/254653>

Caitlin Potter (:caitp)

Comment 197 2020-01-16 09:59:42 PST

Created attachment 387927 [details] Part 2: Private Instance Fields (rev5)

Saam Barati

Comment 198 2020-01-16 23:41:35 PST

(In reply to Caitlin Potter (:caitp) from comment #197) > Created attachment 387927 [details] > Part 2: Private Instance Fields (rev5) I think it is better for this to have its own bugzilla. I think we tend to have 1:1 bugzilla and commit for almost all changes, and especially for big changes.

Caitlin Potter (:caitp)

Comment 199 2020-01-17 11:13:22 PST

Moved review of part2 (private class fields) to https://bugs.webkit.org/show_bug.cgi?id=206431

Xan Lopez

Comment 200 2020-11-15 08:46:28 PST

This landed a while ago, I think we can close the bug?

Note You need to log in before you can comment on or make changes to this bug.