Bug 155974 - [JSC] Implement String.prototype.repeat in builtins JS
Summary: [JSC] Implement String.prototype.repeat in builtins JS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-03-29 10:18 PDT by Yusuke Suzuki
Modified: 2016-03-31 07:59 PDT (History)
8 users (show)

See Also:


Attachments
Patch (15.04 KB, patch)
2016-03-29 17:12 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (42.88 KB, patch)
2016-03-30 06:56 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2016-03-29 10:18:37 PDT
...
Comment 1 Yusuke Suzuki 2016-03-29 17:12:00 PDT
Created attachment 275155 [details]
Patch
Comment 2 Mark Lam 2016-03-29 17:48:17 PDT
Since you're converting the implementation to JS, can you please run perf numbers to make sure that there's no regression?  Thanks.
Comment 3 Yusuke Suzuki 2016-03-29 18:13:45 PDT
(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 :)
Comment 4 Yusuke Suzuki 2016-03-30 06:56:06 PDT
Created attachment 275195 [details]
Patch
Comment 5 Yusuke Suzuki 2016-03-30 06:57:11 PDT
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
Comment 6 Yusuke Suzuki 2016-03-30 07:03:58 PDT
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.
Comment 7 Yusuke Suzuki 2016-03-30 08:45:27 PDT
Comment on attachment 275195 [details]
Patch

Thanks for review :)
Comment 8 WebKit Commit Bot 2016-03-30 09:34:29 PDT
Comment on attachment 275195 [details]
Patch

Clearing flags on attachment: 275195

Committed r198838: <http://trac.webkit.org/changeset/198838>
Comment 9 WebKit Commit Bot 2016-03-30 09:34:33 PDT
All reviewed patches have been landed.  Closing bug.