Bug 184474 - [ESNext][BigInt] Implement support for "==" operation
Summary: [ESNext][BigInt] Implement support for "==" operation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caio Lima
URL:
Keywords: InRadar
Depends on:
Blocks: 179001 179901 185379
  Show dependency treegraph
 
Reported: 2018-04-10 16:14 PDT by Caio Lima
Modified: 2018-05-13 08:38 PDT (History)
11 users (show)

See Also:


Attachments
WIP - Patch (12.23 KB, patch)
2018-04-21 18:14 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (24.17 KB, patch)
2018-04-22 10:49 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (25.57 KB, patch)
2018-05-02 19:33 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (25.57 KB, patch)
2018-05-05 08:15 PDT, Caio Lima
yusukesuzuki: review+
Details | Formatted Diff | Diff
Patch for landing (25.03 KB, patch)
2018-05-09 20:25 PDT, Caio Lima
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Lima 2018-04-10 16:14:48 PDT
...
Comment 1 Caio Lima 2018-04-21 18:14:15 PDT
Created attachment 338542 [details]
WIP - Patch

It starts
Comment 2 Build Bot 2018-04-21 18:17:24 PDT
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.
Comment 3 Build Bot 2018-04-21 19:31:26 PDT
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
Comment 4 Caio Lima 2018-04-22 10:49:02 PDT
Created attachment 338556 [details]
Patch
Comment 5 Build Bot 2018-04-22 10:51:51 PDT
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.
Comment 6 Saam Barati 2018-04-26 15:48:23 PDT
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...
Comment 7 Caio Lima 2018-04-26 19:10:24 PDT
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 "<".
Comment 8 Caio Lima 2018-05-02 19:33:39 PDT
Created attachment 339379 [details]
Patch
Comment 9 Build Bot 2018-05-02 19:34:52 PDT
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.
Comment 10 Caio Lima 2018-05-05 08:15:06 PDT
Created attachment 339647 [details]
Patch
Comment 11 Build Bot 2018-05-05 08:17:35 PDT
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.
Comment 12 Caio Lima 2018-05-07 17:32:40 PDT
Ping Review
Comment 13 Yusuke Suzuki 2018-05-09 07:49:02 PDT
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.
Comment 14 Caio Lima 2018-05-09 20:03:50 PDT
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.
Comment 15 Caio Lima 2018-05-09 20:25:27 PDT
Created attachment 340066 [details]
Patch for landing
Comment 16 Build Bot 2018-05-09 20:28:02 PDT
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.
Comment 17 Yusuke Suzuki 2018-05-09 20:59:24 PDT
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.
Comment 18 WebKit Commit Bot 2018-05-09 21:25:54 PDT
Comment on attachment 340066 [details]
Patch for landing

Clearing flags on attachment: 340066

Committed r231629: <https://trac.webkit.org/changeset/231629>
Comment 19 Radar WebKit Bug Importer 2018-05-13 08:38:15 PDT
<rdar://problem/40201833>