Summary: | [DFG] Constant Folding Phase should convert MakeRope("", String) => Identity(String) | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||||||
Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | buildbot, commit-queue, fpizlo, keith_miller, mark.lam, msaboff, saam, sam | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Yusuke Suzuki
2017-05-15 05:36:15 PDT
Before: firstIteration: 50.02 +- 14.56 ms averageWorstCase: 26.52 +- 4.52 ms steadyState: 8.15 +- 0.23 ms summary: 22.03 +- 3.38 ms After: firstIteration: 49.08 +- 12.90 ms averageWorstCase: 25.16 +- 3.82 ms steadyState: 7.58 +- 0.21 ms summary: 20.99 +- 2.73 ms Created attachment 310129 [details]
Patch
Created attachment 310130 [details]
Patch
Created attachment 310136 [details]
Patch
Comment on attachment 310136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310136&action=review Mostly LGTM, just a few comments > Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:643 > + case MakeRope: { I believe you need this code inside AbstractInterpreter as well to indicate we've found a constant. Perhaps all of it can just be moved there. > Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:653 > + const auto* string = asString(childConstant)->tryGetValueImpl(); Nit: Can we give this a type, since it's hard to see what's going on here without reading tryGetValueImpl()'s type? > Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:661 > + // Don't allow the MakeRope to have zero children. > + if (!i && !node->child2()) > + break; Why not just convert to lazy constant for "" here? Comment on attachment 310136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310136&action=review Thanks >> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:643 >> + case MakeRope: { > > I believe you need this code inside AbstractInterpreter as well to indicate we've found a constant. Perhaps all of it can just be moved there. Oops, right. I think we should have MakeRope here too since it can reduce the number of children (like, "" + a + b => a + b). >> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:653 >> + const auto* string = asString(childConstant)->tryGetValueImpl(); > > Nit: Can we give this a type, since it's hard to see what's going on here without reading tryGetValueImpl()'s type? OK, I've changed it to `StringImpl*`. >> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:661 >> + break; > > Why not just convert to lazy constant for "" here? I think it is better to replace it to Identity(child1). And current code covers all the cases in the following (L668) convertToIdentity simply. Created attachment 310257 [details]
Patch
Created attachment 310259 [details]
Patch
Comment on attachment 310259 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310259&action=review > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:575 > + if (!edge) > + break; How can edge be null if it is a reference? I guess that works if Edge has a operator!, but its still a bit odd. (In reply to Sam Weinig from comment #9) > Comment on attachment 310259 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=310259&action=review > > > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:575 > > + if (!edge) > > + break; > > How can edge be null if it is a reference? I guess that works if Edge has a > operator!, but its still a bit odd. I agree it's a bit odd, but we do this all over the DFG. It does have an operator! Comment on attachment 310259 [details]
Patch
r=me
Comment on attachment 310259 [details]
Patch
Thanks!
Comment on attachment 310259 [details] Patch Clearing flags on attachment: 310259 Committed r216948: <http://trac.webkit.org/changeset/216948> All reviewed patches have been landed. Closing bug. It also works in six-speed. https://arewefastyet.com/#machine=29&view=single&suite=six-speed&subtest=templatestringtag-es5 |