Bug 169544

Summary: [DFG] ToString operation should have fixup for primitives to say this node does not have side effects
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, keith_miller, mark.lam, msaboff, sbarati
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch sbarati: review+

Description Yusuke Suzuki 2017-03-13 01:45:39 PDT
...
Comment 1 Yusuke Suzuki 2017-03-13 09:49:43 PDT
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!
Comment 2 Yusuke Suzuki 2017-03-13 10:21:51 PDT
Created attachment 304268 [details]
Patch
Comment 3 Yusuke Suzuki 2017-03-13 10:27:00 PDT
Created attachment 304270 [details]
Patch
Comment 4 Saam Barati 2017-03-15 16:38:27 PDT
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 5 Yusuke Suzuki 2017-03-15 21:38:26 PDT
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
Comment 6 Yusuke Suzuki 2017-03-15 21:49:55 PDT
Committed r214028: <http://trac.webkit.org/changeset/214028>