Bug 185379 - [ESNext][BigInt] Implement support for "<" and ">" relational operation
Summary: [ESNext][BigInt] Implement support for "<" and ">" relational 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: 184474
Blocks: 179001 185929
  Show dependency treegraph
 
Reported: 2018-05-07 08:35 PDT by Caio Lima
Modified: 2018-05-29 14:18 PDT (History)
12 users (show)

See Also:


Attachments
WIP - Patch (22.03 KB, patch)
2018-05-07 08:39 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
WIP - Patch (28.83 KB, patch)
2018-05-08 04:50 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (30.23 KB, patch)
2018-05-14 08:16 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (30.48 KB, patch)
2018-05-14 20:19 PDT, Caio Lima
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.75 MB, application/zip)
2018-05-15 21:45 PDT, Build Bot
no flags Details
Patch (29.15 KB, patch)
2018-05-19 19:58 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (29.74 KB, patch)
2018-05-19 22:47 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (35.84 KB, patch)
2018-05-20 09:22 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (35.95 KB, patch)
2018-05-20 16:13 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (35.95 KB, patch)
2018-05-20 20:31 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (35.36 KB, patch)
2018-05-23 20:34 PDT, Caio Lima
utatane.tea: review+
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.73 MB, application/zip)
2018-05-24 00:25 PDT, Build Bot
no flags Details
Patch for Landing (35.26 KB, patch)
2018-05-29 12:15 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Benchmark Report (93.82 KB, text/plain)
2018-05-29 13:28 PDT, Caio Lima
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Lima 2018-05-07 08:35:15 PDT
...
Comment 1 Caio Lima 2018-05-07 08:39:06 PDT
Created attachment 339724 [details]
WIP - Patch

This Patch is implementing the basic idea of this Patch, but it is not Spec compliant yet.
Comment 2 Caio Lima 2018-05-08 04:50:53 PDT
Created attachment 339808 [details]
WIP - Patch

