Bug 144956

Summary: [ES6] Implement ES6 arrow function syntax. Arrow function specific features. Lexical bind of this
Product: WebKit Reporter: GSkachkov <gskachkov>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: basile_clement, bburg, buildbot, commit-queue, fpizlo, ggaren, rniwa, saam, ysuzuki
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 144955, 148376, 149338    
Bug Blocks: 140855, 145132, 146507    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews101 for mac-mavericks
none
Archive of layout-test-results from ews107 for mac-mavericks-wk2
none
Patch
none
Patch
none
Archive of layout-test-results from ews101 for mac-mavericks
none
Archive of layout-test-results from ews105 for mac-mavericks-wk2
none
Patch
none
Patch
none
Archive of layout-test-results from ews106 for mac-mavericks-wk2
none
Archive of layout-test-results from ews101 for mac-mavericks
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Dump
none
Dump of the ArrowFunction execution
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews104 for mac-mavericks-wk2
none
Archive of layout-test-results from ews101 for mac-mavericks
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews102 for mac-mavericks
none
Archive of layout-test-results from ews107 for mac-mavericks-wk2
none
Patch
none
Patch
none
Archive of layout-test-results from ews103 for mac-mavericks
none
Archive of layout-test-results from ews105 for mac-mavericks-wk2
none
Patch
none
Patch
none
Patch
buildbot: commit-queue-
Archive of layout-test-results from ews102 for mac-mavericks
none
Archive of layout-test-results from ews105 for mac-mavericks-wk2
none
Patch
none
Patch
none
Patch
saam: review-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
fpizlo: review+, fpizlo: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description GSkachkov 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
Comment 1 GSkachkov 2015-05-13 12:14:58 PDT
Created attachment 253042 [details]
Patch
Comment 2 WebKit Commit Bot 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.
Comment 3 GSkachkov 2015-05-13 12:22:32 PDT
Created attachment 253045 [details]
Patch
Comment 4 WebKit Commit Bot 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.
Comment 5 GSkachkov 2015-05-13 12:48:08 PDT
Created attachment 253049 [details]
Patch
Comment 6 GSkachkov 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
Comment 7 GSkachkov 2015-06-26 09:30:11 PDT
Created attachment 255641 [details]
Patch
Comment 8 GSkachkov 2015-06-30 14:14:35 PDT
Created attachment 255849 [details]
Patch
Comment 9 WebKit Commit Bot 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.
Comment 10 GSkachkov 2015-06-30 14:20:54 PDT
Created attachment 255850 [details]
Patch
Comment 11 GSkachkov 2015-06-30 14:54:10 PDT
Created attachment 255856 [details]
Patch
Comment 12 GSkachkov 2015-06-30 21:17:13 PDT
Created attachment 255895 [details]
Patch
Comment 13 GSkachkov 2015-07-01 00:11:04 PDT
Created attachment 255905 [details]
Patch
Comment 14 GSkachkov 2015-07-01 00:38:33 PDT
Created attachment 255906 [details]
Patch
Comment 15 GSkachkov 2015-07-01 01:13:19 PDT
Created attachment 255909 [details]
Patch
Comment 16 GSkachkov 2015-07-01 01:43:10 PDT
Created attachment 255910 [details]
Patch
Comment 17 Build Bot 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
Comment 18 Build Bot 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
Comment 19 Build Bot 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
Comment 20 Build Bot 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
Comment 21 GSkachkov 2015-07-01 03:30:15 PDT
Created attachment 255914 [details]
Patch
Comment 22 GSkachkov 2015-07-01 14:39:24 PDT
Created attachment 255954 [details]
Patch
Comment 23 Build Bot 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
Comment 24 Build Bot 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
Comment 25 Build Bot 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
Comment 26 Build Bot 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
Comment 27 GSkachkov 2015-07-02 00:06:07 PDT
Created attachment 255995 [details]
Patch
Comment 28 GSkachkov 2015-07-02 01:30:26 PDT
Created attachment 256000 [details]
Patch
Comment 29 Build Bot 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
Comment 30 Build Bot 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
Comment 31 Build Bot 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
Comment 32 Build Bot 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
Comment 33 GSkachkov 2015-07-02 03:09:02 PDT
Created attachment 256003 [details]
Patch
Comment 34 Filip Pizlo 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.
Comment 35 GSkachkov 2015-07-03 14:27:38 PDT
Created attachment 256121 [details]
Patch
Comment 36 GSkachkov 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
Comment 37 Saam Barati 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.
Comment 38 GSkachkov 2015-07-06 14:28:51 PDT
Created attachment 256240 [details]
Patch
Comment 39 GSkachkov 2015-07-06 14:36:44 PDT
Created attachment 256242 [details]
Patch
Comment 40 GSkachkov 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
Comment 41 Saam Barati 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?
Comment 42 GSkachkov 2015-07-07 15:55:46 PDT
Created attachment 256327 [details]
Patch
Comment 43 GSkachkov 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.
Comment 44 GSkachkov 2015-07-08 00:59:35 PDT
Created attachment 256364 [details]
Patch
Comment 45 GSkachkov 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.
Comment 46 GSkachkov 2015-07-08 08:32:51 PDT
Created attachment 256376 [details]
Patch
Comment 47 Saam Barati 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.
Comment 48 GSkachkov 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.
Comment 49 GSkachkov 2015-07-15 00:45:02 PDT
Created attachment 256828 [details]
Patch
Comment 50 GSkachkov 2015-07-15 03:48:27 PDT
Created attachment 256831 [details]
Patch
Comment 51 Saam Barati 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.
Comment 52 Filip Pizlo 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.
Comment 53 GSkachkov 2015-07-22 01:36:15 PDT
Created attachment 257253 [details]
Patch
Comment 54 GSkachkov 2015-07-22 03:06:02 PDT
Current patch is not ready for landing.
Known issue in using arrow function after FTL/DFG optimization.
Comment 55 GSkachkov 2015-07-22 08:35:22 PDT
Created attachment 257272 [details]
Dump

