| Summary: | [DFG] DFG should handle String#toString | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Nicholas Shanks <nickshanks> | ||||||||||
| Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | ews-watchlist, fpizlo, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer, ysuzuki | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | Safari 11 | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Nicholas Shanks
2018-08-30 03:37:21 PDT
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> |