Bug 144160 – [ES6] Implement String.fromCodePoint

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[ES6] Implement String.fromCodePoint

[Yusuke Suzuki]

Created attachment 251579 [details]
Patch

[Build Bot]

Comment on attachment 251579 [details]
Patch

Attachment 251579 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5169665965817856

New failing tests:
js/Object-getOwnPropertyNames.html

[Build Bot]

Created attachment 251581 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 251579 [details]
Patch

Attachment 251579 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5934236179628032

New failing tests:
js/Object-getOwnPropertyNames.html

[Build Bot]

Created attachment 251582 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Alexey Proskuryakov]

Comment on attachment 251579 [details]
Patch

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

> Source/JavaScriptCore/runtime/StringConstructor.cpp:100
> +    StringBuilder builder;
> +
> +    for (unsigned i = 0; i < length; ++i) {

Would it be more efficient to preallocate a 2*length size buffer than using StringBuilder?

[Darin Adler]

Comment on attachment 251579 [details]
Patch

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

Need to update the Object.getOwnPropertyNames test.

>> Source/JavaScriptCore/runtime/StringConstructor.cpp:100
>> +    for (unsigned i = 0; i < length; ++i) {
> 
> Would it be more efficient to preallocate a 2*length size buffer than using StringBuilder?

Or since it’s highly likely it will be length, not 2*length, should just call:

    builder.reserveCapacity(length);

> Source/JavaScriptCore/runtime/StringConstructor.cpp:101
> +        double nextCP = exec->uncheckedArgument(i).toNumber(exec);

I suggest naming this codePointAsDouble rather than nextCP.

> Source/JavaScriptCore/runtime/StringConstructor.cpp:109
> +        if (!sameValue(exec, jsNumber(nextCP), jsNumber(jsNumber(nextCP).toInteger(exec))))
> +            return throwVMError(exec, createRangeError(exec, ASCIILiteral("Arguments contain a value that is out of range of code points")));
> +
> +        if (nextCP < 0 || nextCP > 0x10FFFF)
> +            return throwVMError(exec, createRangeError(exec, ASCIILiteral("Arguments contain a value that is out of range of code points")));

This conversion back to jsNumber is really strange. I think we can do this check without using the sameValue function and without all this conversion back and forth.

I think it’s probably simply this:

    UChar32 codePoint = codePointAsDouble;
    if (codePoint != codePointAsDouble || codePoint > 0x10FFFF)
        return throwVMError(exec, createRangeError(exec, ASCIILiteral("Arguments contain a value that is out of range of code points")));

> Source/JavaScriptCore/runtime/StringConstructor.cpp:111
> +        unsigned codePoint = static_cast<unsigned>(nextCP);

Should be UChar32 rather than unsigned.

> Source/JavaScriptCore/runtime/StringConstructor.cpp:119
> +        int encodeOffset = 0;
> +        UChar encodeBuffer[2] = { 0 };
> +        U16_APPEND_UNSAFE(encodeBuffer, encodeOffset, codePoint);
> +
> +        builder.append(encodeBuffer[0]);
> +        if (encodeOffset == 2)
> +            builder.append(encodeBuffer[1]);

Here is a much cleaner way to write it:

    if (U_IS_BMP(codePoint))
        builder.append(codePoint);
    else {
        builder.append(U16_LEAD(codePoint));
        builder.append(U16_TRAIL(codePoint));
    }

[Yusuke Suzuki]

Created attachment 251632 [details]
Patch

[Yusuke Suzuki]

Created attachment 251633 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 251579 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/StringConstructor.cpp:100
>>> +    for (unsigned i = 0; i < length; ++i) {
>> 
>> Would it be more efficient to preallocate a 2*length size buffer than using StringBuilder?
> 
> Or since it’s highly likely it will be length, not 2*length, should just call:
> 
>     builder.reserveCapacity(length);

Looks nice!
If there's no code points represented by surrogate pairs, a length of a generated string becomes `length`.
OK, so `builder.reserveCapacity(length);` looks very nice.

>> Source/JavaScriptCore/runtime/StringConstructor.cpp:101
>> +        double nextCP = exec->uncheckedArgument(i).toNumber(exec);
> 
> I suggest naming this codePointAsDouble rather than nextCP.

Looks nice. Fixed.

>> Source/JavaScriptCore/runtime/StringConstructor.cpp:109
>> +            return throwVMError(exec, createRangeError(exec, ASCIILiteral("Arguments contain a value that is out of range of code points")));
> 
> This conversion back to jsNumber is really strange. I think we can do this check without using the sameValue function and without all this conversion back and forth.
> 
> I think it’s probably simply this:
> 
>     UChar32 codePoint = codePointAsDouble;
>     if (codePoint != codePointAsDouble || codePoint > 0x10FFFF)
>         return throwVMError(exec, createRangeError(exec, ASCIILiteral("Arguments contain a value that is out of range of code points")));

Looks nice. However, since `UChar32` is typedef-ed as `int32_t`, when `-1` comes, `codePoint != codePointAsDouble` becomes false.
So instead of UChar32, I've changed it to `uint32_t` in the revised patch.

>> Source/JavaScriptCore/runtime/StringConstructor.cpp:111
>> +        unsigned codePoint = static_cast<unsigned>(nextCP);
> 
> Should be UChar32 rather than unsigned.

Sounds very nice! UChar32 is nice for representing unicode code points.

>> Source/JavaScriptCore/runtime/StringConstructor.cpp:119
>> +            builder.append(encodeBuffer[1]);
> 
> Here is a much cleaner way to write it:
> 
>     if (U_IS_BMP(codePoint))
>         builder.append(codePoint);
>     else {
>         builder.append(U16_LEAD(codePoint));
>         builder.append(U16_TRAIL(codePoint));
>     }

Right. Your suggestion is cleaner :D

[Build Bot]

Comment on attachment 251633 [details]
Patch

Attachment 251633 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5669308101296128

New failing tests:
js/Object-getOwnPropertyNames.html

[Build Bot]

Created attachment 251635 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 251633 [details]
Patch

Attachment 251633 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4974387224641536

New failing tests:
js/Object-getOwnPropertyNames.html

[Build Bot]

Created attachment 251638 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Yusuke Suzuki]

Created attachment 251639 [details]
Patch

[Darin Adler]

Comment on attachment 251639 [details]
Patch

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

Looks great.

Correctness-wise this seems just right. Optimization-wise, maybe worth revisiting after landing.

- We might consider a fast path in the extremely likely case that an argument is already an Int32; converting to double and back is expensive and unnecessary. The JSValue::toInt32 and toUInt32 functions does something like this, and while they are doing a different operation, the same principle applies. This one is really easy, we’d just check isUInt32 before calling toNumber and refactor so we don’t do the floating point arithmetic at all in that case.

- We might consider a fast path in the extremely likely case that there is a single BMP argument that should return a single character string, like the special case we already have for fromCharCode.

- If fromCharCode or fromCodePoint ever gets really hot usage then we could also consider optimizing either in the JIT the way we do with some other built-in functions like the math functions.

It’s unfortunate that toUInt32 has the unwanted behavior of doing modulo rather than failing when the value cannot be converted into a number that is in the UInt32 range. Because aside from that, toUInt32 has desirable behavior such as avoiding the "to double and back" thing.

> Source/JavaScriptCore/tests/stress/string-from-code-points.js:51
> +    [ 0x10000,  "\uD800\uDC00" ],
> +    [ 0x10001,  "\uD800\uDC01" ],

Should test that when you pass in an unpaired surrogate such as 0xD800 or 0xDC00 it just gets passed through unmodified, since that seems to be the specified behavior. Or even a pair of surrogates as code points that then turn into a single code point in the resulting string!

> Source/JavaScriptCore/tests/stress/string-from-code-points.js:59
> +var invalidTests = [

Should specifically test a value that works with fromCharCode because of toUInt32 but not for this class. Such as 2^32 + 32, which produces a space if passed to fromCharCode, but should throw an exception if passed to fromCodePoint.

> Source/JavaScriptCore/tests/stress/string-from-code-points.js:103
> +    [ null, "\0" ],
> +    [ [], "\0" ],
> +    [ "0x41", "A" ],
> +    [ "", "\0" ],

Should probably add tests for boolean true and false also.

[Darin Adler]

Comment on attachment 251639 [details]
Patch

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

>> Source/JavaScriptCore/tests/stress/string-from-code-points.js:51
>> +    [ 0x10001,  "\uD800\uDC01" ],
> 
> Should test that when you pass in an unpaired surrogate such as 0xD800 or 0xDC00 it just gets passed through unmodified, since that seems to be the specified behavior. Or even a pair of surrogates as code points that then turn into a single code point in the resulting string!

Besides unpaired surrogates, there are these values that are technically “non-characters” but that we intentionally handle rather than rejecting:

- the last two code points on each plane (U+__FFFE and U+__FFFF, 34 code points total in 17 planes); of these we test only 0x10FFFF, 0x10FFFE and 0xFFFF here but we could test some others
- U+FDD0-U+FDEF (32 code points)

We might want to test a few more of them, at least the true edge cases. If we had used, for example, the U_IS_UNICODE_CHAR function from the ICU library rather than comparing against 0x10FFFF explicitly we would have rejected those characters.

[Yusuke Suzuki]

Comment on attachment 251639 [details]
Patch

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

Thank you for your review!
Your thought is correct. I think if we need more performance for this area, implementing a fast path in JIT layer (implementing intrinsic thunk code for JS function) is preferable.

>>> Source/JavaScriptCore/tests/stress/string-from-code-points.js:51
>>> +    [ 0x10001,  "\uD800\uDC01" ],
>> 
>> Should test that when you pass in an unpaired surrogate such as 0xD800 or 0xDC00 it just gets passed through unmodified, since that seems to be the specified behavior. Or even a pair of surrogates as code points that then turn into a single code point in the resulting string!
> 
> Besides unpaired surrogates, there are these values that are technically “non-characters” but that we intentionally handle rather than rejecting:
> 
> - the last two code points on each plane (U+__FFFE and U+__FFFF, 34 code points total in 17 planes); of these we test only 0x10FFFF, 0x10FFFE and 0xFFFF here but we could test some others
> - U+FDD0-U+FDEF (32 code points)
> 
> We might want to test a few more of them, at least the true edge cases. If we had used, for example, the U_IS_UNICODE_CHAR function from the ICU library rather than comparing against 0x10FFFF explicitly we would have rejected those characters.

Looks nice.
I've added
1. unpaired surrogates
2. all non-characters code points (17 * 2 + 32 = 66)

>> Source/JavaScriptCore/tests/stress/string-from-code-points.js:59
>> +var invalidTests = [
> 
> Should specifically test a value that works with fromCharCode because of toUInt32 but not for this class. Such as 2^32 + 32, which produces a space if passed to fromCharCode, but should throw an exception if passed to fromCodePoint.

Added it (0x100000000 + 32).

>> Source/JavaScriptCore/tests/stress/string-from-code-points.js:103
>> +    [ "", "\0" ],
> 
> Should probably add tests for boolean true and false also.

Sounds nice. Added.

[Yusuke Suzuki]

Committed r183315: <http://trac.webkit.org/changeset/183315>