...
Let's consider the case like this. `${array[0]} and ${array[1]}` If we think ToString(array element) is side effectful, we need to lookup butterfly twice for that!
Created attachment 304268 [details] Patch
Created attachment 304270 [details] Patch
Comment on attachment 304270 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=304270&action=review r=me with comments > Source/JavaScriptCore/ChangeLog:24 > + template-string-array 12.6284+-0.2766 ^ 9.4998+-0.2295 ^ definitely 1.3293x faster > + > + And SixSpeed template_string.es6 shows 16.68x performance improvement due to LICM onto this non-side-effectful ToString(). > + > + baseline patched > + > + template_string.es6 3229.7343+-40.5705 ^ 193.6077+-36.3349 ^ definitely 16.6818x faster nice > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:2239 > + // While Symbol can throw an error, it is excluded since it is a Cell! Symbol can or can't throw an error? Why would it be able to? > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7940 > + callOperation(operationToString, resultGPR, extractResult(op1Regs)); I understand what extractResult does, but it's a very weird function name to be calling for an operand to a function. Perhaps we can come up with a better name. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7943 > + callOperation(operationCallStringConstructor, resultGPR, extractResult(op1Regs)); ditto. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4942 > + else if (m_node->child1().useKind() == NotCellUse) > + value = lowNotCell(m_node->child1()); This is very weird. You're emitting lowNotCell, but then branching on isCell. Please add a new place to emit code for this. Or just assign isCellPredicate to false. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:12240 > + LValue lowNotCell(Edge edge) > + { > + LValue result = lowJSValue(edge, ManualOperandSpeculation); > + speculateNotCell(edge, result); > + return result; > + } Style nit: I think this makes more sense if you did this the other way around: Implement speculateNotCell in terms of lowNotCell. That way, speculateNotCell would be implemented like: void speculateNotCell(Edge edge) { lowNotCell(edge); } (This is how lowCell and speculateCell work.)
Comment on attachment 304270 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=304270&action=review Thanks for your review! >> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:2239 >> + // While Symbol can throw an error, it is excluded since it is a Cell! > > Symbol can or can't throw an error? Why would it be able to? ToString(Symbol) throws an error. So, in that case, we should annotate so in the clobberize. But in the following case, we use NotCellUse edge filter. That filters Symbols since Symbols are Cells. So here, we do not need to consider about ToString(Symbol) case in the clobberize if the edge filter is NotCellUse. >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7940 >> + callOperation(operationToString, resultGPR, extractResult(op1Regs)); > > I understand what extractResult does, but it's a very weird function name to be calling for an operand to a function. Perhaps we can come up with a better name. Make sense. Instead, I added a definition for callOperation(C_JITOperation_EJ, GPRReg, JSValueRegs) for 64bit. It drops them and makes it cleaner. >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7943 >> + callOperation(operationCallStringConstructor, resultGPR, extractResult(op1Regs)); > > ditto. Ditto. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:4942 >> + value = lowNotCell(m_node->child1()); > > This is very weird. You're emitting lowNotCell, but then branching on isCell. > Please add a new place to emit code for this. Or just assign isCellPredicate to false. Thanks! I've just assigned isCellPredicate to false. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:12240 >> + } > > Style nit: > I think this makes more sense if you did this the other way around: > Implement speculateNotCell in terms of lowNotCell. > That way, speculateNotCell would be implemented like: > > void speculateNotCell(Edge edge) { lowNotCell(edge); } > > (This is how lowCell and speculateCell work.) Done
Committed r214028: <http://trac.webkit.org/changeset/214028>