Bug 144956 – [ES6] Implement ES6 arrow function syntax. Arrow function specific features. Lexical bind of this

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[GSkachkov]

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

[GSkachkov]

Created attachment 253042 [details]
Patch

[WebKit Commit Bot]

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]

Created attachment 253045 [details]
Patch

[WebKit Commit Bot]

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]

Created attachment 253049 [details]
Patch

[GSkachkov]

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]

Created attachment 255641 [details]
Patch

[GSkachkov]

Created attachment 255849 [details]
Patch

[WebKit Commit Bot]

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]

Created attachment 255850 [details]
Patch

[GSkachkov]

Created attachment 255856 [details]
Patch

[GSkachkov]

Created attachment 255895 [details]
Patch

[GSkachkov]

Created attachment 255905 [details]
Patch

[GSkachkov]

Created attachment 255906 [details]
Patch

[GSkachkov]

Created attachment 255909 [details]
Patch

[GSkachkov]

Created attachment 255910 [details]
Patch

[Build Bot]

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]

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 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]

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]

Created attachment 255914 [details]
Patch

[GSkachkov]

Created attachment 255954 [details]
Patch

[Build Bot]

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]

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 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]

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]

Created attachment 255995 [details]
Patch

[GSkachkov]

Created attachment 256000 [details]
Patch

[Build Bot]

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]

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 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]

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]

Created attachment 256003 [details]
Patch

[Filip Pizlo]

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]

Created attachment 256121 [details]
Patch

[GSkachkov]

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 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]

Created attachment 256240 [details]
Patch

[GSkachkov]

Created attachment 256242 [details]
Patch

[GSkachkov]

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 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]

Created attachment 256327 [details]
Patch

[GSkachkov]

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]

Created attachment 256364 [details]
Patch

[GSkachkov]

(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]

Created attachment 256376 [details]
Patch

[Saam Barati]

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 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]

Created attachment 256828 [details]
Patch

[GSkachkov]

Created attachment 256831 [details]
Patch

[Saam Barati]

(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 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]

Created attachment 257253 [details]
Patch

[GSkachkov]

Current patch is not ready for landing.
Known issue in using arrow function after FTL/DFG optimization.

[GSkachkov]

Created attachment 257272 [details]
Dump

Dump of working arrow function with error.

[GSkachkov]

Created attachment 257275 [details]
Dump of the ArrowFunction execution

[GSkachkov]

Created attachment 257297 [details]
Patch

[GSkachkov]

Created attachment 257377 [details]
Patch

[GSkachkov]

Created attachment 257437 [details]
Patch

[GSkachkov]

Created attachment 257444 [details]
Patch

[GSkachkov]

Created attachment 257453 [details]
Patch

[GSkachkov]

(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]

Created attachment 257559 [details]
Patch

[Build Bot]

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]

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 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]

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]

Created attachment 257562 [details]
Patch

[Saam Barati]

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]

Created attachment 257585 [details]
Patch

[GSkachkov]

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]

Created attachment 257750 [details]
Patch

[GSkachkov]

Created attachment 257753 [details]
Patch

[Build Bot]

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]

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 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]

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]

Created attachment 257762 [details]
Patch

Fix tests after merge & small fixes from previous comment

[Saam Barati]

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]

Created attachment 257921 [details]
Patch

[Build Bot]

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]

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 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]

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]

Created attachment 257930 [details]
Patch

[GSkachkov]

Created attachment 257956 [details]
Patch

New patch with h allocation sink tests

[GSkachkov]

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 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 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]

(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]

Created attachment 258372 [details]
Patch

[Build Bot]

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]

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 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]

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]

Created attachment 258384 [details]
Patch

[GSkachkov]

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 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]

Created attachment 258495 [details]
Patch

Small fixes

[GSkachkov]

Created attachment 258505 [details]
Patch

Small fixes after rebase

[GSkachkov]

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 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 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 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]

Created attachment 258572 [details]
Patch

New fixes

[Saam Barati]

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]

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 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]

Oh, man. Bugzilla went crazy.  
Sorry for the duplicate posts.

[GSkachkov]

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 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]

Created attachment 258594 [details]
Patch

New fixes

[Saam Barati]

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]

Created attachment 258595 [details]
Patch

Smallest fix

[GSkachkov]

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]

(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]

(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]

Created attachment 258821 [details]
Patch

Upload patch after rebase

[GSkachkov]

Created attachment 258822 [details]
Patch

Upload patch after rebase#2

[GSkachkov]

Created attachment 258823 [details]
Patch

Upload patch after rebase#3

[GSkachkov]

Comment on attachment 258595 [details]
Patch

Obsolete after changes in master

[Filip Pizlo]

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]

Created attachment 259000 [details]
Patch

Upload patch with fixes

[GSkachkov]

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]

Created attachment 259002 [details]
Patch

Upload patch with fixes

[GSkachkov]

Created attachment 259003 [details]
Patch

Upload patch with windows build fix

[GSkachkov]

Created attachment 259028 [details]
Patch

Try to fix win build

[GSkachkov]

Created attachment 259036 [details]
Patch

Try to fix win build

[GSkachkov]

Created attachment 259045 [details]
Patch

Upload patch with win fixes

[GSkachkov]

Created attachment 259063 [details]
Patch

Fix build.Not for release

[GSkachkov]

Created attachment 259064 [details]
Patch

Fix win build

[GSkachkov]

Created attachment 259066 [details]
Patch

Fix win build

[GSkachkov]

Created attachment 259071 [details]
Patch

Fix win build

[GSkachkov]

Created attachment 259104 [details]
Patch

Fix win build

[GSkachkov]

Created attachment 259170 [details]
Patch

Fix win build

[Saam Barati]

Comment on attachment 259170 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 259170 [details]
Patch

Rejecting attachment 259170 [details] from review queue.

[e-mail] 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]

(In reply to comment #137)
> Comment on attachment 259170 [details]
> Patch
> 
> Rejecting attachment 259170 [details] from review queue.
> 
> [e-mail] 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 on attachment 259170 [details]
Patch

Rejecting attachment 259170 [details] from commit-queue.

[e-mail] 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]

Created attachment 259181 [details]
Patch

reupload

[WebKit Commit Bot]

Comment on attachment 259181 [details]
Patch

Clearing flags on attachment: 259181

Committed r188545: <http://trac.webkit.org/changeset/188545>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.