Bug 169544 – [DFG] ToString operation should have fixup for primitives to say this node does not have side effects

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

...

[Yusuke Suzuki]

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!

[Yusuke Suzuki]

Created attachment 304268 [details]
Patch

[Yusuke Suzuki]

Created attachment 304270 [details]
Patch

[Saam Barati]

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.)

[Yusuke Suzuki]

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

[Yusuke Suzuki]

Committed r214028: <http://trac.webkit.org/changeset/214028>