Bug 189151 – [DFG] DFG should handle String#toString

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Nicholas Shanks]

Created attachment 348495 [details]
Screenshot of my results including Firefox

I ran some performance tests at https://jsperf.com/string-object-conversion-to-primitive and found that Chrome is 25% faster than Safari at converting a 32kB String object to a string primitive. Perhaps there is some win that can be achieved by copying whatever they're doing?

My numbers, Safari 11.1.2 (Mac OS 10.11.6):
Object.valueOf()	144,794,835 ± 0.7%
Object.toString()	142,425,187 ± 1.0%

Chrome 68.0.3440:
Object.valueOf()	177,410,422 ± 1.33%
Object.toString()	178,729,042 ± 1.12%


p.s. Safari blows Chrome out of the water with the `+` operator, at almost 100x faster!

[Yusuke Suzuki]

Created attachment 348629 [details]
Patch

[EWS Watchlist]

Comment on attachment 348629 [details]
Patch

Attachment 348629 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/9050571

New failing tests:
stress/string-value-of.js.ftl-eager-no-cjit-b3o1
stress/string-value-of.js.dfg-eager-no-cjit-validate
stress/string-value-of.js.ftl-no-cjit-no-put-stack-validate
stress/string-value-of.js.ftl-no-cjit-no-inline-validate
stress/string-value-of.js.default
stress/string-value-of.js.ftl-no-cjit-b3o1
stress/string-value-of.js.dfg-maximal-flush-validate-no-cjit
stress/string-value-of.js.ftl-no-cjit-small-pool
stress/string-value-of.js.dfg-eager
stress/string-value-of.js.ftl-no-cjit-validate-sampling-profiler
stress/string-value-of.js.ftl-eager-no-cjit
stress/string-value-of.js.ftl-eager
stress/string-value-of.js.no-cjit-collect-continuously
stress/string-value-of.js.no-cjit-validate-phases
stress/string-value-of.js.no-llint
stress/string-value-of.js.no-ftl
apiTests

[Yusuke Suzuki]

Created attachment 348706 [details]
Patch

[Yusuke Suzuki]

Ping?

[Filip Pizlo]

Comment on attachment 348706 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348706&action=review

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2697
> +        addToGraph(Check, Edge(value, StringOrStringObjectUse));

I think that usually when we add new things to the DFG we also add the generic version. Any reason not to do that here?

Specifically:
- add a NodeType for this operation
- parser just emits UntypedUse version
- fixup converts to Check StringOrStringObjectUse and ToString if the predictions say it’s ok
- add support for the UntypedUse version to the back ends. 

I think that now that we have high coverage in the optimizing JITs, we should avoid regressing that by adding static speculations.

[Yusuke Suzuki]

Comment on attachment 348706 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348706&action=review

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2697
>> +        addToGraph(Check, Edge(value, StringOrStringObjectUse));
> 
> I think that usually when we add new things to the DFG we also add the generic version. Any reason not to do that here?
> 
> Specifically:
> - add a NodeType for this operation
> - parser just emits UntypedUse version
> - fixup converts to Check StringOrStringObjectUse and ToString if the predictions say it’s ok
> - add support for the UntypedUse version to the back ends. 
> 
> I think that now that we have high coverage in the optimizing JITs, we should avoid regressing that by adding static speculations.

This is because `StringOrStringObjectUse` usekind is the widest use kind for StringPrototypeValueOf.
If the other type of value comes, String#valueOf throws an error. So, DFG exit occurs.

>>> String.prototype.valueOf.call({})
Exception: TypeError: Type error

That's why I thought we do not need to add a new DFG node for StringValueOf and lower it to ToString in fixup.
All the cases not causing an exit can be handled with this check.

[Saam Barati]

Comment on attachment 348706 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348706&action=review

>>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2697
>>> +        addToGraph(Check, Edge(value, StringOrStringObjectUse));
>> 
>> I think that usually when we add new things to the DFG we also add the generic version. Any reason not to do that here?
>> 
>> Specifically:
>> - add a NodeType for this operation
>> - parser just emits UntypedUse version
>> - fixup converts to Check StringOrStringObjectUse and ToString if the predictions say it’s ok
>> - add support for the UntypedUse version to the back ends. 
>> 
>> I think that now that we have high coverage in the optimizing JITs, we should avoid regressing that by adding static speculations.
> 
> This is because `StringOrStringObjectUse` usekind is the widest use kind for StringPrototypeValueOf.
> If the other type of value comes, String#valueOf throws an error. So, DFG exit occurs.

The DFG knows how to exit for exceptions without jettisoning. So repeatedly exiting because of a type check is different. That will jettison the compilation

[Yusuke Suzuki]

Comment on attachment 348706 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348706&action=review

>>>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2697
>>>> +        addToGraph(Check, Edge(value, StringOrStringObjectUse));
>>> 
>>> I think that usually when we add new things to the DFG we also add the generic version. Any reason not to do that here?
>>> 
>>> Specifically:
>>> - add a NodeType for this operation
>>> - parser just emits UntypedUse version
>>> - fixup converts to Check StringOrStringObjectUse and ToString if the predictions say it’s ok
>>> - add support for the UntypedUse version to the back ends. 
>>> 
>>> I think that now that we have high coverage in the optimizing JITs, we should avoid regressing that by adding static speculations.
>> 
>> This is because `StringOrStringObjectUse` usekind is the widest use kind for StringPrototypeValueOf.
>> If the other type of value comes, String#valueOf throws an error. So, DFG exit occurs.
> 
> The DFG knows how to exit for exceptions without jettisoning. So repeatedly exiting because of a type check is different. That will jettison the compilation

Interesting. I'll add StringValueOf, lower it to ToString / Identity in fixup, and add StringValueOf(Untyped) implementation.

[Yusuke Suzuki]

Created attachment 349041 [details]
Patch

[Yusuke Suzuki]

Can you review again?

[Saam Barati]

Comment on attachment 349041 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349041&action=review

r=me

> Source/JavaScriptCore/dfg/DFGOperations.cpp:2028
> +    auto* stringObject = jsDynamicCast<StringObject*>(vm, argument);
> +    if (stringObject)

style nit: I think this is cleaner if you just did:
if (auto* stringObject = jsDynamicCast<StringObject*>(vm, argument)) ...

> Source/JavaScriptCore/dfg/DFGOperations.h:196
> +JSCell* JIT_OPERATION operationStringValueOf(ExecState*, EncodedJSValue);

Why not return JSString*?

[Yusuke Suzuki]

Comment on attachment 349041 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349041&action=review

>> Source/JavaScriptCore/dfg/DFGOperations.cpp:2028
>> +    if (stringObject)
> 
> style nit: I think this is cleaner if you just did:
> if (auto* stringObject = jsDynamicCast<StringObject*>(vm, argument)) ...

Nice, fixed.

>> Source/JavaScriptCore/dfg/DFGOperations.h:196
>> +JSCell* JIT_OPERATION operationStringValueOf(ExecState*, EncodedJSValue);
> 
> Why not return JSString*?

We can do that, fixed.

[Yusuke Suzuki]

Committed r235790: <https://trac.webkit.org/changeset/235790>

[Radar WebKit Bug Importer]

<rdar://problem/44234420>