WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(88.65 KB, patch)
2015-05-13 12:22 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(88.59 KB, patch)
2015-05-13 12:48 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(88.68 KB, patch)
2015-06-26 09:30 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(91.51 KB, patch)
2015-06-30 14:14 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(91.41 KB, patch)
2015-06-30 14:20 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(91.46 KB, patch)
2015-06-30 14:54 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(91.50 KB, patch)
2015-06-30 21:17 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(91.90 KB, patch)
2015-07-01 00:11 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(92.64 KB, patch)
2015-07-01 00:38 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(104.84 KB, patch)
2015-07-01 01:13 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(104.85 KB, patch)
2015-07-01 01:43 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
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
Details
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
Details
Patch
(101.67 KB, patch)
2015-07-01 03:30 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(106.17 KB, patch)
2015-07-01 14:39 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
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
Details
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
Details
Patch
(106.17 KB, patch)
2015-07-02 00:06 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(106.55 KB, patch)
2015-07-02 01:30 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
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
Details
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
Details
Patch
(106.37 KB, patch)
2015-07-02 03:09 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(103.45 KB, patch)
2015-07-03 14:27 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(109.86 KB, patch)
2015-07-06 14:28 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(109.98 KB, patch)
2015-07-06 14:36 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(111.97 KB, patch)
2015-07-07 15:55 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(108.97 KB, patch)
2015-07-08 00:59 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(109.24 KB, patch)
2015-07-08 08:32 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(109.00 KB, patch)
2015-07-15 00:45 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(108.99 KB, patch)
2015-07-15 03:48 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(150.64 KB, patch)
2015-07-22 01:36 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Dump
(21.46 KB, application/octet-stream)
2015-07-22 08:35 PDT
,
GSkachkov
no flags
Details
Dump of the ArrowFunction execution
(21.46 KB, application/octet-stream)
2015-07-22 08:43 PDT
,
GSkachkov
no flags
Details
Patch
(163.27 KB, patch)
2015-07-22 14:55 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(149.11 KB, patch)
2015-07-23 14:10 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(148.00 KB, patch)
2015-07-23 23:43 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(148.00 KB, patch)
2015-07-24 01:00 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(148.52 KB, patch)
2015-07-24 08:10 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(68.48 KB, patch)
2015-07-27 05:34 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
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
Details
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
Details
Patch
(68.52 KB, patch)
2015-07-27 07:56 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(66.84 KB, patch)
2015-07-27 13:01 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(2.15 MB, patch)
2015-07-29 09:08 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(148.17 KB, patch)
2015-07-29 09:21 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
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
Details
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
Details
Patch
(147.50 KB, patch)
2015-07-29 12:43 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(156.39 KB, patch)
2015-07-31 08:17 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
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
Details
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
Details
Patch
(156.61 KB, patch)
2015-07-31 10:39 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(171.88 KB, patch)
2015-07-31 13:42 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(164.85 KB, patch)
2015-08-06 10:27 PDT
,
GSkachkov
buildbot
: commit-queue-
Details
Formatted Diff
Diff
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
Details
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
Details
Patch
(165.00 KB, patch)
2015-08-06 12:26 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(165.03 KB, patch)
2015-08-07 07:32 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(165.07 KB, patch)
2015-08-07 09:27 PDT
,
GSkachkov
saam
: review-
Details
Formatted Diff
Diff
Patch
(170.61 KB, patch)
2015-08-08 13:29 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(171.27 KB, patch)
2015-08-09 09:54 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(171.41 KB, patch)
2015-08-09 11:04 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(172.67 KB, patch)
2015-08-12 07:52 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(173.51 KB, patch)
2015-08-12 08:17 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(172.60 KB, patch)
2015-08-12 08:56 PDT
,
GSkachkov
fpizlo
: review+
fpizlo
: commit-queue-
Details
Formatted Diff
Diff
Patch
(171.72 KB, patch)
2015-08-14 07:23 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(172.17 KB, patch)
2015-08-14 09:26 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(172.05 KB, patch)
2015-08-14 09:34 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(172.04 KB, patch)
2015-08-14 12:43 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(172.01 KB, patch)
2015-08-14 13:53 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(172.52 KB, patch)
2015-08-14 14:47 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(16.64 KB, patch)
2015-08-14 18:32 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(23.49 KB, patch)
2015-08-14 18:48 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(24.82 KB, patch)
2015-08-14 18:57 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(27.88 KB, patch)
2015-08-14 19:06 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(171.72 KB, patch)
2015-08-15 13:52 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(172.61 KB, patch)
2015-08-17 12:18 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Patch
(172.61 KB, patch)
2015-08-17 14:08 PDT
,
GSkachkov
no flags
Details
Formatted Diff
Diff
Show Obsolete
(78)
View All
Add attachment
proposed patch, testcase, etc.
GSkachkov
Comment 1
2015-05-13 12:14:58 PDT
Created
attachment 253042
[details]
Patch
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
Created
attachment 253045
[details]
Patch
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
Created
attachment 253049
[details]
Patch
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
Created
attachment 255641
[details]
Patch
GSkachkov
Comment 8
2015-06-30 14:14:35 PDT
Created
attachment 255849
[details]
Patch
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
Created
attachment 255850
[details]
Patch
GSkachkov
Comment 11
2015-06-30 14:54:10 PDT
Created
attachment 255856
[details]
Patch
GSkachkov
Comment 12
2015-06-30 21:17:13 PDT
Created
attachment 255895
[details]
Patch
GSkachkov
Comment 13
2015-07-01 00:11:04 PDT
Created
attachment 255905
[details]
Patch
GSkachkov
Comment 14
2015-07-01 00:38:33 PDT
Created
attachment 255906
[details]
Patch
GSkachkov
Comment 15
2015-07-01 01:13:19 PDT
Created
attachment 255909
[details]
Patch
GSkachkov
Comment 16
2015-07-01 01:43:10 PDT
Created
attachment 255910
[details]
Patch
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
Created
attachment 255914
[details]
Patch
GSkachkov
Comment 22
2015-07-01 14:39:24 PDT
Created
attachment 255954
[details]
Patch
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
Created
attachment 255995
[details]
Patch
GSkachkov
Comment 28
2015-07-02 01:30:26 PDT
Created
attachment 256000
[details]
Patch
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
Created
attachment 256003
[details]
Patch
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
Created
attachment 256121
[details]
Patch
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
Created
attachment 256240
[details]
Patch
GSkachkov
Comment 39
2015-07-06 14:36:44 PDT
Created
attachment 256242
[details]
Patch
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
Created
attachment 256327
[details]
Patch
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
Created
attachment 256364
[details]
Patch
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
Created
attachment 256376
[details]
Patch
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
Created
attachment 256828
[details]
Patch
GSkachkov
Comment 50
2015-07-15 03:48:27 PDT
Created
attachment 256831
[details]
Patch
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
Created
attachment 257253
[details]
Patch
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
Created
attachment 257297
[details]
Patch
GSkachkov
Comment 58
2015-07-23 14:10:41 PDT
Created
attachment 257377
[details]
Patch
GSkachkov
Comment 59
2015-07-23 23:43:54 PDT
Created
attachment 257437
[details]
Patch
GSkachkov
Comment 60
2015-07-24 01:00:14 PDT
Created
attachment 257444
[details]
Patch
GSkachkov
Comment 61
2015-07-24 08:10:26 PDT
Created
attachment 257453
[details]
Patch
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
Created
attachment 257559
[details]
Patch
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
Created
attachment 257562
[details]
Patch
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
Created
attachment 257585
[details]
Patch
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
Created
attachment 257750
[details]
Patch
GSkachkov
Comment 73
2015-07-29 09:21:07 PDT
Created
attachment 257753
[details]
Patch
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
Created
attachment 257921
[details]
Patch
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
Created
attachment 257930
[details]
Patch
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
Created
attachment 258372
[details]
Patch
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
Created
attachment 258384
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug