RESOLVED FIXED 144956
[ES6] Implement ES6 arrow function syntax. Arrow function specific features. Lexical bind of this
https://bugs.webkit.org/show_bug.cgi?id=144956
Summary [ES6] Implement ES6 arrow function syntax. Arrow function specific features. ...
GSkachkov
Reported 2015-05-13 10:48:17 PDT
It is necessary to implemented arrow specific feature - lexical bind of this. See small description https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions#Lexical_this
Attachments
Patch (88.65 KB, patch)
2015-05-13 12:14 PDT, GSkachkov
no flags
Patch (88.65 KB, patch)
2015-05-13 12:22 PDT, GSkachkov
no flags
Patch (88.59 KB, patch)
2015-05-13 12:48 PDT, GSkachkov
no flags
Patch (88.68 KB, patch)
2015-06-26 09:30 PDT, GSkachkov
no flags
Patch (91.51 KB, patch)
2015-06-30 14:14 PDT, GSkachkov
no flags
Patch (91.41 KB, patch)
2015-06-30 14:20 PDT, GSkachkov
no flags
Patch (91.46 KB, patch)
2015-06-30 14:54 PDT, GSkachkov
no flags
Patch (91.50 KB, patch)
2015-06-30 21:17 PDT, GSkachkov
no flags
Patch (91.90 KB, patch)
2015-07-01 00:11 PDT, GSkachkov
no flags
Patch (92.64 KB, patch)
2015-07-01 00:38 PDT, GSkachkov
no flags
Patch (104.84 KB, patch)
2015-07-01 01:13 PDT, GSkachkov
no flags
Patch (104.85 KB, patch)
2015-07-01 01:43 PDT, GSkachkov
no flags
Archive of layout-test-results from ews101 for mac-mavericks (577.53 KB, application/zip)
2015-07-01 03:05 PDT, Build Bot
no flags
Archive of layout-test-results from ews107 for mac-mavericks-wk2 (606.37 KB, application/zip)
2015-07-01 03:11 PDT, Build Bot
no flags
Patch (101.67 KB, patch)
2015-07-01 03:30 PDT, GSkachkov
no flags
Patch (106.17 KB, patch)
2015-07-01 14:39 PDT, GSkachkov
no flags
Archive of layout-test-results from ews101 for mac-mavericks (695.57 KB, application/zip)
2015-07-01 15:57 PDT, Build Bot
no flags
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (659.60 KB, application/zip)
2015-07-01 16:01 PDT, Build Bot
no flags
Patch (106.17 KB, patch)
2015-07-02 00:06 PDT, GSkachkov
no flags
Patch (106.55 KB, patch)
2015-07-02 01:30 PDT, GSkachkov
no flags
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (573.95 KB, application/zip)
2015-07-02 02:33 PDT, Build Bot
no flags
Archive of layout-test-results from ews101 for mac-mavericks (561.75 KB, text/plain)
2015-07-02 02:57 PDT, Build Bot
no flags
Patch (106.37 KB, patch)
2015-07-02 03:09 PDT, GSkachkov
no flags
Patch (103.45 KB, patch)
2015-07-03 14:27 PDT, GSkachkov
no flags
Patch (109.86 KB, patch)
2015-07-06 14:28 PDT, GSkachkov
no flags
Patch (109.98 KB, patch)
2015-07-06 14:36 PDT, GSkachkov
no flags
Patch (111.97 KB, patch)
2015-07-07 15:55 PDT, GSkachkov
no flags
Patch (108.97 KB, patch)
2015-07-08 00:59 PDT, GSkachkov
no flags
Patch (109.24 KB, patch)
2015-07-08 08:32 PDT, GSkachkov
no flags
Patch (109.00 KB, patch)
2015-07-15 00:45 PDT, GSkachkov
no flags
Patch (108.99 KB, patch)
2015-07-15 03:48 PDT, GSkachkov
no flags
Patch (150.64 KB, patch)
2015-07-22 01:36 PDT, GSkachkov
no flags
Dump (21.46 KB, application/octet-stream)
2015-07-22 08:35 PDT, GSkachkov
no flags
Dump of the ArrowFunction execution (21.46 KB, application/octet-stream)
2015-07-22 08:43 PDT, GSkachkov
no flags
Patch (163.27 KB, patch)
2015-07-22 14:55 PDT, GSkachkov
no flags
Patch (149.11 KB, patch)
2015-07-23 14:10 PDT, GSkachkov
no flags
Patch (148.00 KB, patch)
2015-07-23 23:43 PDT, GSkachkov
no flags
Patch (148.00 KB, patch)
2015-07-24 01:00 PDT, GSkachkov
no flags
Patch (148.52 KB, patch)
2015-07-24 08:10 PDT, GSkachkov
no flags
Patch (68.48 KB, patch)
2015-07-27 05:34 PDT, GSkachkov
no flags
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (1008.87 KB, application/zip)
2015-07-27 07:19 PDT, Build Bot
no flags
Archive of layout-test-results from ews101 for mac-mavericks (1006.76 KB, application/zip)
2015-07-27 07:43 PDT, Build Bot
no flags
Patch (68.52 KB, patch)
2015-07-27 07:56 PDT, GSkachkov
no flags
Patch (66.84 KB, patch)
2015-07-27 13:01 PDT, GSkachkov
no flags
Patch (2.15 MB, patch)
2015-07-29 09:08 PDT, GSkachkov
no flags
Patch (148.17 KB, patch)
2015-07-29 09:21 PDT, GSkachkov
no flags
Archive of layout-test-results from ews102 for mac-mavericks (568.03 KB, application/zip)
2015-07-29 10:13 PDT, Build Bot
no flags
Archive of layout-test-results from ews107 for mac-mavericks-wk2 (594.64 KB, application/zip)
2015-07-29 10:47 PDT, Build Bot
no flags
Patch (147.50 KB, patch)
2015-07-29 12:43 PDT, GSkachkov
no flags
Patch (156.39 KB, patch)
2015-07-31 08:17 PDT, GSkachkov
no flags
Archive of layout-test-results from ews103 for mac-mavericks (540.65 KB, application/zip)
2015-07-31 09:11 PDT, Build Bot
no flags
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (588.90 KB, application/zip)
2015-07-31 09:15 PDT, Build Bot
no flags
Patch (156.61 KB, patch)
2015-07-31 10:39 PDT, GSkachkov
no flags
Patch (171.88 KB, patch)
2015-07-31 13:42 PDT, GSkachkov
no flags
Patch (164.85 KB, patch)
2015-08-06 10:27 PDT, GSkachkov
buildbot: commit-queue-
Archive of layout-test-results from ews102 for mac-mavericks (539.96 KB, application/zip)
2015-08-06 11:26 PDT, Build Bot
no flags
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (604.93 KB, application/zip)
2015-08-06 11:32 PDT, Build Bot
no flags
Patch (165.00 KB, patch)
2015-08-06 12:26 PDT, GSkachkov
no flags
Patch (165.03 KB, patch)
2015-08-07 07:32 PDT, GSkachkov
no flags
Patch (165.07 KB, patch)
2015-08-07 09:27 PDT, GSkachkov
saam: review-
Patch (170.61 KB, patch)
2015-08-08 13:29 PDT, GSkachkov
no flags
Patch (171.27 KB, patch)
2015-08-09 09:54 PDT, GSkachkov
no flags
Patch (171.41 KB, patch)
2015-08-09 11:04 PDT, GSkachkov
no flags
Patch (172.67 KB, patch)
2015-08-12 07:52 PDT, GSkachkov
no flags
Patch (173.51 KB, patch)
2015-08-12 08:17 PDT, GSkachkov
no flags
Patch (172.60 KB, patch)
2015-08-12 08:56 PDT, GSkachkov
fpizlo: review+
fpizlo: commit-queue-
Patch (171.72 KB, patch)
2015-08-14 07:23 PDT, GSkachkov
no flags
Patch (172.17 KB, patch)
2015-08-14 09:26 PDT, GSkachkov
no flags
Patch (172.05 KB, patch)
2015-08-14 09:34 PDT, GSkachkov
no flags
Patch (172.04 KB, patch)
2015-08-14 12:43 PDT, GSkachkov
no flags
Patch (172.01 KB, patch)
2015-08-14 13:53 PDT, GSkachkov
no flags
Patch (172.52 KB, patch)
2015-08-14 14:47 PDT, GSkachkov
no flags
Patch (16.64 KB, patch)
2015-08-14 18:32 PDT, GSkachkov
no flags
Patch (23.49 KB, patch)
2015-08-14 18:48 PDT, GSkachkov
no flags
Patch (24.82 KB, patch)
2015-08-14 18:57 PDT, GSkachkov
no flags
Patch (27.88 KB, patch)
2015-08-14 19:06 PDT, GSkachkov
no flags
Patch (171.72 KB, patch)
2015-08-15 13:52 PDT, GSkachkov
no flags
Patch (172.61 KB, patch)
2015-08-17 12:18 PDT, GSkachkov
no flags
Patch (172.61 KB, patch)
2015-08-17 14:08 PDT, GSkachkov
no flags
GSkachkov
Comment 1 2015-05-13 12:14:58 PDT
WebKit Commit Bot
Comment 2 2015-05-13 12:17:57 PDT
Attachment 253042 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ChangeLog:31: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5] Total errors found: 1 in 67 files If any of these errors are false positives, please file a bug against check-webkit-style.
GSkachkov
Comment 3 2015-05-13 12:22:32 PDT
WebKit Commit Bot
Comment 4 2015-05-13 12:43:13 PDT
Attachment 253045 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ChangeLog:31: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5] Total errors found: 1 in 67 files If any of these errors are false positives, please file a bug against check-webkit-style.
GSkachkov
Comment 5 2015-05-13 12:48:08 PDT
GSkachkov
Comment 6 2015-05-13 12:49:57 PDT
Comment on attachment 253049 [details] Patch Can be landed only after landing of the task #144955 https://bugs.webkit.org/show_bug.cgi?id=144955
GSkachkov
Comment 7 2015-06-26 09:30:11 PDT
GSkachkov
Comment 8 2015-06-30 14:14:35 PDT
WebKit Commit Bot
Comment 9 2015-06-30 14:18:01 PDT
Attachment 255849 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jsc.cpp:1176: An else should appear on the same line as the preceding } [whitespace/newline] [4] Total errors found: 1 in 68 files If any of these errors are false positives, please file a bug against check-webkit-style.
GSkachkov
Comment 10 2015-06-30 14:20:54 PDT
GSkachkov
Comment 11 2015-06-30 14:54:10 PDT
GSkachkov
Comment 12 2015-06-30 21:17:13 PDT
GSkachkov
Comment 13 2015-07-01 00:11:04 PDT
GSkachkov
Comment 14 2015-07-01 00:38:33 PDT
GSkachkov
Comment 15 2015-07-01 01:13:19 PDT
GSkachkov
Comment 16 2015-07-01 01:43:10 PDT
Build Bot
Comment 17 2015-07-01 03:04:59 PDT
Comment on attachment 255910 [details] Patch Attachment 255910 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6123425340850176 New failing tests: js/arrowfunction-tostring.html
Build Bot
Comment 18 2015-07-01 03:05:04 PDT
Created attachment 255912 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
Build Bot
Comment 19 2015-07-01 03:11:14 PDT
Comment on attachment 255910 [details] Patch Attachment 255910 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4871530689331200 New failing tests: js/arrowfunction-tostring.html
Build Bot
Comment 20 2015-07-01 03:11:18 PDT
Created attachment 255913 [details] Archive of layout-test-results from ews107 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
GSkachkov
Comment 21 2015-07-01 03:30:15 PDT
GSkachkov
Comment 22 2015-07-01 14:39:24 PDT
Build Bot
Comment 23 2015-07-01 15:57:53 PDT
Comment on attachment 255954 [details] Patch Attachment 255954 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4828512028459008 New failing tests: js/arrowfunction-lexical-this-2.html js/arrowfunction-lexical-this-1.html
Build Bot
Comment 24 2015-07-01 15:57:57 PDT
Created attachment 255960 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
Build Bot
Comment 25 2015-07-01 16:01:44 PDT
Comment on attachment 255954 [details] Patch Attachment 255954 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5855855589195776 New failing tests: js/arrowfunction-lexical-this-2.html js/arrowfunction-lexical-this-1.html
Build Bot
Comment 26 2015-07-01 16:01:48 PDT
Created attachment 255961 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
GSkachkov
Comment 27 2015-07-02 00:06:07 PDT
GSkachkov
Comment 28 2015-07-02 01:30:26 PDT
Build Bot
Comment 29 2015-07-02 02:33:05 PDT
Comment on attachment 256000 [details] Patch Attachment 256000 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6357477436162048 New failing tests: js/arrowfunction-lexical-this-2.html
Build Bot
Comment 30 2015-07-02 02:33:10 PDT
Created attachment 256001 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Build Bot
Comment 31 2015-07-02 02:57:16 PDT
Comment on attachment 256000 [details] Patch Attachment 256000 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5494963445956608 New failing tests: js/arrowfunction-lexical-this-2.html
Build Bot
Comment 32 2015-07-02 02:57:21 PDT
Created attachment 256002 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
GSkachkov
Comment 33 2015-07-02 03:09:02 PDT
Filip Pizlo
Comment 34 2015-07-03 13:00:47 PDT
Comment on attachment 256003 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256003&action=review The thing that should be conditional on ENABLE_ES6_ARROWFUNCTION_SYNTAX is the code in the parser. Then, you could just make JSArrowFunction be unconditionally available, which removes all of the #if's in the compiler. > Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj:1 > -<?xml version="1.0" encoding="utf-8"?> > +<?xml version="1.0" encoding="utf-8"?> You should probably revert this. > Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:92 > +#define FOR_EACH_ABSTRACT_FIELD_ARROWFUNCTION(macro) \ If you made JSArrowFunction non-conditional, then you wouldn't need this. > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3147 > +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX) Ditto. > Source/JavaScriptCore/runtime/JSArrowFunction.h:29 > +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX) I would remove this conditional here. You could always declare the class. Then, you need fewer conditionals elsewhere in the system, and it simplifies how you do the abstract heap repo in the FTL, for example.
GSkachkov
Comment 35 2015-07-03 14:27:38 PDT
GSkachkov
Comment 36 2015-07-03 14:57:56 PDT
Comment on attachment 256003 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256003&action=review I've removed conditions from non parser modules. >> Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj:1 >> +<?xml version="1.0" encoding="utf-8"?> > > You should probably revert this. Done
Saam Barati
Comment 37 2015-07-04 20:56:31 PDT
Comment on attachment 256121 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256121&action=review > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1822 > +RegisterID* BytecodeGenerator::emitNewArrowFunctionExpression(RegisterID* r0, FuncExprNode* n) Use better variable names here. I'd use 'destination' and 'function' or something similar. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4522 > +void SpeculativeJIT::compileNewArrowFunction(Node* node) This and compileNewFunction(.) share a lot of code. I'd find a way to abstract out the common bits into a helper function. > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3145 > + void compileNewArrowFunction() Ditto here. Find a way to abstract this. > Source/JavaScriptCore/jit/JITOpcodes.cpp:1003 > +void JIT::emit_op_new_arrow_func_exp(Instruction* currentInstruction) Ditto. Find a way to abstract this. It's not good to have two functions doing almost identical computations. > Source/JavaScriptCore/jit/JITOpcodes.cpp:1011 > + store64(TrustedImm64(JSValue::encode(jsUndefined())), Address(callFrameRegister, sizeof(Register) * dst)); Do we have a helper function for this? > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:177 > +#define LLINT_CALC_PROPERTY_FOR_FUNC_EXPR() \ I don't love this being a macro. Maybe you can create one function that returns JSFunction and op_new_func_exp can just return that while op_new_arrow_func can use it as a helper function. > Source/JavaScriptCore/runtime/JSArrowFunction.cpp:1 > +/* I still need to read through this and the rest of the patch.
GSkachkov
Comment 38 2015-07-06 14:28:51 PDT
GSkachkov
Comment 39 2015-07-06 14:36:44 PDT
GSkachkov
Comment 40 2015-07-06 14:40:23 PDT
Comment on attachment 256121 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256121&action=review Saam Barati: I've tried to fix comments. Please take a looks if it make sense. >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1822 >> +RegisterID* BytecodeGenerator::emitNewArrowFunctionExpression(RegisterID* r0, FuncExprNode* n) > > Use better variable names here. I'd use 'destination' and 'function' or something similar. Refactored and renamed >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4522 >> +void SpeculativeJIT::compileNewArrowFunction(Node* node) > > This and compileNewFunction(.) share a lot of code. I'd find a way to abstract out the common bits into a helper function. Reractored >> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3145 >> + void compileNewArrowFunction() > > Ditto here. Find a way to abstract this. Refactored >> Source/JavaScriptCore/jit/JITOpcodes.cpp:1003 >> +void JIT::emit_op_new_arrow_func_exp(Instruction* currentInstruction) > > Ditto. Find a way to abstract this. It's not good to have two functions doing almost identical computations. Refactored >> Source/JavaScriptCore/jit/JITOpcodes.cpp:1011 >> + store64(TrustedImm64(JSValue::encode(jsUndefined())), Address(callFrameRegister, sizeof(Register) * dst)); > > Do we have a helper function for this? I didn't find >> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:177 >> +#define LLINT_CALC_PROPERTY_FOR_FUNC_EXPR() \ > > I don't love this being a macro. Maybe you can create one function that returns JSFunction and op_new_func_exp can just return that while op_new_arrow_func can use it as a helper function. Refactored
Saam Barati
Comment 41 2015-07-07 10:57:04 PDT
Comment on attachment 256242 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256242&action=review This is looking better. Some suggestions below. I think other people should also take a look because I'm not the best person to review all of this patch. Will the DFG inline an ArrowFunctions or will it just never inline them? > Source/JavaScriptCore/ChangeLog:9 > + In patch were implemented the following cases cases: "cases cases" => "cases" > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1809 > +void BytecodeGenerator::emitNewFunctionAbstract(RegisterID* dst, FuncExprNode* func, OpcodeID opcodeID) Call this function "emitNewFunctionCommon" and also assert that opcodeID is what you expect. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1817 > instructions().append(index); I would append thisRegister conditionally here: if (opcodeID == op_new_arrow_func_exp) instruction().append(thisRegister()->index()) > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1829 > + emitTDZCheck(thisRegister()); Do we need a TDZ check here? If so, why? > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1831 > + instructions().append(thisRegister()->index()); Above code change will let you remove this. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4564 > + ? m_jit.graph().globalObjectFor(node->origin.semantic)->arrowFunctionStructure() Indent four spaces (and to all below). > Source/JavaScriptCore/jit/JITOpcodes.cpp:986 > +void JIT::emit_op_new_abstract_func_exp(Instruction* currentInstruction, FunctionType functionType) Nit: I would camel case this and call it: emitNewFuncExprCommon. > Source/JavaScriptCore/jit/JITOperations.cpp:1024 > +EncodedJSValue JIT_OPERATION operationNewArrowFunctionWithInvalidatedReallocationWatchpoint(ExecState* exec, JSScope* scope, JSCell* functionExecutable, EncodedJSValue thisValue) Nit: This function and opeartionNewArrowFunction are nearly identical. There is probably an easy way to combine them. > Source/JavaScriptCore/runtime/JSArrowFunction.cpp:99 > + // Fixme: m_arrowFunction already removed by GC, because it created in before run JSArrowFunction::create method What's happening with this?
GSkachkov
Comment 42 2015-07-07 15:55:46 PDT
GSkachkov
Comment 43 2015-07-07 15:58:50 PDT
Comment on attachment 256242 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256242&action=review >> Source/JavaScriptCore/ChangeLog:9 >> + In patch were implemented the following cases cases: > > "cases cases" => "cases" Done >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1809 >> +void BytecodeGenerator::emitNewFunctionAbstract(RegisterID* dst, FuncExprNode* func, OpcodeID opcodeID) > > Call this function "emitNewFunctionCommon" > and also assert that opcodeID is what you expect. Done >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1817 >> instructions().append(index); > > I would append thisRegister conditionally here: > if (opcodeID == op_new_arrow_func_exp) > instruction().append(thisRegister()->index()) Done >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1829 >> + emitTDZCheck(thisRegister()); > > Do we need a TDZ check here? If so, why? See comment from Ryosuke Niwa https://bugs.webkit.org/show_bug.cgi?id=140855#c66 Without this TDZ checks following test will fail LayoutTests/js/script-tests/arrowfunction-tdz.js >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1831 >> + instructions().append(thisRegister()->index()); > > Above code change will let you remove this. Done >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4564 >> + ? m_jit.graph().globalObjectFor(node->origin.semantic)->arrowFunctionStructure() > > Indent four spaces (and to all below). Done >> Source/JavaScriptCore/jit/JITOpcodes.cpp:986 >> +void JIT::emit_op_new_abstract_func_exp(Instruction* currentInstruction, FunctionType functionType) > > Nit: I would camel case this and call it: > emitNewFuncExprCommon. Done >> Source/JavaScriptCore/jit/JITOperations.cpp:1024 >> +EncodedJSValue JIT_OPERATION operationNewArrowFunctionWithInvalidatedReallocationWatchpoint(ExecState* exec, JSScope* scope, JSCell* functionExecutable, EncodedJSValue thisValue) > > Nit: This function and opeartionNewArrowFunction are nearly identical. There is probably an easy way > to combine them. Done >> Source/JavaScriptCore/runtime/JSArrowFunction.cpp:99 >> + // Fixme: m_arrowFunction already removed by GC, because it created in before run JSArrowFunction::create method > > What's happening with this? Actually I don't know. When JSArrowFunction::visitChildren is invoked the thisObject->m_arrowFunction property is already empty so it lead to error. I susspect that this property is already deleted by GC.
GSkachkov
Comment 44 2015-07-08 00:59:35 PDT
GSkachkov
Comment 45 2015-07-08 02:19:19 PDT
(In reply to comment #41) > Comment on attachment 256242 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=256242&action=review > > This is looking better. Some suggestions below. > I think other people should also take a look because I'm not the best person > to review all of this patch. > > Will the DFG inline an ArrowFunctions or will it just never inline them? > Mmm, I not sure. In most cases ArrowFunction repeats features of the Function, so I expect that DFG inlines the ArrowFunctions, but can't see any prove of that.
GSkachkov
Comment 46 2015-07-08 08:32:51 PDT
Saam Barati
Comment 47 2015-07-14 08:23:28 PDT
Comment on attachment 256376 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256376&action=review > Source/JavaScriptCore/runtime/JSArrowFunction.cpp:101 > + // visitor.append(&thisObject->m_arrowFunction); This should be fixed. > Source/JavaScriptCore/runtime/JSArrowFunction.h:69 > + const static unsigned StructureFlags = OverridesHasInstance | Base::StructureFlags; Looks like you're reporting the wrong things here. It should have OverridesVisitChildren and it looks like it should not have OverridesHasInstance. Let's see if that fixes GC issues.
GSkachkov
Comment 48 2015-07-15 00:29:26 PDT
Comment on attachment 256376 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256376&action=review >> Source/JavaScriptCore/runtime/JSArrowFunction.cpp:101 >> + // visitor.append(&thisObject->m_arrowFunction); > > This should be fixed. I can't reproduce this bug now :(. Possible it was fixed somewhere else. >> Source/JavaScriptCore/runtime/JSArrowFunction.h:69 >> + const static unsigned StructureFlags = OverridesHasInstance | Base::StructureFlags; > > Looks like you're reporting the wrong things here. > It should have OverridesVisitChildren and it looks like it should > not have OverridesHasInstance. > > Let's see if that fixes GC issues. I've removed OverridesHasInstance but I didn't find OverridesVisitChildren method. Anyway it doesn't raise exception with now. Possible it was bug somewhere else.
GSkachkov
Comment 49 2015-07-15 00:45:02 PDT
GSkachkov
Comment 50 2015-07-15 03:48:27 PDT
Saam Barati
Comment 51 2015-07-15 10:02:43 PDT
(In reply to comment #48) > Comment on attachment 256376 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=256376&action=review > > >> Source/JavaScriptCore/runtime/JSArrowFunction.cpp:101 > >> + // visitor.append(&thisObject->m_arrowFunction); > > > > This should be fixed. > > I can't reproduce this bug now :(. Possible it was fixed somewhere else. > > >> Source/JavaScriptCore/runtime/JSArrowFunction.h:69 > >> + const static unsigned StructureFlags = OverridesHasInstance | Base::StructureFlags; > > > > Looks like you're reporting the wrong things here. > > It should have OverridesVisitChildren and it looks like it should > > not have OverridesHasInstance. > > > > Let's see if that fixes GC issues. > > I've removed OverridesHasInstance but I didn't find OverridesVisitChildren > method. Anyway it doesn't raise exception with now. Possible it was bug > somewhere else. Yes. I don't think that exists anymore. I saw that flag on an old repository on github while on my phone.
Filip Pizlo
Comment 52 2015-07-15 13:22:39 PDT
Comment on attachment 256831 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256831&action=review > Source/JavaScriptCore/runtime/JSArrowFunction.cpp:56 > +EncodedJSValue JSC_HOST_CALL arrowFunctionCall(ExecState* exec) > +{ > + JSArrowFunction* arrowFunction = jsCast<JSArrowFunction*>(exec->callee()); > + > + MarkedArgumentBuffer args; > + for (unsigned i = 0; i < exec->argumentCount(); ++i) > + args.append(exec->uncheckedArgument(i)); > + > + JSObject* function = arrowFunction->arrowFunction(); > + CallData callData; > + CallType callType = getCallData(function, callData); > + ASSERT(callType != CallTypeNone); > + return JSValue::encode(call(exec, function, callType, callData, arrowFunction->boundThis(), args)); > +} This is pretty bad. You should implement this in a way that doesn't require every arrow function call to call into native and then back into JS. This will be extremely slow.
GSkachkov
Comment 53 2015-07-22 01:36:15 PDT
GSkachkov
Comment 54 2015-07-22 03:06:02 PDT
Current patch is not ready for landing. Known issue in using arrow function after FTL/DFG optimization.
GSkachkov
Comment 55 2015-07-22 08:35:22 PDT
Created attachment 257272 [details] Dump Dump of working arrow function with error.
GSkachkov
Comment 56 2015-07-22 08:43:33 PDT
Created attachment 257275 [details] Dump of the ArrowFunction execution
GSkachkov
Comment 57 2015-07-22 14:55:46 PDT
GSkachkov
Comment 58 2015-07-23 14:10:41 PDT
GSkachkov
Comment 59 2015-07-23 23:43:54 PDT
GSkachkov
Comment 60 2015-07-24 01:00:14 PDT
GSkachkov
Comment 61 2015-07-24 08:10:26 PDT
GSkachkov
Comment 62 2015-07-24 12:57:53 PDT
(In reply to comment #52) > Comment on attachment 256831 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=256831&action=review > > > Source/JavaScriptCore/runtime/JSArrowFunction.cpp:56 > > +EncodedJSValue JSC_HOST_CALL arrowFunctionCall(ExecState* exec) > > +{ > > + JSArrowFunction* arrowFunction = jsCast<JSArrowFunction*>(exec->callee()); > > + > > + MarkedArgumentBuffer args; > > + for (unsigned i = 0; i < exec->argumentCount(); ++i) > > + args.append(exec->uncheckedArgument(i)); > > + > > + JSObject* function = arrowFunction->arrowFunction(); > > + CallData callData; > > + CallType callType = getCallData(function, callData); > > + ASSERT(callType != CallTypeNone); > > + return JSValue::encode(call(exec, function, callType, callData, arrowFunction->boundThis(), args)); > > +} > > This is pretty bad. You should implement this in a way that doesn't require > every arrow function call to call into native and then back into JS. This > will be extremely slow. New fix, with help of the Saam Barati, implemented without switching between native and JS.
GSkachkov
Comment 63 2015-07-27 05:34:39 PDT
Build Bot
Comment 64 2015-07-27 07:19:10 PDT
Comment on attachment 257559 [details] Patch Attachment 257559 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4537078633201664 New failing tests: js/arrowfunction-syntax.html js/arrowfunction-lexical-this-1.html js/arrowfunction-call.html js/arrowfunction-asparamter-2.html js/arrowfunction-associativity-1.html js/arrowfunction-block-2.html js/arrowfunction-strict-mode.html js/arrowfunction-bind.html js/arrowfunction-associativity-2.html js/arrowfunction-lexical-this-2.html js/arrowfunction-lexical-this-3.html js/arrowfunction-syntax-endings.html js/arrowfunction-asparamter-1.html js/arrowfunction-block-1.html
Build Bot
Comment 65 2015-07-27 07:19:14 PDT
Created attachment 257560 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Build Bot
Comment 66 2015-07-27 07:43:03 PDT
Comment on attachment 257559 [details] Patch Attachment 257559 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6715932722331648 New failing tests: js/arrowfunction-syntax.html js/arrowfunction-block-1.html js/arrowfunction-lexical-this-1.html js/arrowfunction-asparamter-2.html js/arrowfunction-associativity-1.html js/arrowfunction-block-2.html js/arrowfunction-strict-mode.html js/arrowfunction-bind.html js/arrowfunction-associativity-2.html js/arrowfunction-lexical-this-2.html js/arrowfunction-lexical-this-3.html js/arrowfunction-syntax-endings.html js/arrowfunction-asparamter-1.html js/arrowfunction-call.html
Build Bot
Comment 67 2015-07-27 07:43:07 PDT
Created attachment 257561 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
GSkachkov
Comment 68 2015-07-27 07:56:33 PDT
Saam Barati
Comment 69 2015-07-27 11:43:07 PDT
Comment on attachment 257562 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257562&action=review > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:515 > + m_arrowFunctionThisRegister = addVar(); This shouldn't be an addVar(). It should be a temporary. Also, I'd put this "if" statement above the "m_TDZStack.append(...)" line. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2201 > +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX) You don't need guars here. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:304 > + RegisterID* arrowFunctionThisRegister() { return m_arrowFunctionThisRegister; } Don't need this. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:481 > +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX) You don't need guards here. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:483 > + RegisterID* emitThisNodeBytecode(RegisterID*, JSTextPosition); This looks like it should be removed. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:766 > + RegisterID* m_arrowFunctionThisRegister { nullptr }; We don't need this to be a field. > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3070 > +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX) Don't need guards here. > Source/JavaScriptCore/parser/NodeConstructors.h:848 > +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX) Don't need guards here. > Source/JavaScriptCore/parser/Nodes.h:167 > +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX) ditto. > Source/JavaScriptCore/parser/Nodes.h:1742 > +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX) ditto. > Source/JavaScriptCore/runtime/CommonIdentifiers.h:187 > + macro(__private_arrowfunction_this__) \ You probably want this under the "JSC_COMMON_PRIVATE_IDENTIFIERS_EACH_PROPERTY_NAME" macro. Also, there is no need to make this a crazy name with underscores. Just call it "arrowFunctionBoundThis" or something similar. Also, make sure that this property is not visible to JS code. > Source/JavaScriptCore/runtime/JSFunction.cpp:132 > +void JSFunction::finishCreation(VM& vm) Is this called? > Source/JavaScriptCore/runtime/JSFunction.h:147 > using Base::finishCreation; Maybe we no longer want this line?
GSkachkov
Comment 70 2015-07-27 13:01:02 PDT
GSkachkov
Comment 71 2015-07-27 13:01:04 PDT
Comment on attachment 257562 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257562&action=review >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:515 >> + m_arrowFunctionThisRegister = addVar(); > > This shouldn't be an addVar(). It should be a temporary. > Also, I'd put this "if" statement above the "m_TDZStack.append(...)" line. Done >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2201 >> +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX) > > You don't need guars here. Done >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:304 >> + RegisterID* arrowFunctionThisRegister() { return m_arrowFunctionThisRegister; } > > Don't need this. Done >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:481 >> +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX) > > You don't need guards here. Done >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:483 >> + RegisterID* emitThisNodeBytecode(RegisterID*, JSTextPosition); > > This looks like it should be removed. Done >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:766 >> + RegisterID* m_arrowFunctionThisRegister { nullptr }; > > We don't need this to be a field. Done >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3070 >> +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX) > > Don't need guards here. OK. I've removed. What is difference with ES6_CLASS_SYNTAX guards? >> Source/JavaScriptCore/parser/NodeConstructors.h:848 >> +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX) > > Don't need guards here. Done >> Source/JavaScriptCore/parser/Nodes.h:167 >> +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX) > > ditto. Done >> Source/JavaScriptCore/parser/Nodes.h:1742 >> +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX) > > ditto. Done >> Source/JavaScriptCore/runtime/CommonIdentifiers.h:187 >> + macro(__private_arrowfunction_this__) \ > > You probably want this under the "JSC_COMMON_PRIVATE_IDENTIFIERS_EACH_PROPERTY_NAME" macro. > Also, there is no need to make this a crazy name with underscores. Just call it "arrowFunctionBoundThis" or something > similar. Also, make sure that this property is not visible to JS code. Done >> Source/JavaScriptCore/runtime/JSFunction.cpp:132 >> +void JSFunction::finishCreation(VM& vm) > > Is this called? Yes, it is called in createImpl function >> Source/JavaScriptCore/runtime/JSFunction.h:147 >> using Base::finishCreation; > > Maybe we no longer want this line? Done
GSkachkov
Comment 72 2015-07-29 09:08:59 PDT
GSkachkov
Comment 73 2015-07-29 09:21:07 PDT
Build Bot
Comment 74 2015-07-29 10:13:43 PDT
Comment on attachment 257753 [details] Patch Attachment 257753 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5993309172400128 New failing tests: js/arrowfunction-bind.html js/arrowfunction-lexical-this-2.html js/arrowfunction-lexical-this-3.html js/arrowfunction-lexical-this-1.html js/arrowfunction-call.html
Build Bot
Comment 75 2015-07-29 10:13:47 PDT
Created attachment 257755 [details] Archive of layout-test-results from ews102 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Build Bot
Comment 76 2015-07-29 10:46:55 PDT
Comment on attachment 257753 [details] Patch Attachment 257753 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4633625605701632 New failing tests: js/arrowfunction-bind.html js/arrowfunction-lexical-this-2.html js/arrowfunction-lexical-this-3.html js/arrowfunction-lexical-this-1.html js/arrowfunction-call.html
Build Bot
Comment 77 2015-07-29 10:47:01 PDT
Created attachment 257756 [details] Archive of layout-test-results from ews107 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
GSkachkov
Comment 78 2015-07-29 12:43:41 PDT
Created attachment 257762 [details] Patch Fix tests after merge & small fixes from previous comment
Saam Barati
Comment 79 2015-07-30 15:41:09 PDT
Comment on attachment 257762 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257762&action=review > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:953 > + fixEdge<FinalObjectUse>(node->child1()); This should probably be: fixEdge<KnownCellUse>(node->child1()) > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:954 > + node->convertToIdentity(); This seems wrong. This should be removed. > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1275 > + m_insertionSet.insertNode(m_indexInBlock, SpecNone, Check, node->origin, What is this doing? I think it should be removed. > Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:843 > + case NewArrowFunction: Lets focus on getting this properly working in the DFG before spending time getting the allocation sinking phase right. Allocation sinking is only run when we compile into the FTL. That said, some basic comments below: We will need to change this a bit to incorporate the bound this of the arrow function. Look below at FunctionActivationPLoc/FunctionExecutablePLoc. Maybe you would create a FunctionBoundThisPLoc. > Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:988 > + if (target && target->isFunctionAllocation()) isFunctionAllocation() should return true for NewArrowFunction. > Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:990 > + else Why would this ever not be a function? That seems like it'd be wrong. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4599 > +void SpeculativeJIT::compileNewFunction(Node* node, NodeType nodeType) You don't need NodeType as a parameter to this function. Node will have that information.
GSkachkov
Comment 80 2015-07-31 08:17:50 PDT
Build Bot
Comment 81 2015-07-31 09:11:03 PDT
Comment on attachment 257921 [details] Patch Attachment 257921 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3059 New failing tests: js/regress/arrowfunction-call.html
Build Bot
Comment 82 2015-07-31 09:11:10 PDT
Created attachment 257923 [details] Archive of layout-test-results from ews103 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Build Bot
Comment 83 2015-07-31 09:15:11 PDT
Comment on attachment 257921 [details] Patch Attachment 257921 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3068 New failing tests: js/regress/arrowfunction-call.html
Build Bot
Comment 84 2015-07-31 09:15:18 PDT
Created attachment 257924 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
GSkachkov
Comment 85 2015-07-31 10:39:37 PDT
GSkachkov
Comment 86 2015-07-31 13:42:06 PDT
Created attachment 257956 [details] Patch New patch with h allocation sink tests
GSkachkov
Comment 87 2015-07-31 13:44:31 PDT
Comment on attachment 257762 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257762&action=review >> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:953 >> + fixEdge<FinalObjectUse>(node->child1()); > > This should probably be: > fixEdge<KnownCellUse>(node->child1()) Done >> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:954 >> + node->convertToIdentity(); > > This seems wrong. This should be removed. Done >> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1275 >> + m_insertionSet.insertNode(m_indexInBlock, SpecNone, Check, node->origin, > > What is this doing? > I think it should be removed. Ohh, I've removed only line 1274, without 1275 it raises error during tests >> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:843 >> + case NewArrowFunction: > > Lets focus on getting this properly working in the DFG before spending time getting the allocation sinking phase right. > Allocation sinking is only run when we compile into the FTL. That said, some basic comments below: > > We will need to change this a bit to incorporate the bound this of the arrow function. > Look below at FunctionActivationPLoc/FunctionExecutablePLoc. Maybe you would > create a FunctionBoundThisPLoc. I've added new FunctionBoundThisPLoc >> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:988 >> + if (target && target->isFunctionAllocation()) > > isFunctionAllocation() should return true for NewArrowFunction. Done >> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:990 >> + else > > Why would this ever not be a function? That seems like it'd be wrong. It is the same as for GetScope, except instead of FunctionActivationPLoc is used FunctionActivationPLoc >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4599 >> +void SpeculativeJIT::compileNewFunction(Node* node, NodeType nodeType) > > You don't need NodeType as a parameter to this function. Node will have that information. Done
Saam Barati
Comment 88 2015-08-01 09:26:50 PDT
Comment on attachment 257956 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257956&action=review > Source/JavaScriptCore/bytecode/BytecodeUseDef.h:119 > + case op_new_arrow_func_exp: This should also report "bound this" byte code operand as a use. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:513 > + RefPtr<RegisterID> arrowFunctionBoundThis = emitLoadArrowFunctionThis(addVar()); this should be a temporary register, not an addVar(). > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2868 > + arrowFunctionThis->ref(); don't ref this. this is an anti-pattern. > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2962 > +// ------------------------------ ArrowFuncExprNode --------------------------------- Nit: new line after comment to follow style of the file. > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1744 > + if (JSArrowFunction* function = jsDynamicCast<JSArrowFunction*>(base)) { Will this every be nullptr when base is truthy? > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3726 > + result = weakJSConstant(function->boundThis()); I don't think this needs to be weak. I don't think it will make any difference because the arrow function will always keep bound this alive, but it seems more correct to me to not be weak. If something is weak, it means that if that cell is GCed, then the resulting DFG code goes away. If something is strong, it means the DFG code keeps that cell alive. I think strong makes more sense here. > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1273 > + m_insertionSet.insertNode(m_indexInBlock, SpecNone, Check, node->origin, What is this doing? Why are we doing this instead of "fixEdge<CellUse>(node->child2())"? > Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:843 > + case NewArrowFunction: { I would combine this case with the case below so you don't repeat code. You can just do: "if (op == NewArrowFunction) write.add(FunctionBoundThisPLoc ...)" > Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:991 > + exactRead = FunctionActivationPLoc; This is wrong. Revert. > Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:997 > + case LoadArrowFunctionThis: I don't think we need to worry about LoadArrowFunctionThis in the allocation sinking phase. This will just be loading from callee which should be materialized already at the top of a function. > Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:1998 > + case NewArrowFunction: { I think we can combine this and the below case and make necessary changes/asserts based on if op is NewArrowFunction or NewFunction > Source/JavaScriptCore/dfg/DFGPromoteHeapAccess.h:74 > + case LoadArrowFunctionThis: I think this can be removed. It doesn't seem correct. We're using FunctionBoundThisPLoc when sinking the arrow function, not when actually executing an arrow function (LoadArrowFunctionThis is only used when Arrow function is the callee). > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3074 > + void compileNewFunction(NodeType nodeType) We don't need NodeType as a parameter here. m_node should have all the info you need. > Source/JavaScriptCore/jit/JIT.h:642 > + void emitNewFuncExprCommon(Instruction*, FunctionType); If you have Instruction* you don't need FunctionType. You can just look at the opcode to determine if it's an arrow function or not. > Source/JavaScriptCore/jit/JITOperations.cpp:969 > +EncodedJSValue operationNewArrowFunctionCommon(ExecState* exec, JSScope* scope, JSCell* functionExecutable, EncodedJSValue thisValue, bool isInvalidated) I don't think you need to declare this in the header. You can just make this a static function in this file. > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1016 > +inline JSFunction* getJSFunctionExpr(ExecState* exec, Instruction* pc, VM& vm) Let's get rid of this function now that it only has one caller. It made more sense before you refactored how the arrow function object layout. > Source/JavaScriptCore/runtime/JSArrowFunction.cpp:40 > + There are some thing in this file where I'm the wrong person to review them. I'll see if somebody else can take a look. > Source/JavaScriptCore/runtime/JSArrowFunction.cpp:51 > + executable->singletonFunction()->notifyWrite(vm, result, "Allocating a arrow function"); nit: "a" => "an" > Source/JavaScriptCore/runtime/JSArrowFunction.cpp:85 > +} nit: newline after this. > LayoutTests/js/script-tests/arrowfunction-lexical-this-1.js:1 > +description('Tests for ES6 arrow function lexical bind of this'); I would combine "arrow function-lexical-this-1/2/3" into one file or give them more descriptive names. > LayoutTests/js/script-tests/arrowfunction-lexical-this-2.js:9 > +shouldBe("afObjLexBind2Instance.func() === afObjLexBind2Instance", "true"); Can we also try doing stuff like: afObjLexBind2.call(V, ...) where V might be many different things? Basically, we should test "bound" this outside a constructor and with different this values. (Maybe a separate test).
Basile Clement
Comment 89 2015-08-01 11:10:16 PDT
Comment on attachment 257956 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257956&action=review Saam asked me to comment on allocation sinking stuff, so here it is. > Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:848 > + target = &m_heap.newAllocation(node, Allocation::Kind::Function); This is wrong. You need to add Allocation::Kind::ArrowFunction and use it here, otherwise you are transforming an arrow function into a regular function when sinking (not eliminating). You should have isFunction() return true for Kind::ArrowFunction as well, and define a new isArrowFunction() helper in the Allocation class. Here is a test case for this, if my understanding of arrow functions is correct: ```` function f(p) { var arr = (x) => this; if (p) return arr; } for (var i = 0; i < 100000; ++i) f(i % 2); if (f(true)() != f) throw new Error("Bad sinking."); ```` >> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:997 >> + case LoadArrowFunctionThis: > > I don't think we need to worry about LoadArrowFunctionThis in the allocation > sinking phase. This will just be loading from callee which should be materialized already at the top of a function. We'd need to worry about it if we inlined arrow functions, which this patch does not handle by the look of it. If the plan is to implement inlining of arrow functions later, this should at least be a FIXME. Otherwise, this should check for target->isArrowFunctionAllocation() instead; cf above. >> Source/JavaScriptCore/dfg/DFGPromoteHeapAccess.h:74 >> + case LoadArrowFunctionThis: > > I think this can be removed. It doesn't seem correct. > We're using FunctionBoundThisPLoc when sinking the arrow function, not when > actually executing an arrow function (LoadArrowFunctionThis is only used when Arrow function is the callee). The whole file should be removed. It was used by the old object allocation sinking phase and is no longer included by anyone. I'll put up a patch to remove it.
Basile Clement
Comment 90 2015-08-01 11:12:03 PDT
(In reply to comment #89) > > Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:848 > > + target = &m_heap.newAllocation(node, Allocation::Kind::Function); > > This is wrong. You need to add Allocation::Kind::ArrowFunction and use it > here, otherwise you are transforming an arrow function into a regular > function when sinking (not eliminating). Look at the createMaterialization() function.
GSkachkov
Comment 91 2015-08-06 10:27:03 PDT
Build Bot
Comment 92 2015-08-06 11:26:07 PDT
Comment on attachment 258372 [details] Patch Attachment 258372 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/23719 New failing tests: js/arrowfunction-lexical-bind-this.html
Build Bot
Comment 93 2015-08-06 11:26:13 PDT
Created attachment 258377 [details] Archive of layout-test-results from ews102 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Build Bot
Comment 94 2015-08-06 11:32:07 PDT
Comment on attachment 258372 [details] Patch Attachment 258372 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/23733 New failing tests: js/arrowfunction-lexical-bind-this.html
Build Bot
Comment 95 2015-08-06 11:32:12 PDT
Created attachment 258381 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
GSkachkov
Comment 96 2015-08-06 12:26:27 PDT
GSkachkov
Comment 97 2015-08-06 12:54:03 PDT
Comment on attachment 257956 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257956&action=review >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:513 >> + RefPtr<RegisterID> arrowFunctionBoundThis = emitLoadArrowFunctionThis(addVar()); > > this should be a temporary register, not an addVar(). In new patch I've used directly m_thisRegister, without any temp and var, Is it Ok? >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2868 >> + arrowFunctionThis->ref(); > > don't ref this. this is an anti-pattern. Done >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2962 >> +// ------------------------------ ArrowFuncExprNode --------------------------------- > > Nit: new line after comment to follow style of the file. Done >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1744 >> + if (JSArrowFunction* function = jsDynamicCast<JSArrowFunction*>(base)) { > > Will this every be nullptr when base is truthy? I hope so, do I need to change to some another condition? >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3726 >> + result = weakJSConstant(function->boundThis()); > > I don't think this needs to be weak. I don't think it will make any difference because the arrow function > will always keep bound this alive, but it seems more correct to me to not be weak. > > If something is weak, it means that if that cell is GCed, then the resulting DFG code goes away. > If something is strong, it means the DFG code keeps that cell alive. I think strong makes more sense here. Done, it is jsConstant now >> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1273 >> + m_insertionSet.insertNode(m_indexInBlock, SpecNone, Check, node->origin, > > What is this doing? > Why are we doing this instead of "fixEdge<CellUse>(node->child2())"? Fixed >> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:843 >> + case NewArrowFunction: { > > I would combine this case with the case below so you don't repeat code. > You can just do: > "if (op == NewArrowFunction) write.add(FunctionBoundThisPLoc ...)" Done >> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:848 >> + target = &m_heap.newAllocation(node, Allocation::Kind::Function); > > This is wrong. You need to add Allocation::Kind::ArrowFunction and use it here, otherwise you are transforming an arrow function into a regular function when sinking (not eliminating). > You should have isFunction() return true for Kind::ArrowFunction as well, and define a new isArrowFunction() helper in the Allocation class. Here is a test case for this, if my understanding of arrow functions is correct: > > ```` > function f(p) { > var arr = (x) => this; > if (p) return arr; > } > > for (var i = 0; i < 100000; ++i) > f(i % 2); > > if (f(true)() != f) > throw new Error("Bad sinking."); > ```` I've add Kind::ArrowFunction enum and isArrowFunction() function, also add following tests: *tests/stress/arrowfunction-lexical-this-activation-sink-osrexit.js * tests/stress/arrowfunction-lexical-this-activation-sink.js * tests/stress/arrowfunction-lexical-this-sinking-no-double-allocate.js * tests/stress/arrowfunction-lexical-this-sinking-osrexit.js * tests/stress/arrowfunction-lexical-this-sinking-put.js * tests/stress/arrowfunction-sinking-no-double-allocate.js * tests/stress/arrowfunction-sinking-osrexit.js * tests/stress/arrowfunction-sinking-put.js Could you please take a look if it make sense? >> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:991 >> + exactRead = FunctionActivationPLoc; > > This is wrong. Revert. Done >>> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:997 >>> + case LoadArrowFunctionThis: >> >> I don't think we need to worry about LoadArrowFunctionThis in the allocation >> sinking phase. This will just be loading from callee which should be materialized already at the top of a function. > > We'd need to worry about it if we inlined arrow functions, which this patch does not handle by the look of it. If the plan is to implement inlining of arrow functions later, this should at least be a FIXME. Otherwise, this should check for target->isArrowFunctionAllocation() instead; cf above. Done >> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:1998 >> + case NewArrowFunction: { > > I think we can combine this and the below case and make necessary changes/asserts > based on if op is NewArrowFunction or NewFunction Done >> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3074 >> + void compileNewFunction(NodeType nodeType) > > We don't need NodeType as a parameter here. m_node should have all the info you need. Done >> Source/JavaScriptCore/jit/JIT.h:642 >> + void emitNewFuncExprCommon(Instruction*, FunctionType); > > If you have Instruction* you don't need FunctionType. You can just look at the opcode to determine if it's an arrow function or not. Done >> Source/JavaScriptCore/jit/JITOperations.cpp:969 >> +EncodedJSValue operationNewArrowFunctionCommon(ExecState* exec, JSScope* scope, JSCell* functionExecutable, EncodedJSValue thisValue, bool isInvalidated) > > I don't think you need to declare this in the header. You can just make this a static function in this file. Done >> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1016 >> +inline JSFunction* getJSFunctionExpr(ExecState* exec, Instruction* pc, VM& vm) > > Let's get rid of this function now that it only has one caller. > It made more sense before you refactored how the arrow function object layout. Done >> Source/JavaScriptCore/runtime/JSArrowFunction.cpp:51 >> + executable->singletonFunction()->notifyWrite(vm, result, "Allocating a arrow function"); > > nit: "a" => "an" Done >> LayoutTests/js/script-tests/arrowfunction-lexical-this-1.js:1 >> +description('Tests for ES6 arrow function lexical bind of this'); > > I would combine "arrow function-lexical-this-1/2/3" into one file or give them more descriptive names. Done >> LayoutTests/js/script-tests/arrowfunction-lexical-this-2.js:9 >> +shouldBe("afObjLexBind2Instance.func() === afObjLexBind2Instance", "true"); > > Can we also try doing stuff like: > afObjLexBind2.call(V, ...) > where V might be many different things? > Basically, we should test "bound" this outside > a constructor and with different this values. > (Maybe a separate test). What kind different |this| should be, number, undefined, string?
Basile Clement
Comment 98 2015-08-06 13:54:06 PDT
Comment on attachment 258384 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258384&action=review The sinking-related stuff looks good to me. > Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:2022 > + ASSERT(locations.size() == isArrowFunction ? 3 : 2); This is parsed as "ASSERT((locations.size() == isArrowFunction) ? 3 : 2);" and will always be true. > Source/JavaScriptCore/dfg/DFGPromotedHeapLocation.h:50 > + FunctionBoundThisPLoc This is a nit, but I'd have called this ArrowFunctionBoundThisPLoc.
GSkachkov
Comment 99 2015-08-07 07:32:13 PDT
Created attachment 258495 [details] Patch Small fixes
GSkachkov
Comment 100 2015-08-07 09:27:26 PDT
Created attachment 258505 [details] Patch Small fixes after rebase
GSkachkov
Comment 101 2015-08-07 09:49:36 PDT
Comment on attachment 258384 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258384&action=review >> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:2022 >> + ASSERT(locations.size() == isArrowFunction ? 3 : 2); > > This is parsed as "ASSERT((locations.size() == isArrowFunction) ? 3 : 2);" and will always be true. Fixed >> Source/JavaScriptCore/dfg/DFGPromotedHeapLocation.h:50 >> + FunctionBoundThisPLoc > > This is a nit, but I'd have called this ArrowFunctionBoundThisPLoc. Done
GSkachkov
Comment 102 2015-08-07 09:49:39 PDT
Comment on attachment 258384 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258384&action=review >> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:2022 >> + ASSERT(locations.size() == isArrowFunction ? 3 : 2); > > This is parsed as "ASSERT((locations.size() == isArrowFunction) ? 3 : 2);" and will always be true. Fixed >> Source/JavaScriptCore/dfg/DFGPromotedHeapLocation.h:50 >> + FunctionBoundThisPLoc > > This is a nit, but I'd have called this ArrowFunctionBoundThisPLoc. Done
Saam Barati
Comment 103 2015-08-07 16:22:10 PDT
Comment on attachment 258505 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258505&action=review Looks pretty good to me. This seems really close to being finished. Some comments below. I still need to read more closely through the tests. > Source/JavaScriptCore/bytecode/BytecodeUseDef.h:119 > + case op_new_arrow_func_exp: This needs to also report the bound this operand as a use. > Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:83 > +UnlinkedFunctionExecutable::UnlinkedFunctionExecutable(VM* vm, Structure* structure, const SourceCode& source, RefPtr<SourceProvider>&& sourceOverride, FunctionBodyNode* node, UnlinkedFunctionKind kind, ConstructAbility constructAbility, VariableEnvironment& parentScopeTDZVariables, bool isArrowFunction) Lets make FunctionBodyNode know about whether or not it's an arrow function instead of having an extra parameter. (FunctionBodyNode is poorly named but it's job is to keep track of information like this). > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2301 > + if (m_codeBlock->isConstructor()) Is there a way to do this only for ES6 classes when we're in a constructor? Otherwise, this is probably unnecessary. I think isConstructor() will also be true in the case of "function F(){}; new F;" which isn't an ES6 constructor function. Might be worth looking into. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2312 > + nit: Whitespace. > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1743 > + if (JSValue base = forNode(node->child1()).m_value) { Will it ever be the case that this is true and the below dynamic case will return nullptr? > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4093 > + else I think we can do something even better here. Consider this function: "function F() { this.x = 20; var f = () => this.x; f() }" If we inline the call to "f" into "F", then I think the get(VirtualRegister(JSStack::Callee) will result in a callee node whose op is NewArrowFunction. If we see that callee is a NewArrowFunction, we can simply do: "set(VirtualRegister(currentInstruction[1].u.operand), callee->child2())". But, maybe this won't play nice with allocation sinking. It's worth looking into if doing this will report an escape with some test program that benefits from allocation sinking. If keeping LoadArrowFunctionThis enables allocation sinking on arrow functions then it's not worth doing what I suggested. > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:175 > +enum FunctionExpressionType { ArrowFunctionExpressionType, CommonFunctionExpressionType}; Is this used?
Saam Barati
Comment 104 2015-08-07 20:12:19 PDT
Comment on attachment 258505 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258505&action=review >> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4093 >> + else > > I think we can do something even better here. > Consider this function: > "function F() { this.x = 20; var f = () => this.x; f() }" > If we inline the call to "f" into "F", then I think the get(VirtualRegister(JSStack::Callee) will result in a callee > node whose op is NewArrowFunction. If we see that callee is a NewArrowFunction, we can simply > do: "set(VirtualRegister(currentInstruction[1].u.operand), callee->child2())". > > But, maybe this won't play nice with allocation sinking. It's worth looking into if doing this will report an escape with some test program that benefits from allocation sinking. > If keeping LoadArrowFunctionThis enables allocation sinking on arrow functions then it's not worth doing what I suggested. I'm wrong here. There are definitely programs where we won't be able to fold this. Basile convinced me of this. Consider: "function f() { var arr = (x)=>this; var o = {}; o.arr = arr; o.arr(); }
GSkachkov
Comment 105 2015-08-08 13:29:00 PDT
Created attachment 258572 [details] Patch New fixes
Saam Barati
Comment 106 2015-08-08 15:02:39 PDT
Comment on attachment 258572 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258572&action=review Patch looks good to me. I'll ask pizlo to take a look as well. > Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters:-4557 > -</Project> Is this intended? > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:729 > + return UnlinkedFunctionExecutable::create(m_vm, m_scopeNode->source(), body, isBuiltinFunction() ? UnlinkedBuiltinFunction : UnlinkedNormalFunction, constructAbility, variablesUnderTDZ, nullptr); Not: if nullptr is already the default parameter here you can revert this line. > Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:852 > + case NewArrowFunction: { Nit: This code is mostly the same as the NewFunction case. I think it's better to have these cases be the same and then have them differ where they need to based on op == NewArrowFunction. I know this is a bit pedantic it's better for future proofing code. > Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:1446 > + case Allocation::Kind::NewArrowFunction: { Ditto here. > Source/JavaScriptCore/jit/JITOperations.cpp:945 > +EncodedJSValue static operationNewArrowFunctionCommon(ExecState* exec, JSScope* scope, JSCell* functionExecutable, EncodedJSValue thisValue, bool isInvalidated) Nit: call this operationNewFunctionCommon > Source/JavaScriptCore/jit/JITOperations.cpp:947 > + UNUSED_PARAM(thisValue); This seems wrong. You do use "thisValue" > Source/JavaScriptCore/parser/Nodes.h:1880 > + class ArrowFuncExprNode : public FuncExprNode { I would either have his inherit from ExpressionNode or create a new parent class that both FuncExprNode and ArrowFuncExprNode inherit from. This inheritance chain seems a bit weird.
Saam Barati
Comment 107 2015-08-08 15:02:52 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=258572&action=review Patch looks good to me. I'll ask pizlo to take a look as well. > Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters:-4557 > -</Project> Is this intended? > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:729 > + return UnlinkedFunctionExecutable::create(m_vm, m_scopeNode->source(), body, isBuiltinFunction() ? UnlinkedBuiltinFunction : UnlinkedNormalFunction, constructAbility, variablesUnderTDZ, nullptr); Not: if nullptr is already the default parameter here you can revert this line. > Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:852 > + case NewArrowFunction: { Nit: This code is mostly the same as the NewFunction case. I think it's better to have these cases be the same and then have them differ where they need to based on op == NewArrowFunction. I know this is a bit pedantic it's better for future proofing code. > Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:1446 > + case Allocation::Kind::NewArrowFunction: { Ditto here. > Source/JavaScriptCore/jit/JITOperations.cpp:945 > +EncodedJSValue static operationNewArrowFunctionCommon(ExecState* exec, JSScope* scope, JSCell* functionExecutable, EncodedJSValue thisValue, bool isInvalidated) Nit: call this operationNewFunctionCommon > Source/JavaScriptCore/jit/JITOperations.cpp:947 > + UNUSED_PARAM(thisValue); This seems wrong. You do use "thisValue" > Source/JavaScriptCore/parser/Nodes.h:1880 > + class ArrowFuncExprNode : public FuncExprNode { I would either have his inherit from ExpressionNode or create a new parent class that both FuncExprNode and ArrowFuncExprNode inherit from. This inheritance chain seems a bit weird.
Saam Barati
Comment 108 2015-08-08 15:03:30 PDT
Comment on attachment 258572 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258572&action=review Patch looks good to me. I'll ask pizlo to take a look as well. > Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters:-4557 > -</Project> Is this intended? > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:729 > + return UnlinkedFunctionExecutable::create(m_vm, m_scopeNode->source(), body, isBuiltinFunction() ? UnlinkedBuiltinFunction : UnlinkedNormalFunction, constructAbility, variablesUnderTDZ, nullptr); Not: if nullptr is already the default parameter here you can revert this line. > Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:852 > + case NewArrowFunction: { Nit: This code is mostly the same as the NewFunction case. I think it's better to have these cases be the same and then have them differ where they need to based on op == NewArrowFunction. I know this is a bit pedantic it's better for future proofing code. > Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:1446 > + case Allocation::Kind::NewArrowFunction: { Ditto here. > Source/JavaScriptCore/jit/JITOperations.cpp:945 > +EncodedJSValue static operationNewArrowFunctionCommon(ExecState* exec, JSScope* scope, JSCell* functionExecutable, EncodedJSValue thisValue, bool isInvalidated) Nit: call this operationNewFunctionCommon > Source/JavaScriptCore/jit/JITOperations.cpp:947 > + UNUSED_PARAM(thisValue); This seems wrong. You do use "thisValue" > Source/JavaScriptCore/parser/Nodes.h:1880 > + class ArrowFuncExprNode : public FuncExprNode { I would either have his inherit from ExpressionNode or create a new parent class that both FuncExprNode and ArrowFuncExprNode inherit from. This inheritance chain seems a bit weird.
Saam Barati
Comment 109 2015-08-08 15:04:35 PDT
Oh, man. Bugzilla went crazy. Sorry for the duplicate posts.
GSkachkov
Comment 110 2015-08-09 09:22:22 PDT
Comment on attachment 258505 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258505&action=review Ohh, it seems that I forget to post what I did in last patch >> Source/JavaScriptCore/bytecode/BytecodeUseDef.h:119 >> + case op_new_arrow_func_exp: > > This needs to also report the bound this operand as a use. Do you mean that that op_new_arrow_func_exp should have name that report about bound |this|, for instance op_new_arrow_func_exp_with_bound_this? I'm not sure, because arrow function also has lexical |arguments| binding, and |super|, |new.target| as well, so after implementing some of this feature we need to change this name. >> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:83 >> +UnlinkedFunctionExecutable::UnlinkedFunctionExecutable(VM* vm, Structure* structure, const SourceCode& source, RefPtr<SourceProvider>&& sourceOverride, FunctionBodyNode* node, UnlinkedFunctionKind kind, ConstructAbility constructAbility, VariableEnvironment& parentScopeTDZVariables, bool isArrowFunction) > > Lets make FunctionBodyNode know about whether or not it's an arrow function instead of having an extra parameter. > (FunctionBodyNode is poorly named but it's job is to keep track of information like this). Done >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1743 >> + if (JSValue base = forNode(node->child1()).m_value) { > > Will it ever be the case that this is true and the below dynamic case will return nullptr? No I think no, only function or exception, so I've removed condition. >> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:175 >> +enum FunctionExpressionType { ArrowFunctionExpressionType, CommonFunctionExpressionType}; > > Is this used? Removed
GSkachkov
Comment 111 2015-08-09 09:53:51 PDT
Comment on attachment 258572 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258572&action=review >>> Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters:-4557 >>> -</Project> >> >> Is this intended? > > Is this intended? Reverted >>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:729 >>> + return UnlinkedFunctionExecutable::create(m_vm, m_scopeNode->source(), body, isBuiltinFunction() ? UnlinkedBuiltinFunction : UnlinkedNormalFunction, constructAbility, variablesUnderTDZ, nullptr); >> >> Not: if nullptr is already the default parameter here you can revert this line. > > Not: if nullptr is already the default parameter here you can revert this line. Done >>> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:852 >>> + case NewArrowFunction: { >> >> Nit: >> This code is mostly the same as the NewFunction case. >> I think it's better to have these cases be the same and then >> have them differ where they need to based on op == NewArrowFunction. >> I know this is a bit pedantic it's better for future proofing code. > > Nit: > This code is mostly the same as the NewFunction case. > I think it's better to have these cases be the same and then > have them differ where they need to based on op == NewArrowFunction. > I know this is a bit pedantic it's better for future proofing code. Refactored >>> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:1446 >>> + case Allocation::Kind::NewArrowFunction: { >> >> Ditto here. > > Ditto here. Done >>> Source/JavaScriptCore/jit/JITOperations.cpp:945 >>> +EncodedJSValue static operationNewArrowFunctionCommon(ExecState* exec, JSScope* scope, JSCell* functionExecutable, EncodedJSValue thisValue, bool isInvalidated) >> >> Nit: call this operationNewFunctionCommon > > Nit: call this operationNewFunctionCommon Renamed >>> Source/JavaScriptCore/jit/JITOperations.cpp:947 >>> + UNUSED_PARAM(thisValue); >> >> This seems wrong. You do use "thisValue" > > This seems wrong. You do use "thisValue" Fixed >>> Source/JavaScriptCore/parser/Nodes.h:1880 >>> + class ArrowFuncExprNode : public FuncExprNode { >> >> I would either have his inherit from ExpressionNode or create a new >> parent class that both FuncExprNode and ArrowFuncExprNode inherit from. >> This inheritance chain seems a bit weird. > > I would either have his inherit from ExpressionNode or create a new > parent class that both FuncExprNode and ArrowFuncExprNode inherit from. > This inheritance chain seems a bit weird. Done. Added new class BaseFuncExprNode
GSkachkov
Comment 112 2015-08-09 09:54:44 PDT
Created attachment 258594 [details] Patch New fixes
Saam Barati
Comment 113 2015-08-09 10:06:30 PDT
Comment on attachment 258505 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258505&action=review >>> Source/JavaScriptCore/bytecode/BytecodeUseDef.h:119 >>> + case op_new_arrow_func_exp: >> >> This needs to also report the bound this operand as a use. > > Do you mean that that op_new_arrow_func_exp should have name that report about bound |this|, for instance op_new_arrow_func_exp_with_bound_this? I'm not sure, because arrow function also has lexical |arguments| binding, and |super|, |new.target| as well, so after implementing some of this feature we need to change this name. No, I mean that the op_new_arrowfunction has two bytecode operands it uses in this patch. You need to report those as uses here. For example, you need to have it do: functor(codeBlock, instruction, opcodeID, instruction[2].u.operand); // Scope functor(codeBlock, instruction, opcodeID, instruction[4].u.operand); // Bound this.
GSkachkov
Comment 114 2015-08-09 11:04:31 PDT
Created attachment 258595 [details] Patch Smallest fix
GSkachkov
Comment 115 2015-08-09 11:05:17 PDT
Comment on attachment 258505 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258505&action=review >>>> Source/JavaScriptCore/bytecode/BytecodeUseDef.h:119 >>>> + case op_new_arrow_func_exp: >>> >>> This needs to also report the bound this operand as a use. >> >> Do you mean that that op_new_arrow_func_exp should have name that report about bound |this|, for instance op_new_arrow_func_exp_with_bound_this? I'm not sure, because arrow function also has lexical |arguments| binding, and |super|, |new.target| as well, so after implementing some of this feature we need to change this name. > > No, I mean that the op_new_arrowfunction has two bytecode operands it uses in this patch. > You need to report those as uses here. For example, you need to have it do: > functor(codeBlock, instruction, opcodeID, instruction[2].u.operand); // Scope > functor(codeBlock, instruction, opcodeID, instruction[4].u.operand); // Bound this. Ohh, I got it. Please take a look if it ok now.
GSkachkov
Comment 116 2015-08-09 14:50:16 PDT
(In reply to comment #113) > Comment on attachment 258505 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=258505&action=review > > >>> Source/JavaScriptCore/bytecode/BytecodeUseDef.h:119 > >>> + case op_new_arrow_func_exp: > >> > >> This needs to also report the bound this operand as a use. > > > > Do you mean that that op_new_arrow_func_exp should have name that report about bound |this|, for instance op_new_arrow_func_exp_with_bound_this? I'm not sure, because arrow function also has lexical |arguments| binding, and |super|, |new.target| as well, so after implementing some of this feature we need to change this name. > > No, I mean that the op_new_arrowfunction has two bytecode operands it uses > in this patch. > You need to report those as uses here. For example, you need to have it do: > functor(codeBlock, instruction, opcodeID, instruction[2].u.operand); > // Scope > functor(codeBlock, instruction, opcodeID, instruction[4].u.operand); > // Bound this. Thanks for the review and your time!
Saam Barati
Comment 117 2015-08-09 16:52:28 PDT
(In reply to comment #115) > Comment on attachment 258505 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=258505&action=review > > >>>> Source/JavaScriptCore/bytecode/BytecodeUseDef.h:119 > >>>> + case op_new_arrow_func_exp: > >>> > >>> This needs to also report the bound this operand as a use. > >> > >> Do you mean that that op_new_arrow_func_exp should have name that report about bound |this|, for instance op_new_arrow_func_exp_with_bound_this? I'm not sure, because arrow function also has lexical |arguments| binding, and |super|, |new.target| as well, so after implementing some of this feature we need to change this name. > > > > No, I mean that the op_new_arrowfunction has two bytecode operands it uses in this patch. > > You need to report those as uses here. For example, you need to have it do: > > functor(codeBlock, instruction, opcodeID, instruction[2].u.operand); // Scope > > functor(codeBlock, instruction, opcodeID, instruction[4].u.operand); // Bound this. > > Ohh, I got it. Please take a look if it ok now. 👍
GSkachkov
Comment 118 2015-08-12 07:52:39 PDT
Created attachment 258821 [details] Patch Upload patch after rebase
GSkachkov
Comment 119 2015-08-12 08:17:06 PDT
Created attachment 258822 [details] Patch Upload patch after rebase#2
GSkachkov
Comment 120 2015-08-12 08:56:11 PDT
Created attachment 258823 [details] Patch Upload patch after rebase#3
GSkachkov
Comment 121 2015-08-12 09:52:37 PDT
Comment on attachment 258595 [details] Patch Obsolete after changes in master
Filip Pizlo
Comment 122 2015-08-13 13:21:53 PDT
Comment on attachment 258823 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258823&action=review I'll cq+ if you make the two suggested changes. > Source/JavaScriptCore/dfg/DFGMayExit.cpp:111 > + case LoadArrowFunctionThis: This sort of can exit. It speculates that its child is a cell. > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3115 > + LBasicBlock slowPath = isArrowFunction > + ? FTL_NEW_BLOCK(m_out, ("NewArrowFunction slow path")) > + : FTL_NEW_BLOCK(m_out, ("NewFunction slow path")); Just call it NewFunction. No need for these branches just to get a different basic block name. > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3119 > - LBasicBlock slowPath = FTL_NEW_BLOCK(m_out, ("NewFunction slow path")); > - LBasicBlock continuation = FTL_NEW_BLOCK(m_out, ("NewFunction continuation")); > + LBasicBlock continuation = isArrowFunction > + ? FTL_NEW_BLOCK(m_out, ("NewArrowFunction continuation")) > + : FTL_NEW_BLOCK(m_out, ("NewFunction continuation")); Ditto. > Source/JavaScriptCore/runtime/JSArrowFunction.h:92 > +friend class LLIntOffsetsExtractor; Indent this line.
GSkachkov
Comment 123 2015-08-14 07:23:29 PDT
Created attachment 259000 [details] Patch Upload patch with fixes
GSkachkov
Comment 124 2015-08-14 09:14:56 PDT
Comment on attachment 258823 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258823&action=review >> Source/JavaScriptCore/dfg/DFGMayExit.cpp:111 >> + case LoadArrowFunctionThis: > > This sort of can exit. It speculates that its child is a cell. Removed from this list >> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3115 >> + : FTL_NEW_BLOCK(m_out, ("NewFunction slow path")); > > Just call it NewFunction. No need for these branches just to get a different basic block name. Done >> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3119 >> + : FTL_NEW_BLOCK(m_out, ("NewFunction continuation")); > > Ditto. Done >> Source/JavaScriptCore/runtime/JSArrowFunction.h:92 >> +friend class LLIntOffsetsExtractor; > > Indent this line. Done
GSkachkov
Comment 125 2015-08-14 09:26:31 PDT
Created attachment 259002 [details] Patch Upload patch with fixes
GSkachkov
Comment 126 2015-08-14 09:34:27 PDT
Created attachment 259003 [details] Patch Upload patch with windows build fix
GSkachkov
Comment 127 2015-08-14 12:43:08 PDT
Created attachment 259028 [details] Patch Try to fix win build
GSkachkov
Comment 128 2015-08-14 13:53:53 PDT
Created attachment 259036 [details] Patch Try to fix win build
GSkachkov
Comment 129 2015-08-14 14:47:28 PDT
Created attachment 259045 [details] Patch Upload patch with win fixes
GSkachkov
Comment 130 2015-08-14 18:32:58 PDT
Created attachment 259063 [details] Patch Fix build.Not for release
GSkachkov
Comment 131 2015-08-14 18:48:26 PDT
Created attachment 259064 [details] Patch Fix win build
GSkachkov
Comment 132 2015-08-14 18:57:32 PDT
Created attachment 259066 [details] Patch Fix win build
GSkachkov
Comment 133 2015-08-14 19:06:14 PDT
Created attachment 259071 [details] Patch Fix win build
GSkachkov
Comment 134 2015-08-15 13:52:54 PDT
Created attachment 259104 [details] Patch Fix win build
GSkachkov
Comment 135 2015-08-17 12:18:25 PDT
Created attachment 259170 [details] Patch Fix win build
Saam Barati
Comment 136 2015-08-17 13:22:40 PDT
Comment on attachment 259170 [details] Patch r=me
WebKit Commit Bot
Comment 137 2015-08-17 13:22:55 PDT
Comment on attachment 259170 [details] Patch Rejecting attachment 259170 [details] from review queue. sbarati@apple.com does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your reviewer rights.
Saam Barati
Comment 138 2015-08-17 13:23:38 PDT
(In reply to comment #137) > Comment on attachment 259170 [details] > Patch > > Rejecting attachment 259170 [details] from review queue. > > sbarati@apple.com does not have reviewer permissions according to > http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/ > contributors.json. > > - If you do not have reviewer rights please read > http://webkit.org/coding/contributing.html for instructions on how to use > bugzilla flags. > > - If you have reviewer rights please correct the error in > Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to > the file (no review needed). The commit-queue restarts itself every 2 > hours. After restart the commit-queue will correctly respect your reviewer > rights. Oops. I need to change my email.
WebKit Commit Bot
Comment 139 2015-08-17 13:23:40 PDT
Comment on attachment 259170 [details] Patch Rejecting attachment 259170 [details] from commit-queue. sbarati@apple.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.
GSkachkov
Comment 140 2015-08-17 14:08:38 PDT
Created attachment 259181 [details] Patch reupload
WebKit Commit Bot
Comment 141 2015-08-17 15:24:28 PDT
Comment on attachment 259181 [details] Patch Clearing flags on attachment: 259181 Committed r188545: <http://trac.webkit.org/changeset/188545>
WebKit Commit Bot
Comment 142 2015-08-17 15:24:45 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.