Bug 156013 – ES6: Implement String.prototype.split and RegExp.prototype[@@split].

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

[Mark Lam]

Patch coming.

[Radar WebKit Bug Importer]

<rdar://problem/25435210>

[Mark Lam]

Created attachment 275415 [details]
work in progress: still need tests an a real @isPureRegExp implementation.

[Mark Lam]

Created attachment 275697 [details]
work in progress: need to apply get_by_id_pure next.

[Mark Lam]

Created attachment 275817 [details]
proposed patch.

This patch should be correct.  Some layout tests may need to be rebased, and I'll do if needed that after I run them.

Note that hasObservableSideEffects() in the JS regexp split implementation checks a lot of stuff, and I have to call it twice for subclasses of RegExp because their species constructor does not match the RegExp one by definition.  Hence, there might be some perf implications here.  Will find out after @tryGetById gets some DFG love from Keith.

[Mark Lam]

Created attachment 276240 [details]
proposed patch.

[Mark Lam]

Created attachment 276241 [details]
x86_64 benchmark results.

[Mark Lam]

Perf is neutral with this patch.  All JSC and layout tests pass on x86_64 with a release build.

[WebKit Commit Bot]

Attachment 276240 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:8:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 1 in 80 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Created attachment 276243 [details]
proposed patch: with a better name for hasObservableSideEffectsForRegExpSplit().

[Keith Miller]

Comment on attachment 276243 [details]
proposed patch: with a better name for hasObservableSideEffectsForRegExpSplit().

View in context: https://bugs.webkit.org/attachment.cgi?id=276243&action=review

r=me with comments.

> Source/JavaScriptCore/builtins/RegExpPrototype.js:101
> +    let builtinExec = @RegExp.prototype.@exec;

What's the advantage of having this on the RegExp prototype over the global object?

> Source/JavaScriptCore/builtins/RegExpPrototype.js:114
> +    if (regexpExec !== @RegExp.prototype.@exec)

Ditto.

> Source/JavaScriptCore/builtins/RegExpPrototype.js:154
> +    if (!(this instanceof @Object))

This is not correct and slow! I can do: foo = {}; foo.__proto__ = null;  split.call(foo,...) and I will get a type error when it should work fine. You should use the @isObject function, which is also much faster.

> Source/JavaScriptCore/builtins/RegExpPrototype.js:172
> +    let unicodeMatching = flags.includes("u");

What happens here if someone deletes String.prototype.includes? Does this still work?

> Source/JavaScriptCore/builtins/RegExpPrototype.js:189
> +    // 13. If limit is undefined, let lim be 232-1; else let lim be ? ToUint32(limit).

Is this supposed to be 2^32-1?

> Source/JavaScriptCore/builtins/RegExpPrototype.js:207
> +        result[0] = str;

I think you need to use @putDirectByVal here. CreateDataProperty does not look at the prototype chain, which result[0] will do.

> Source/JavaScriptCore/builtins/RegExpPrototype.js:259
> +                    result.push(nextCapture);

I don't think you can use push here since result might not be an array and push might be customized. You need to do @putDirectByVal.

> Source/JavaScriptCore/builtins/RegExpPrototype.js:274
> +    result.push(remainingStr);

Ditto.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:652
> +        // @substrInternal should not have any observable side effects (e.g. it should not call
> +        // GetMethod(..., @@toPrimitive) on the thisValue). It is ok to use the default
> +        // stringProtoFuncSubstr as the implementation of @substrInternal because @substrInternal
> +        // will only be called by builtins, which will guarantee that we only pass it a string
> +        // thisValue. As a result, stringProtoFuncSubstr will not need to call toString() on the
> +        // thisValue, and there will be no observable side-effects.

Why is this comment here and not at the implementation of stringProtoFuncSubstr?

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1189
>      JSValue separatorValue = exec->argument(0);

This can be uncheckedArgument since it's only called by us.

[Mark Lam]

Comment on attachment 276243 [details]
proposed patch: with a better name for hasObservableSideEffectsForRegExpSplit().

View in context: https://bugs.webkit.org/attachment.cgi?id=276243&action=review

>> Source/JavaScriptCore/builtins/RegExpPrototype.js:101
>> +    let builtinExec = @RegExp.prototype.@exec;
> 
> What's the advantage of having this on the RegExp prototype over the global object?

This was a pre-existing idiom from the @@match implementation.  In the interest of not changing too many things at once, I'll leave it as is for now.  I'll do a follow up patch to bring this over to a global.

>> Source/JavaScriptCore/builtins/RegExpPrototype.js:114
>> +    if (regexpExec !== @RegExp.prototype.@exec)
> 
> Ditto.

Same as above.

>> Source/JavaScriptCore/builtins/RegExpPrototype.js:154
>> +    if (!(this instanceof @Object))
> 
> This is not correct and slow! I can do: foo = {}; foo.__proto__ = null;  split.call(foo,...) and I will get a type error when it should work fine. You should use the @isObject function, which is also much faster.

Thanks.  Fixed.

>> Source/JavaScriptCore/builtins/RegExpPrototype.js:172
>> +    let unicodeMatching = flags.includes("u");
> 
> What happens here if someone deletes String.prototype.includes? Does this still work?

Thanks.  Will fix and add a test case.

>> Source/JavaScriptCore/builtins/RegExpPrototype.js:189
>> +    // 13. If limit is undefined, let lim be 232-1; else let lim be ? ToUint32(limit).
> 
> Is this supposed to be 2^32-1?

Yes.  Fixed.