Dump of working arrow function with error.
Comment 56 GSkachkov 2015-07-22 08:43:33 PDT
Created attachment 257275 [details]
Dump of the ArrowFunction execution
Comment 57 GSkachkov 2015-07-22 14:55:46 PDT
Created attachment 257297 [details]
Patch
Comment 58 GSkachkov 2015-07-23 14:10:41 PDT
Created attachment 257377 [details]
Patch
Comment 59 GSkachkov 2015-07-23 23:43:54 PDT
Created attachment 257437 [details]
Patch
Comment 60 GSkachkov 2015-07-24 01:00:14 PDT
Created attachment 257444 [details]
Patch
Comment 61 GSkachkov 2015-07-24 08:10:26 PDT
Created attachment 257453 [details]
Patch
Comment 62 GSkachkov 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.
Comment 63 GSkachkov 2015-07-27 05:34:39 PDT
Created attachment 257559 [details]
Patch
Comment 64 Build Bot 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
Comment 65 Build Bot 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
Comment 66 Build Bot 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
Comment 67 Build Bot 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
Comment 68 GSkachkov 2015-07-27 07:56:33 PDT
Created attachment 257562 [details]
Patch
Comment 69 Saam Barati 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?
Comment 70 GSkachkov 2015-07-27 13:01:02 PDT
Created attachment 257585 [details]
Patch
Comment 71 GSkachkov 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
Comment 72 GSkachkov 2015-07-29 09:08:59 PDT
Created attachment 257750 [details]
Patch
Comment 73 GSkachkov 2015-07-29 09:21:07 PDT
Created attachment 257753 [details]
Patch
Comment 74 Build Bot 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
Comment 75 Build Bot 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
Comment 76 Build Bot 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
Comment 77 Build Bot 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
Comment 78 GSkachkov 2015-07-29 12:43:41 PDT
Created attachment 257762 [details]
Patch

Fix tests after merge & small fixes from previous comment
Comment 79 Saam Barati 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.
Comment 80 GSkachkov 2015-07-31 08:17:50 PDT
Created attachment 257921 [details]
Patch
Comment 81 Build Bot 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
Comment 82 Build Bot 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
Comment 83 Build Bot 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
Comment 84 Build Bot 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
Comment 85 GSkachkov 2015-07-31 10:39:37 PDT
Created attachment 257930 [details]
Patch
Comment 86 GSkachkov 2015-07-31 13:42:06 PDT
Created attachment 257956 [details]
Patch

New patch with h allocation sink tests
Comment 87 GSkachkov 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
Comment 88 Saam Barati 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).
Comment 89 Basile Clement 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.
Comment 90 Basile Clement 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.
Comment 91 GSkachkov 2015-08-06 10:27:03 PDT
Created attachment 258372 [details]
Patch
Comment 92 Build Bot 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
Comment 93 Build Bot 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
Comment 94 Build Bot 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
Comment 95 Build Bot 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
Comment 96 GSkachkov 2015-08-06 12:26:27 PDT
Created attachment 258384 [details]
Patch
Comment 97 GSkachkov 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?
Comment 98 Basile Clement 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.
Comment 99 GSkachkov 2015-08-07 07:32:13 PDT
Created attachment 258495 [details]
Patch

