1288772 - Move String.fromCodePoint to native code

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[André Bargull [:anba]]

The performance of String.fromCodePoint can be significantly improved when the function is implemented in C++.


Micro-benchmark:
---
function f() {
    var len = 0;
    var d = Date.now();
    for (var i = 0; i < 1000000; ++i) len += String.fromCodePoint(0x61).length;
    var e = Date.now() - d;
    if (len !== 1000000*1*1) print("bad length");
    return e;
}

var d = 0;
for (var i = 0; i < 15; ++i) {
    var r = f();
    d += r;
    print(r);
}
print("Mean: ", (d / 15));
---


Results with current patch:

String.fromCodePoint( 1 x 0x61): before =   420 ms, after =   20 ms
String.fromCodePoint( 2 x 0x61): before =   680 ms, after =   37 ms
String.fromCodePoint( 5 x 0x61): before =  1340 ms, after =   95 ms
String.fromCodePoint(10 x 0x61): before =  2680 ms, after =  260 ms
String.fromCodePoint(50 x 0x61): before = 11800 ms, after = 1040 ms

String.fromCodePoint( 1 x 0x10ffff): before =   720 ms, after =   65 ms
String.fromCodePoint( 2 x 0x10ffff): before =  1135 ms, after =   73 ms
String.fromCodePoint( 5 x 0x10ffff): before =  2650 ms, after =  110 ms
String.fromCodePoint(10 x 0x10ffff): before =  5220 ms, after =  430 ms
String.fromCodePoint(50 x 0x10ffff): before = 23800 ms, after = 1050 ms

Comment 1

[André Bargull [:anba]]

Attachment: bug1288772.patch

I had a braino in the first patch, which caused unnecessary allocations. The updated patch gives the following performance results:

String.fromCodePoint( 1 x 0x61), before =   420 ms, after =   20 ms
String.fromCodePoint( 2 x 0x61), before =   680 ms, after =   37 ms
String.fromCodePoint( 5 x 0x61), before =  1340 ms, after =   95 ms
String.fromCodePoint(10 x 0x61), before =  2680 ms, after =  260 ms
String.fromCodePoint(50 x 0x61), before = 11800 ms, after =  800 ms

String.fromCodePoint( 1 x 0x10ffff), before =   720 ms, after =   65 ms
String.fromCodePoint( 2 x 0x10ffff), before =  1135 ms, after =   73 ms
String.fromCodePoint( 5 x 0x10ffff), before =  2650 ms, after =  110 ms
String.fromCodePoint(10 x 0x10ffff), before =  5220 ms, after =  290 ms
String.fromCodePoint(50 x 0x10ffff), before = 23800 ms, after =  800 ms

Comment 2

[Tom S. (please needinfo tschuster)]

Comment on attachment 8773872 [details] [diff] [review]
bug1288772.patch

Review of attachment 8773872 [details] [diff] [review]:
-----------------------------------------------------------------

Can you make sure all the three different variants are exercised in out test suite?

::: js/src/jsstr.cpp
@@ +2757,5 @@
> +    return codePoint > 0xFFFF;
> +}
> +
> +static inline char16_t
> +LeadSurrogate(uint32_t codePoint)

Sadly this is not the exact definition that ECMAScript uses, but it seems correct.

@@ +2765,5 @@
> +
> +static inline char16_t
> +TrailSurrogate(uint32_t codePoint)
> +{
> +    return char16_t((codePoint & 0x3FF) + 0xDC00);

You can write | 0xDC00 here.

@@ +2853,5 @@
> +str_fromCodePoint(JSContext* cx, unsigned argc, Value* vp)
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +
> +    MOZ_ASSERT(args.length() <= ARGS_LENGTH_MAX);

I don't think we need this.

@@ +2876,5 @@
> +        return false;
> +
> +    // Steps 4-5.
> +    unsigned length = 0;
> +    for (unsigned nextIndex = 0; nextIndex < args.length(); nextIndex++) {

Can we share this with the few_args variant?

Comment 3

[André Bargull [:anba]]

Attachment: bug1288772.patch

Updated patch, carrying r+ from evilpie.

Updates:
- Added ToCodePoint method to convert a Value to a code point. MOZ_ALWAYS_INLINE is required to ensure the performance numbers reported in comment #1 don't change.
Without always inline:
String.fromCodePoint( 1 x 0x61), time =   27 ms
String.fromCodePoint( 2 x 0x61), time =   45 ms
String.fromCodePoint( 5 x 0x61), time =  115 ms
String.fromCodePoint(10 x 0x61), time =  285 ms
...

- Added UTF16Encode method to store a code point into an char16_t array.

- Replaced the MOZ_ASSERT (which was copied over from str_fromCharCode) with a static_assert to ensure |args.length() * 2 + 1| does not overflow.

- And applied the other suggestions.

Comment 4

[Tom S. (please needinfo tschuster)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cceacf30346

Ugh we have to stop forgetting your patches.

Comment 5

[André Bargull [:anba]]

(In reply to Tom Schuster [:evilpie] from comment #4)
> Ugh we have to stop forgetting your patches.

No worries, it's actually my fault for not requesting try-access. ;-)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a30d64f85722
Move String.fromCodePoint to native code. r=evilpie

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a30d64f85722

Comment 8

[Tom S. (please needinfo tschuster)]

Done