Bug 184474 – [ESNext][BigInt] Implement support for "==" operation

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

[Caio Lima]

...

[Caio Lima]

Created attachment 338542 [details]
WIP - Patch

It starts

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:847:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:851:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:852:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:877:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:879:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:908:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:918:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:919:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:924:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:977:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 10 in 4 files


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

[EWS Watchlist]

Comment on attachment 338542 [details]
WIP - Patch

Attachment 338542 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/7400116

New failing tests:
stress/big-int-equals-basic.js.big-int-enabled

[Caio Lima]

Created attachment 338556 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:923:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:924:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 8 files


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

[Saam Barati]

Comment on attachment 338556 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSBigInt.cpp:857
> +        return (this->length() == 1) && (this->sign() == (value < 0)) && (this->digit(0) == static_cast<Digit>(std::abs(static_cast<int64_t>(value))));

This works for INT32_MIN, right?

> Source/JavaScriptCore/runtime/JSBigInt.cpp:874
> +    if (std::isnan(y))
> +        return ComparisonResult::Undefined;
> +
> +    if (y == std::numeric_limits<double>::infinity())
> +        return ComparisonResult::LessThan;
> +
> +    if (y == -std::numeric_limits<double>::infinity())
> +        return ComparisonResult::GreaterThan;

I think there is a way to check if it's any of these three in a single branch, check out some of the JIT code I emit here:
https://bugs.webkit.org/attachment.cgi?id=334549&action=review

We should see if the compiler is smart enough to make this a single branch followed by individual compares. If not, maybe the C++ stdlib has some function that does similarly.

Since you're grabbing the raw exponent below, you may as well make it a single branch.

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:984
> +        if (v1.isBigInt() && s2) {

== for bigint really does type conversions? That's really unfortunate...

[Caio Lima]

Comment on attachment 338556 [details]
Patch

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

Thank you very much for the review.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:857
>> +        return (this->length() == 1) && (this->sign() == (value < 0)) && (this->digit(0) == static_cast<Digit>(std::abs(static_cast<int64_t>(value))));
> 
> This works for INT32_MIN, right?

I think so. I'm going to add a test case for that.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:874
>> +        return ComparisonResult::GreaterThan;
> 
> I think there is a way to check if it's any of these three in a single branch, check out some of the JIT code I emit here:
> https://bugs.webkit.org/attachment.cgi?id=334549&action=review
> 
> We should see if the compiler is smart enough to make this a single branch followed by individual compares. If not, maybe the C++ stdlib has some function that does similarly.
> 
> Since you're grabbing the raw exponent below, you may as well make it a single branch.

Cool. I'm going to update that.

>> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:984
>> +        if (v1.isBigInt() && s2) {
> 
> == for bigint really does type conversions? That's really unfortunate...

Yes. Also relational operations "<".

[Caio Lima]

Created attachment 339379 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:923:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:924:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 8 files


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

[Caio Lima]

Created attachment 339647 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:922:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:923:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 8 files


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

[Caio Lima]

Ping Review

[Yusuke Suzuki]

Comment on attachment 339647 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/JSBigInt.cpp:867
> +    uint64_t doubleBits = *(reinterpret_cast<uint64_t*>(&y));

Let's do `uint64_t doubleBits = bitwise_cast<uint64_t>(y);`.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:886
> +    if (!y)
> +        return x->isZero() ? ComparisonResult::Equal : ComparisonResult::GreaterThan;

Adding `ASSERT(!xSign)` inside this `if` helps readers.

[Caio Lima]

Comment on attachment 339647 [details]
Patch

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

Thx for the review!

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:867
>> +    uint64_t doubleBits = *(reinterpret_cast<uint64_t*>(&y));
> 
> Let's do `uint64_t doubleBits = bitwise_cast<uint64_t>(y);`.

Good to know that! I'm just worried about the memcpy of bitwise_cast, but I think it is ok.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:886
>> +        return x->isZero() ? ComparisonResult::Equal : ComparisonResult::GreaterThan;
> 
> Adding `ASSERT(!xSign)` inside this `if` helps readers.

Done.

[Caio Lima]

Created attachment 340066 [details]
Patch for landing

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:905:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:906:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 8 files


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

[Yusuke Suzuki]

Comment on attachment 339647 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/JSBigInt.cpp:867
>>> +    uint64_t doubleBits = *(reinterpret_cast<uint64_t*>(&y));
>> 
>> Let's do `uint64_t doubleBits = bitwise_cast<uint64_t>(y);`.
> 
> Good to know that! I'm just worried about the memcpy of bitwise_cast, but I think it is ok.

Compilers have optimizations for memcpy, so it produces simple mov related instructions inlinely.

[WebKit Commit Bot]

Comment on attachment 340066 [details]
Patch for landing

Clearing flags on attachment: 340066

Committed r231629: <https://trac.webkit.org/changeset/231629>

[Radar WebKit Bug Importer]

<rdar://problem/40201833>