Small fixes
Comment 100 GSkachkov 2015-08-07 09:27:26 PDT
Created attachment 258505 [details]
Patch

Small fixes after rebase
Comment 101 GSkachkov 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
Comment 102 GSkachkov 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
Comment 103 Saam Barati 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?
Comment 104 Saam Barati 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(); }
Comment 105 GSkachkov 2015-08-08 13:29:00 PDT
Created attachment 258572 [details]
Patch

New fixes
Comment 106 Saam Barati 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.
Comment 107 Saam Barati 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.
Comment 108 Saam Barati 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.
Comment 109 Saam Barati 2015-08-08 15:04:35 PDT
Oh, man. Bugzilla went crazy.  
Sorry for the duplicate posts.
Comment 110 GSkachkov 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
Comment 111 GSkachkov 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
Comment 112 GSkachkov 2015-08-09 09:54:44 PDT
Created attachment 258594 [details]
Patch

New fixes
Comment 113 Saam Barati 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.
Comment 114 GSkachkov 2015-08-09 11:04:31 PDT
Created attachment 258595 [details]
Patch

Smallest fix
Comment 115 GSkachkov 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.
Comment 116 GSkachkov 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!
Comment 117 Saam Barati 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.
👍
Comment 118 GSkachkov 2015-08-12 07:52:39 PDT
Created attachment 258821 [details]
Patch

Upload patch after rebase
Comment 119 GSkachkov 2015-08-12 08:17:06 PDT
Created attachment 258822 [details]
Patch

Upload patch after rebase#2
Comment 120 GSkachkov 2015-08-12 08:56:11 PDT
Created attachment 258823 [details]
Patch

Upload patch after rebase#3
Comment 121 GSkachkov 2015-08-12 09:52:37 PDT
Comment on attachment 258595 [details]
Patch

Obsolete after changes in master
Comment 122 Filip Pizlo 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.
Comment 123 GSkachkov 2015-08-14 07:23:29 PDT
Created attachment 259000 [details]
Patch

Upload patch with fixes
Comment 124 GSkachkov 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
Comment 125 GSkachkov 2015-08-14 09:26:31 PDT
Created attachment 259002 [details]
Patch

Upload patch with fixes
Comment 126 GSkachkov 2015-08-14 09:34:27 PDT
Created attachment 259003 [details]
Patch

Upload patch with windows build fix
Comment 127 GSkachkov 2015-08-14 12:43:08 PDT
Created attachment 259028 [details]
Patch

Try to fix win build
Comment 128 GSkachkov 2015-08-14 13:53:53 PDT
Created attachment 259036 [details]
Patch

Try to fix win build
Comment 129 GSkachkov 2015-08-14 14:47:28 PDT
Created attachment 259045 [details]
Patch

Upload patch with win fixes
Comment 130 GSkachkov 2015-08-14 18:32:58 PDT
Created attachment 259063 [details]
Patch

Fix build.Not for release
Comment 131 GSkachkov 2015-08-14 18:48:26 PDT
Created attachment 259064 [details]
Patch

Fix win build
Comment 132 GSkachkov 2015-08-14 18:57:32 PDT
Created attachment 259066 [details]
Patch

Fix win build
Comment 133 GSkachkov 2015-08-14 19:06:14 PDT
Created attachment 259071 [details]
Patch

Fix win build
Comment 134 GSkachkov 2015-08-15 13:52:54 PDT
Created attachment 259104 [details]
Patch

Fix win build
Comment 135 GSkachkov 2015-08-17 12:18:25 PDT
Created attachment 259170 [details]
Patch

Fix win build
Comment 136 Saam Barati 2015-08-17 13:22:40 PDT
Comment on attachment 259170 [details]
Patch

r=me
Comment 137 WebKit Commit Bot 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.
Comment 138 Saam Barati 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.
Comment 139 WebKit Commit Bot 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.
Comment 140 GSkachkov 2015-08-17 14:08:38 PDT
Created attachment 259181 [details]
Patch

reupload
Comment 141 WebKit Commit Bot 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>
Comment 142 WebKit Commit Bot 2015-08-17 15:24:45 PDT
All reviewed patches have been landed.  Closing bug.