Now Patch is spec compliant, but it depends on https://bugs.webkit.org/show_bug.cgi?id=184474 to compile.
Comment 3 Caio Lima 2018-05-14 08:16:57 PDT
Created attachment 340317 [details]
Patch
Comment 4 Caio Lima 2018-05-14 08:19:00 PDT
(In reply to Caio Lima from comment #3)
> Created attachment 340317 [details]
> Patch

Performance comparison is coming soon.
Comment 5 Caio Lima 2018-05-14 20:19:07 PDT
Created attachment 340392 [details]
Patch
Comment 6 Build Bot 2018-05-15 21:44:56 PDT
Comment on attachment 340392 [details]
Patch

Attachment 340392 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7694760

New failing tests:
http/tests/security/canvas-remote-read-remote-video-redirect.html
Comment 7 Build Bot 2018-05-15 21:45:08 PDT
Created attachment 340467 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 8 Robin Morisset 2018-05-17 08:23:18 PDT
Comment on attachment 340392 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSBigInt.cpp:526
> +JSBigInt::ComparisonResult JSBigInt::unequalSign(bool leftNegative)

I am not convinced that unequalSign, absoluteGreater and absoluteLess are useful to have as methods. Each is a single expression that fits on a single line, is only used once by JSBigInt::compare, does not really make sense outside of that context, and don't have a name that make their semantics especially clear without reading their code. I would just inline them in compare.

> Source/JavaScriptCore/runtime/JSBigInt.h:87
> +    enum ComparisonResult {

We usually prefer "enum class" to "enum", as it prevents accidental type conversions.

> JSTests/stress/big-int-less-than-general.js:72
> +

I would probably add a test or two where a BigInt is compared to a string that does not represent a number (which should return undefined if I read 4.m.ii and 4.n.ii correctly in https://tc39.github.io/proposal-bigint/#sec-abstract-relational-comparison)

> JSTests/stress/big-int-less-than-general.js:111
> +assert(0n < NaN, false, "0n < NaN");

This surprises me: point 4.s. from https://tc39.github.io/proposal-bigint/#sec-abstract-relational-comparison seems to suggest undefined as a result.
Comment 9 Caio Lima 2018-05-19 19:53:17 PDT
Comment on attachment 340392 [details]
Patch

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

Thank you for the review and sorry for the late reply here.

>> Source/JavaScriptCore/runtime/JSBigInt.h:87
>> +    enum ComparisonResult {
> 
> We usually prefer "enum class" to "enum", as it prevents accidental type conversions.

Oops. Changed.

>> JSTests/stress/big-int-less-than-general.js:72
>> +
> 
> I would probably add a test or two where a BigInt is compared to a string that does not represent a number (which should return undefined if I read 4.m.ii and 4.n.ii correctly in https://tc39.github.io/proposal-bigint/#sec-abstract-relational-comparison)

You are right. I'm going to add them. Comparison operations just return booleans. Take a look at https://tc39.github.io/ecma262/#sec-relational-operators-runtime-semantics-evaluation.

>> JSTests/stress/big-int-less-than-general.js:111
>> +assert(0n < NaN, false, "0n < NaN");
> 
> This surprises me: point 4.s. from https://tc39.github.io/proposal-bigint/#sec-abstract-relational-comparison seems to suggest undefined as a result.

It is a good observation! However comparison operators only returns booleans. In the case of Abstract Relational Comparison returns undefined, the "<" operation returns "false". https://tc39.github.io/ecma262/#sec-relational-operators-runtime-semantics-evaluation step 7.
Comment 10 Caio Lima 2018-05-19 19:58:03 PDT
Created attachment 340791 [details]
Patch
Comment 11 Build Bot 2018-05-19 21:18:24 PDT
Comment on attachment 340791 [details]
Patch

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

New failing tests:
stress/big-int-greater-than-general.js.big-int-enabled
stress/big-int-greater-than-wrapped-values.js.big-int-enabled
stress/big-int-less-than-jit.js.big-int-enabled
stress/big-int-less-than-order-of-evaluation.js.big-int-enabled
stress/big-int-less-than-wrapped-values.js.big-int-enabled
stress/big-int-less-than-general.js.big-int-enabled
Comment 12 Caio Lima 2018-05-19 22:47:32 PDT
Created attachment 340792 [details]
Patch
Comment 13 Build Bot 2018-05-19 22:49:35 PDT
Attachment 340792 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/Operations.h:181:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:583:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Caio Lima 2018-05-20 09:22:09 PDT
Created attachment 340794 [details]
Patch
Comment 15 Build Bot 2018-05-20 10:40:50 PDT
Comment on attachment 340794 [details]
Patch

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

New failing tests:
stress/big-int-greater-than-general.js.big-int-enabled
stress/big-int-less-than-general.js.big-int-enabled
Comment 16 Caio Lima 2018-05-20 16:13:41 PDT
Created attachment 340809 [details]
Patch
Comment 17 Build Bot 2018-05-20 17:34:09 PDT
Comment on attachment 340809 [details]
Patch

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

New failing tests:
stress/big-int-greater-than-general.js.big-int-enabled
Comment 18 Caio Lima 2018-05-20 20:31:17 PDT
Created attachment 340818 [details]
Patch
Comment 19 Robin Morisset 2018-05-22 03:21:12 PDT
> >> JSTests/stress/big-int-less-than-general.js:111
> >> +assert(0n < NaN, false, "0n < NaN");
> > 
> > This surprises me: point 4.s. from https://tc39.github.io/proposal-bigint/#sec-abstract-relational-comparison seems to suggest undefined as a result.
> 
> It is a good observation! However comparison operators only returns
> booleans. In the case of Abstract Relational Comparison returns undefined,
> the "<" operation returns "false".
> https://tc39.github.io/ecma262/#sec-relational-operators-runtime-semantics-
> evaluation step 7.

Thanks, I had completely missed that part.
Comment 20 Caio Lima 2018-05-23 20:34:25 PDT
Created attachment 341164 [details]
Patch
Comment 21 Build Bot 2018-05-24 00:24:56 PDT
Comment on attachment 341164 [details]
Patch

Attachment 341164 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7785476

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html
Comment 22 Build Bot 2018-05-24 00:25:07 PDT
Created attachment 341178 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 23 Yusuke Suzuki 2018-05-29 10:59:41 PDT
Comment on attachment 341164 [details]
Patch

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

r=me with nit.

> Source/JavaScriptCore/runtime/Operations.h:262
> +            bool result = bigIntCompareLess(callFrame, p1, p2);
> +            RETURN_IF_EXCEPTION(scope, false);
> +            return result;

Use,

scope.release();
return bigIntCompareLess(callFrame, p1, p2);
Comment 24 Caio Lima 2018-05-29 12:15:43 PDT
Created attachment 341504 [details]
Patch for Landing
Comment 25 Caio Lima 2018-05-29 12:21:12 PDT
Thank you for the review!
Comment 26 Caio Lima 2018-05-29 13:28:49 PDT
Created attachment 341508 [details]
Benchmark Report

This patch is perf neutral
Comment 27 WebKit Commit Bot 2018-05-29 14:15:30 PDT
Comment on attachment 341504 [details]
Patch for Landing

Clearing flags on attachment: 341504

Committed r232273: <https://trac.webkit.org/changeset/232273>
Comment 28 WebKit Commit Bot 2018-05-29 14:15:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 Radar WebKit Bug Importer 2018-05-29 14:18:03 PDT
<rdar://problem/40628352>