Bug 155974 – [JSC] Implement String.prototype.repeat in builtins JS

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

[Yusuke Suzuki]

...

[Yusuke Suzuki]

Created attachment 275155 [details]
Patch

[Mark Lam]

Since you're converting the implementation to JS, can you please run perf numbers to make sure that there's no regression?  Thanks.

[Yusuke Suzuki]

(In reply to comment #2)
> Since you're converting the implementation to JS, can you please run perf
> numbers to make sure that there's no regression?  Thanks.

Since no existing performance tests uses String.prototype.repeat, I'll add a simple js/regress tests to measure the performance :)

[Yusuke Suzuki]

Created attachment 275195 [details]
Patch

[Yusuke Suzuki]

Updated the patch. Here is the performance results. (newly added test cases)

Benchmark report for JSRegress on 192-168-22-121 (MacBookPro10,1).

VMs tested:
"Baseline" at /Users/yusukesuzuki/dev/WebKit/WebKitBuild/string-repeat-master/Release/jsc
"JS" at /Users/yusukesuzuki/dev/WebKit/WebKitBuild/string-repeat/Release/jsc

Collected 10 samples per benchmark/VM, with 10 VM invocations per benchmark. Emitted a call to gc() between sample
measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the jsc-specific preciseTime() function to
get microsecond-level timing. Reporting benchmark execution times with 95% confidence intervals in milliseconds.

                                                Baseline                     JS                                        

string-repeat-not-resolving-fixed           14.8288+-0.6408     ^      3.7547+-0.1815        ^ definitely 3.9494x faster
string-repeat-not-resolving-no-inline      651.4889+-6.0899     ^      5.3789+-0.2811        ^ definitely 121.1192x faster
string-repeat-not-resolving                649.4908+-5.2484     ^      5.2331+-0.0694        ^ definitely 124.1125x faster
string-repeat-resolving-fixed               31.1334+-0.9833     ^     27.4511+-1.0381        ^ definitely 1.1341x faster
string-repeat-resolving-no-inline         1476.1249+-12.9186    ^   1064.8980+-5.9103        ^ definitely 1.3862x faster
string-repeat-resolving                   1480.4327+-8.7177     ^   1060.9495+-6.4284        ^ definitely 1.3954x faster
string-repeat-single-not-resolving           9.2135+-1.1121     ?      9.3901+-0.8220        ? might be 1.0192x slower
string-repeat-single-resolving               9.9348+-0.6680            9.6160+-0.8545          might be 1.0332x faster
string-repeat-small-not-resolving          645.1441+-5.2865     ^      4.3385+-0.3318        ^ definitely 148.7004x faster
string-repeat-small-resolving             1423.9953+-13.7065    ^   1000.8561+-6.2684        ^ definitely 1.4228x faster

<geometric>                                179.7742+-1.8844     ^     32.3610+-0.6781        ^ definitely 5.5553x faster

[Yusuke Suzuki]

Comment on attachment 275195 [details]
Patch

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

Added comments.

> Source/JavaScriptCore/ChangeLog:29
> +        of the generated string. As a result, the generated rope is not resolved.

Of course, the test cases postfixed "-resolving" attempt to resolve the given string. But in these cases, still we can observe the significant performance improvement. This is due to the reason (2).

> Source/JavaScriptCore/builtins/StringPrototype.js:74
> +    if (repeatCount < 0 || repeatCount === @Infinity)

-infinity, negative numbers, and +infinity fall into this branch.

> Source/JavaScriptCore/builtins/StringPrototype.js:78
> +    if (repeatCount === 0 || string.length === 0)

repeatCount === -0 case will fall into this branch.

> Source/JavaScriptCore/builtins/StringPrototype.js:89
> +        // Here, |repeatCount| is always Int32.

So, here, -0 never comes.

> Source/JavaScriptCore/builtins/StringPrototype.js:107
> +function repeat(count)

Keep this code very small, to make this inlineable.

> Source/JavaScriptCore/builtins/StringPrototype.js:121
> +        if (result !== null)

If the count is not int32, we return null.

[Yusuke Suzuki]

Comment on attachment 275195 [details]
Patch

Thanks for review :)

[WebKit Commit Bot]

Comment on attachment 275195 [details]
Patch

Clearing flags on attachment: 275195

Committed r198838: <http://trac.webkit.org/changeset/198838>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.