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

[bugzilla-daemon at webkit.org]

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

--- Comment #97 from GSkachkov <gskachkov at gmail.com> ---
Comment on attachment 257956
  --> https://bugs.webkit.org/attachment.cgi?id=257956
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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150806/a47391f9/attachment.html>