>> Source/JavaScriptCore/builtins/RegExpPrototype.js:207
>> +        result[0] = str;
> 
> I think you need to use @putDirectByVal here. CreateDataProperty does not look at the prototype chain, which result[0] will do.

Thanks.  Fixed.

>> Source/JavaScriptCore/builtins/RegExpPrototype.js:259
>> +                    result.push(nextCapture);
> 
> I don't think you can use push here since result might not be an array and push might be customized. You need to do @putDirectByVal.

Thanks. Fixed.

>> Source/JavaScriptCore/builtins/RegExpPrototype.js:274
>> +    result.push(remainingStr);
> 
> Ditto.

Fixed.

>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:652
>> +        // thisValue, and there will be no observable side-effects.
> 
> Why is this comment here and not at the implementation of stringProtoFuncSubstr?

Because this is where we define @substrInternal, and this comment pertains to @substrInternal.

On further consideration, I think it's clearer to have an explicit builtinStringSubstrInternal() function and to move this comment there.  Plus I can add some asserts there.  I'm going with that.

>> Source/JavaScriptCore/runtime/StringPrototype.cpp:1189
>>      JSValue separatorValue = exec->argument(0);
> 
> This can be uncheckedArgument since it's only called by us.

Thanks.  Fixed.

[Mark Lam]

Created attachment 276281 [details]
patch for landing (w/ Keith's feedback resolved).

[Mark Lam]

Created attachment 276282 [details]
Diff between patches.

[Keith Miller]

Comment on attachment 276281 [details]
patch for landing (w/ Keith's feedback resolved).

View in context: https://bugs.webkit.org/attachment.cgi?id=276281&action=review

> Source/JavaScriptCore/builtins/RegExpPrototype.js:209
> +        result.length = 1;

unneeded.

> Source/JavaScriptCore/builtins/RegExpPrototype.js:249
> +                if (resultLength == limit) {
> +                    result.length = resultLength;
> +                    return result;
> +                }

unneeded.

> Source/JavaScriptCore/builtins/RegExpPrototype.js:270
> +                    if (resultLength == limit) {
> +                        result.length = resultLength;
> +                        return result;
> +                    }

unneeded.

> Source/JavaScriptCore/builtins/RegExpPrototype.js:283
> +    result.length = ++resultLength;

unneeded.

[Keith Miller]

Comment on attachment 276281 [details]
patch for landing (w/ Keith's feedback resolved).

View in context: https://bugs.webkit.org/attachment.cgi?id=276281&action=review

>> Source/JavaScriptCore/builtins/RegExpPrototype.js:249
>> +                }
> 
> unneeded.

Whoops, this should just refer the length setting part.

>> Source/JavaScriptCore/builtins/RegExpPrototype.js:270
>> +                    }
> 
> unneeded.

Ditto.

[Mark Lam]

Created attachment 276285 [details]
patch for landing 2: removed unneeded explicit setting of result.length.

[Mark Lam]

Created attachment 276286 [details]
Diff between patches 2.

[Mark Lam]

Thanks for all the reviews.  Landed in r199393: <http://trac.webkit.org/r199393>.

[Mark Lam]

Speculative rollout in r199400: <http://trac.webkit.org/r199400> to fix this failure: https://build.webkit.org/builders/Apple%20El%20Capitan%2032-bit%20JSC%20%28BuildAndTest%29/builds/2075

[Alexey Proskuryakov]

The rollout didn't fix 32-bit failures, still occurring with r199401.

[Mark Lam]

The crash issue was due to https://bugs.webkit.org/show_bug.cgi?id=156532, and is now fixed.

Re-landed r199393 in r199502: <http://trac.webkit.org/r199502>.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 156557

[Mark Lam]

Created attachment 276732 [details]
browser sunspider benchmark baseline on MBP

[Mark Lam]

Created attachment 276733 [details]
browser sunspider benchmark new on MBP

[Mark Lam]

I've benchmarked this patch (on rev 199568) many times on a MBP with the in browser sunspider, and the results show no regression.  Whatever perf regression appears on arewefastyet.com previously are probably due to interplay with other factors (maybe cache effects?).

I will re-land and monitor the bots.

[Mark Lam]

Re-landed in r199731: <http://trac.webkit.org/r199731>.

[Mark Lam]

Created attachment 276742 [details]
browser sunspider benchmark baseline on MacPro

[Mark Lam]

Created attachment 276743 [details]
browser sunspider benchmark new on MacPro

In browser Sunspider benchmark results on my MacPro agrees.  There's no perf regression here.

[Mark Lam]

Strange.  http://arewefastyet.com/#machine=29&view=breakdown&suite=ss is still complaining that the time for ss-format-xparb has nearly doubled (likely due to this patch).  But locally, I can't replicate the issue at all.  Investigating ...

[Mark Lam]

Command line sunspider run without warmup ...

                              base                      new                                        
Normal FTL on:
date-format-xparb        5.9714+-0.0932            5.8345+-0.2865          might be 1.0235x faster
FTL disable:
date-format-xparb        5.7622+-0.2792            5.6508+-0.1500          might be 1.0197x faster
DFG disable:
date-format-xparb       13.0135+-0.5711           12.5326+-0.3871          might be 1.0384x faster
Baseline JIT disabled:
date-format-xparb       20.2147+-0.3067           20.1280+-0.3900        

Based on the above, it does not look like there are any regressions at any tier level either.

[Mark Lam]

All internal test bots also show no regression.  The arewefastyet bot is an anomaly.  Will ignore and move on to other things.