This optimization is performed in Fixup phase. But in fixup phase, we may not figure out many constant values... We should later perform this optimization in constant folding phase again. I investigated ARES-6 Babylon and figured out that Babylon's Array#indexOf repeatedly find roped string. This is because of readWord1 in Babylon, the implementation is like, var word = ""; ... ... ... return word + data.slice(...); Due to the above limitation, this now accidentally produces MakeRope!
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