Bug 172115 – [DFG] Constant Folding Phase should convert MakeRope("", String) => Identity(String)

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

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!

[Yusuke Suzuki]

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

[Yusuke Suzuki]

Created attachment 310129 [details]
Patch

[Yusuke Suzuki]

Created attachment 310130 [details]
Patch

[Yusuke Suzuki]

Created attachment 310136 [details]
Patch

[Saam Barati]

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?

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 310257 [details]
Patch

[Yusuke Suzuki]

Created attachment 310259 [details]
Patch

[Sam Weinig]

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.

[Saam Barati]

(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!

[Saam Barati]

Comment on attachment 310259 [details]
Patch

r=me

[Yusuke Suzuki]

Comment on attachment 310259 [details]
Patch

Thanks!

[WebKit Commit Bot]

Comment on attachment 310259 [details]
Patch

Clearing flags on attachment: 310259

Committed r216948: <http://trac.webkit.org/changeset/216948>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yusuke Suzuki]

It also works in six-speed. https://arewefastyet.com/#machine=29&view=single&suite=six-speed&subtest=templatestringtag-es5