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!
Created attachment 348629 [details] Patch
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
Created attachment 348706 [details] Patch
Ping?
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.
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.
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
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.
Created attachment 349041 [details] Patch
Can you review again?
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*?
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.
Committed r235790: <https://trac.webkit.org/changeset/235790>
<rdar://problem/44234420>