<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [ES6] Implement ES6 arrow function syntax. Arrow function specific features. Lexical bind of this"
href="https://bugs.webkit.org/show_bug.cgi?id=144956#c97">Comment # 97</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [ES6] Implement ES6 arrow function syntax. Arrow function specific features. Lexical bind of this"
href="https://bugs.webkit.org/show_bug.cgi?id=144956">bug 144956</a>
from <span class="vcard"><a class="email" href="mailto:gskachkov@gmail.com" title="GSkachkov <gskachkov@gmail.com>"> <span class="fn">GSkachkov</span></a>
</span></b>
<pre>Comment on <span class="bz_obsolete"><a href="attachment.cgi?id=257956&action=diff" name="attach_257956" title="Patch">attachment 257956</a> <a href="attachment.cgi?id=257956&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=257956&action=review">https://bugs.webkit.org/attachment.cgi?id=257956&action=review</a>
<span class="quote">>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:513
>> + RefPtr<RegisterID> arrowFunctionBoundThis = emitLoadArrowFunctionThis(addVar());
>
> this should be a temporary register, not an addVar().</span >
In new patch I've used directly m_thisRegister, without any temp and var, Is it Ok?
<span class="quote">>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2868
>> + arrowFunctionThis->ref();
>
> don't ref this. this is an anti-pattern.</span >
Done
<span class="quote">>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2962
>> +// ------------------------------ ArrowFuncExprNode ---------------------------------
>
> Nit: new line after comment to follow style of the file.</span >
Done
<span class="quote">>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1744
>> + if (JSArrowFunction* function = jsDynamicCast<JSArrowFunction*>(base)) {
>
> Will this every be nullptr when base is truthy?</span >
I hope so, do I need to change to some another condition?
<span class="quote">>> 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.</span >
Done, it is jsConstant now
<span class="quote">>> 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())"?</span >
Fixed
<span class="quote">>> 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 ...)"</span >
Done
<span class="quote">>> 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.");
> ````</span >
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?
<span class="quote">>> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:991
>> + exactRead = FunctionActivationPLoc;
>
> This is wrong. Revert.</span >
Done
<span class="quote">>>> 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.</span >
Done
<span class="quote">>> 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</span >
Done
<span class="quote">>> 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.</span >
Done
<span class="quote">>> 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.</span >
Done
<span class="quote">>> 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.</span >
Done
<span class="quote">>> 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.</span >
Done
<span class="quote">>> Source/JavaScriptCore/runtime/JSArrowFunction.cpp:51
>> + executable->singletonFunction()->notifyWrite(vm, result, "Allocating a arrow function");
>
> nit: "a" => "an"</span >
Done
<span class="quote">>> 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.</span >
Done
<span class="quote">>> 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).</span >
What kind different |this| should be, number, undefined, string?</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>