Description
GSkachkov
2015-05-13 10:48:17 PDT
Created attachment 253042 [details]
Patch
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.
Created attachment 253045 [details]
Patch
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.
Created attachment 253049 [details]
Patch
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 Created attachment 255641 [details]
Patch
Created attachment 255849 [details]
Patch
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.
Created attachment 255850 [details]
Patch
Created attachment 255856 [details]
Patch
Created attachment 255895 [details]
Patch
Created attachment 255905 [details]
Patch
Created attachment 255906 [details]
Patch
Created attachment 255909 [details]
Patch
Created attachment 255910 [details]
Patch
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 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 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 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
Created attachment 255914 [details]
Patch
Created attachment 255954 [details]
Patch
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 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 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 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
Created attachment 255995 [details]
Patch
Created attachment 256000 [details]
Patch
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 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 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 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
Created attachment 256003 [details]
Patch
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. Created attachment 256121 [details]
Patch
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 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. Created attachment 256240 [details]
Patch
Created attachment 256242 [details]
Patch
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 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? Created attachment 256327 [details]
Patch
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. Created attachment 256364 [details]
Patch
(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. Created attachment 256376 [details]
Patch
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 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. Created attachment 256828 [details]
Patch
Created attachment 256831 [details]
Patch
(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 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. Created attachment 257253 [details]
Patch
Current patch is not ready for landing. Known issue in using arrow function after FTL/DFG optimization. Created attachment 257272 [details]
Dump
Dump of working arrow function with error.
Created attachment 257275 [details]
Dump of the ArrowFunction execution
Created attachment 257297 [details]
Patch
Created attachment 257377 [details]
Patch
Created attachment 257437 [details]
Patch
Created attachment 257444 [details]
Patch
Created attachment 257453 [details]
Patch
(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. Created attachment 257559 [details]
Patch
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 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 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 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
Created attachment 257562 [details]
Patch
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? Created attachment 257585 [details]
Patch
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 Created attachment 257750 [details]
Patch
Created attachment 257753 [details]
Patch
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 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 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 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
Created attachment 257762 [details]
Patch
Fix tests after merge & small fixes from previous comment
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. Created attachment 257921 [details]
Patch
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 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 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 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
Created attachment 257930 [details]
Patch
Created attachment 257956 [details]
Patch
New patch with h allocation sink tests
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 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 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. (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. Created attachment 258372 [details]
Patch
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 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 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 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
Created attachment 258384 [details]
Patch
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 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. Created attachment 258495 [details]
Patch
Small fixes
Created attachment 258505 [details]
Patch
Small fixes after rebase
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 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 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 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(); } Created attachment 258572 [details]
Patch
New fixes
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. 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 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. Oh, man. Bugzilla went crazy. Sorry for the duplicate posts. 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 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 Created attachment 258594 [details]
Patch
New fixes
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. Created attachment 258595 [details]
Patch
Smallest fix
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. (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! (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. 👍 Created attachment 258821 [details]
Patch
Upload patch after rebase
Created attachment 258822 [details]
Patch
Upload patch after rebase#2
Created attachment 258823 [details]
Patch
Upload patch after rebase#3
Comment on attachment 258595 [details]
Patch
Obsolete after changes in master
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. Created attachment 259000 [details]
Patch
Upload patch with fixes
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 Created attachment 259002 [details]
Patch
Upload patch with fixes
Created attachment 259003 [details]
Patch
Upload patch with windows build fix
Created attachment 259028 [details]
Patch
Try to fix win build
Created attachment 259036 [details]
Patch
Try to fix win build
Created attachment 259045 [details]
Patch
Upload patch with win fixes
Created attachment 259063 [details]
Patch
Fix build.Not for release
Created attachment 259064 [details]
Patch
Fix win build
Created attachment 259066 [details]
Patch
Fix win build
Created attachment 259071 [details]
Patch
Fix win build
Created attachment 259104 [details]
Patch
Fix win build
Created attachment 259170 [details]
Patch
Fix win build
Comment on attachment 259170 [details]
Patch
r=me
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. (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 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. Created attachment 259181 [details]
Patch
reupload
Comment on attachment 259181 [details] Patch Clearing flags on attachment: 259181 Committed r188545: <http://trac.webkit.org/changeset/188545> All reviewed patches have been landed. Closing bug. |