Bug 183721 - [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: 185112 185118
Blocks: 179001
  Show dependency treegraph
 
Reported: 2018-03-17 13:14 PDT by Caio Lima
Modified: 2018-05-31 19:17 PDT (History)
13 users (show)

See Also:


Attachments
WIP - Patch (11.26 KB, patch)
2018-03-17 15:37 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (29.21 KB, patch)
2018-03-18 11:49 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (43.66 KB, patch)
2018-04-01 04:00 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (44.26 KB, patch)
2018-04-17 07:36 PDT, Caio Lima
saam: review+
Details | Formatted Diff | Diff
Patch for landing (43.28 KB, patch)
2018-04-26 20:30 PDT, Caio Lima
ticaiolima: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (43.27 KB, patch)
2018-04-26 20:39 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (44.23 KB, patch)
2018-04-27 12:46 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (44.34 KB, patch)
2018-04-28 09:57 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (46.61 KB, patch)
2018-05-01 12:43 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Benchmark (88.13 KB, text/plain)
2018-05-01 13:12 PDT, Caio Lima
no flags Details
Patch (46.77 KB, patch)
2018-05-03 09:17 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (46.30 KB, patch)
2018-05-04 21:46 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (44.31 KB, patch)
2018-05-10 19:31 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (43.93 KB, patch)
2018-05-10 19:53 PDT, Caio Lima
ysuzuki: review+
Details | Formatted Diff | Diff
Patch for landing (42.69 KB, patch)
2018-05-11 20:15 PDT, Caio Lima
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (42.68 KB, patch)
2018-05-11 20:53 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-03-17 13:14:08 PDT
Support BigInts into times operation.
Comment 1 Caio Lima 2018-03-17 15:37:14 PDT
Created attachment 336016 [details]
WIP - Patch

It starts. Need to write tests and ChangeLog yet.
Comment 2 EWS Watchlist 2018-03-17 15:39:21 PDT
Attachment 336016 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:264:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:267:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:489:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:490:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:490:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:491:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:492:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:493:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:494:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:495:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:511:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:513:  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:513:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:522:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 14 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Caio Lima 2018-03-18 11:49:00 PDT
Created attachment 336027 [details]
Patch
Comment 4 Caio Lima 2018-03-23 00:59:28 PDT
Ping Review
Comment 5 Saam Barati 2018-03-25 22:02:45 PDT
Comment on attachment 336027 [details]
Patch

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

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:494
> +    if (UNLIKELY(throwScope.exception()))
> +        RETURN(JSValue());

CHECK_EXCEPTION() here

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:504
> +        THROW(createTypeError(exec, "Invalid operand in BigInt operation."));

Let's have this be more descriptive. You know this is a multiply. Also, is this really a type error? There is no automatic conversion to BigInt?

Like, if I want to multiply a BigInt by 2, I must write "* 2n"?

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:509
> +    if (UNLIKELY(throwScope.exception()))
> +        RETURN(JSValue());

CHECK_EXCEPTION()

> Source/JavaScriptCore/runtime/JSBigInt.cpp:258
> +    int resultLength = x->length() + y->length();

Why int here? Should definitely be using unsigned or size_t.
Also, are we worried about overflow here?

> Source/JavaScriptCore/runtime/JSBigInt.cpp:262
> +    for (int i = 0; i < x->length(); i++)

ditto for type.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:493
> +    for (int i = 0; i < multiplicand->length(); i++, accumulatorIndex++) {

ditto for type. I see we're using int everywhere instead of unsigned/size_t. What's the reason for that?
Comment 6 Caio Lima 2018-03-30 02:52:31 PDT
Comment on attachment 336027 [details]
Patch

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

Thank you for the review

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:494
>> +        RETURN(JSValue());
> 
> CHECK_EXCEPTION() here

ok

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:504
>> +        THROW(createTypeError(exec, "Invalid operand in BigInt operation."));
> 
> Let's have this be more descriptive. You know this is a multiply. Also, is this really a type error? There is no automatic conversion to BigInt?
> 
> Like, if I want to multiply a BigInt by 2, I must write "* 2n"?

Yes. According to current spec, we should throw if type of operands doesn't match https://tc39.github.io/proposal-bigint/#sec-multiplicative-operators-runtime-semantics-evaluation

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:258
>> +    int resultLength = x->length() + y->length();
> 
> Why int here? Should definitely be using unsigned or size_t.
> Also, are we worried about overflow here?

It is from the original implementation in V8. AFIK, we could always use unsigned or size_t with no problems.
About overflow, I don't think we should be worried because current implementation max length is no greater than 2 ** 11 (https://github.com/caiolima/webkit/blob/master/Source/JavaScriptCore/runtime/JSBigInt.h#L100)

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:493
>> +    for (int i = 0; i < multiplicand->length(); i++, accumulatorIndex++) {
> 
> ditto for type. I see we're using int everywhere instead of unsigned/size_t. What's the reason for that?

Ditto.
Comment 7 Caio Lima 2018-04-01 04:00:26 PDT
Created attachment 336955 [details]
Patch
Comment 8 Caio Lima 2018-04-02 09:57:57 PDT
Ping Review
Comment 9 Caio Lima 2018-04-02 23:58:42 PDT
Png review
Comment 10 Caio Lima 2018-04-08 04:58:01 PDT
Ping Review
Comment 11 Caio Lima 2018-04-17 07:36:02 PDT
Created attachment 338110 [details]
Patch
Comment 12 Caio Lima 2018-04-19 15:40:17 PDT
Ping Review
Comment 13 Caio Lima 2018-04-24 19:24:44 PDT
Ping Review
Comment 14 Saam Barati 2018-04-26 15:31:12 PDT
Comment on attachment 338110 [details]
Patch

r=me
Comment 15 Caio Lima 2018-04-26 20:30:00 PDT
Created attachment 338959 [details]
Patch for landing
Comment 16 Caio Lima 2018-04-26 20:39:13 PDT
Created attachment 338960 [details]
Patch for landing
Comment 17 WebKit Commit Bot 2018-04-26 21:18:38 PDT
Comment on attachment 338960 [details]
Patch for landing

Clearing flags on attachment: 338960

Committed r231086: <https://trac.webkit.org/changeset/231086>
Comment 18 WebKit Commit Bot 2018-04-26 21:18:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Radar WebKit Bug Importer 2018-04-26 21:19:19 PDT
<rdar://problem/39781587>
Comment 20 Ryan Haddad 2018-04-27 09:11:56 PDT
This change caused 100 failures on the JSC debug bot due to unchecked exceptions:

stress/untyped-mul.js.default: ERROR: Unchecked JS exception:
stress/untyped-mul.js.default:     This scope can throw a JS exception: toNumber @ ./runtime/JSString.cpp:418
stress/untyped-mul.js.default:         (ExceptionScope::m_recursionDepth was 5)
stress/untyped-mul.js.default:     But the exception was unchecked as of this scope: jsMul @ /Volumes/Data/slave/highsierra-debug/build/Source/JavaScriptCore/runtime/Operations.h:263
stress/untyped-mul.js.default:         (ExceptionScope::m_recursionDepth was 4)
stress/untyped-mul.js.default: 
stress/untyped-mul.js.default: Unchecked exception detected at:
stress/untyped-mul.js.default:     1   0x110047623 JSC::VM::verifyExceptionCheckNeedIsSatisfied(unsigned int, JSC::ExceptionEventLocation&)
stress/untyped-mul.js.default:     2   0x11002210b JSC::ThrowScope::~ThrowScope()
stress/untyped-mul.js.default:     3   0x1100224e5 JSC::ThrowScope::~ThrowScope()
stress/untyped-mul.js.default:     4   0x10fb8a997 JSC::jsMul(JSC::ExecState*, JSC::JSValue, JSC::JSValue)
stress/untyped-mul.js.default:     5   0x10fb79d06 JSC::profiledMul(JSC::ExecState*, long long, long long, JSC::ArithProfile&, bool)
stress/untyped-mul.js.default:     6   0x10fb79c75 operationValueMulProfiled
stress/untyped-mul.js.default:     7   0x5ab6aefac79
stress/untyped-mul.js.default:     8   0x10ecf1ca1 llint_entry
stress/untyped-mul.js.default:     9   0x10ece9552 vmEntryToJavaScript
stress/untyped-mul.js.default:     10  0x10fb58e3a JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
stress/untyped-mul.js.default:     11  0x10faf8b13 JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*)
stress/untyped-mul.js.default:     12  0x10fdc2857 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
stress/untyped-mul.js.default:     13  0x10eb3cf49 runWithOptions(GlobalObject*, CommandLine&, bool&)
stress/untyped-mul.js.default:     14  0x10eb14fac jscmain(int, char**)::$_3::operator()(JSC::VM&, GlobalObject*, bool&) const
stress/untyped-mul.js.default:     15  0x10eafccc4 int runJSC<jscmain(int, char**)::$_3>(CommandLine, bool, jscmain(int, char**)::$_3 const&)
stress/untyped-mul.js.default:     16  0x10eafb7af jscmain(int, char**)
stress/untyped-mul.js.default:     17  0x10eafb70e main
stress/untyped-mul.js.default:     18  0x7fff6e830015 start

https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/939
Comment 21 Ryan Haddad 2018-04-27 10:22:52 PDT
Reverted r231086 for reason:

Caused JSC test failures due to an unchecked exception.

Committed r231104: <https://trac.webkit.org/changeset/231104>
Comment 22 Caio Lima 2018-04-27 12:46:50 PDT
Created attachment 339014 [details]
Patch
Comment 23 WebKit Commit Bot 2018-04-27 23:12:03 PDT
Comment on attachment 339014 [details]
Patch

Clearing flags on attachment: 339014

Committed r231131: <https://trac.webkit.org/changeset/231131>
Comment 24 WebKit Commit Bot 2018-04-27 23:12:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 WebKit Commit Bot 2018-04-28 07:44:49 PDT
Re-opened since this is blocked by bug 185112
Comment 26 Caio Lima 2018-04-28 09:57:28 PDT
Created attachment 339068 [details]
Patch
Comment 27 WebKit Commit Bot 2018-04-28 10:37:26 PDT
Comment on attachment 339068 [details]
Patch

Clearing flags on attachment: 339068

Committed r231137: <https://trac.webkit.org/changeset/231137>
Comment 28 WebKit Commit Bot 2018-04-28 10:37:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 WebKit Commit Bot 2018-04-29 06:30:20 PDT
Re-opened since this is blocked by bug 185118
Comment 30 Caio Lima 2018-05-01 12:43:38 PDT
Created attachment 339214 [details]
Patch
Comment 31 Caio Lima 2018-05-01 13:12:48 PDT
Created attachment 339218 [details]
Benchmark

This new Path is perf neutral.
Comment 32 Caio Lima 2018-05-02 19:38:40 PDT
Ping review?
Comment 33 Caio Lima 2018-05-03 09:17:35 PDT
Created attachment 339413 [details]
Patch
Comment 34 Caio Lima 2018-05-03 19:43:42 PDT
Hey Saam, could you take a new look at it? I added toNumeric to JSValue to make this change Spec compliant.

https://tc39.github.io/proposal-bigint/#sec-tonumeric
Comment 35 Saam Barati 2018-05-04 10:55:14 PDT
Comment on attachment 339413 [details]
Patch

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

> Source/JavaScriptCore/jit/JITOperations.cpp:2573
> +    JSValue result = jsMul(exec, op1, op2);

You're missing an exception after this.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:504
> +    Variant<JSBigInt*, double> leftNumeric = left.toNumeric(exec);
> +    CHECK_EXCEPTION();
> +    Variant<JSBigInt*, double> rightNumeric = right.toNumeric(exec);
> +    CHECK_EXCEPTION();
> +    
> +    if (WTF::holds_alternative<JSBigInt*>(leftNumeric) || WTF::holds_alternative<JSBigInt*>(rightNumeric)) {
> +        if (WTF::holds_alternative<JSBigInt*>(leftNumeric) && WTF::holds_alternative<JSBigInt*>(rightNumeric)) {
> +            JSValue result(JSBigInt::multiply(exec, WTF::get<JSBigInt*>(leftNumeric), WTF::get<JSBigInt*>(rightNumeric)));
> +            RETURN_WITH_PROFILING(result, {
> +                updateArithProfileForBinaryArithOp(exec, pc, result, left, right);
> +            });
> +        }
> +
> +        THROW(createTypeError(exec, "Invalid mix of BigInt and other type in multiplication."));
> +    }

Why isn't this just calling jsMul?

> Source/JavaScriptCore/runtime/JSCJSValue.h:262
> +    Variant<JSBigInt*, double> toNumeric(ExecState*) const;

This seems like a reasonable interface. I wonder if we care about making it faster or not since users of this branch on type.

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:742
> +    VM& vm = exec->vm();
> +    auto scope = DECLARE_THROW_SCOPE(vm);
> +    JSValue primValue = this->toPrimitive(exec, PreferNumber);
> +    RETURN_IF_EXCEPTION(scope, 0);
> +    if (primValue.isBigInt())
> +        return asBigInt(primValue);
> +    double value = primValue.toNumber(exec);
> +    RETURN_IF_EXCEPTION(scope, 0);
> +    return value;

You should probably have fast paths for the type being int/double/bigint before calling toPrimitive.
Comment 36 Saam Barati 2018-05-04 10:55:38 PDT
Comment on attachment 339413 [details]
Patch

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

>> Source/JavaScriptCore/jit/JITOperations.cpp:2573
>> +    JSValue result = jsMul(exec, op1, op2);
> 
> You're missing an exception after this.

exception check*
Comment 37 Caio Lima 2018-05-04 21:46:55 PDT
Created attachment 339634 [details]
Patch
Comment 38 Caio Lima 2018-05-08 04:51:13 PDT
Ping Review
Comment 39 Caio Lima 2018-05-10 19:31:29 PDT
Created attachment 340159 [details]
Patch
Comment 40 Caio Lima 2018-05-10 19:53:53 PDT
Created attachment 340160 [details]
Patch
Comment 41 Yusuke Suzuki 2018-05-11 00:15:36 PDT
Comment on attachment 340160 [details]
Patch

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

r=me

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:150
>                      // FIXME: Revisit this condition when introducing BigInt to JSC.

Remove this FIXME. And add a test for this case (in particular, we need to loop things to get compiled in DFG).

> Source/JavaScriptCore/jit/JITOperations.cpp:2560
> +    VM& vm = exec->vm();
>      auto scope = DECLARE_THROW_SCOPE(vm);

Remove this.

> Source/JavaScriptCore/jit/JITOperations.cpp:2564
> +    JSValue jsResult = jsMul(exec, op1, op2);

Just return `JSValue::encode(jsMul(exec, op1, op2))`.

> Source/JavaScriptCore/jit/JITOperations.cpp:2565
>      RETURN_IF_EXCEPTION(scope, encodedJSValue());

Since `jsMul()` just returns the value, we do not need to have scope and exception checks.

> Source/JavaScriptCore/jit/JITOperations.cpp:2580
>      RETURN_IF_EXCEPTION(scope, encodedJSValue());

On the other hand, this profiledMul uses arithProfile after `jsMul`. So this function should keep scope and exception checks.`
Comment 42 Caio Lima 2018-05-11 19:47:24 PDT
Comment on attachment 340160 [details]
Patch

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

Thx for the review!

>> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:150
>>                      // FIXME: Revisit this condition when introducing BigInt to JSC.
> 
> Remove this FIXME. And add a test for this case (in particular, we need to loop things to get compiled in DFG).

```JSTests/stress/big-int-mul-jit.js``` is already testing this.
Comment 43 Caio Lima 2018-05-11 20:15:25 PDT
Created attachment 340240 [details]
Patch for landing
Comment 44 WebKit Commit Bot 2018-05-11 20:17:58 PDT
Comment on attachment 340240 [details]
Patch for landing

Rejecting attachment 340240 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 340240, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

NOBODY Yusuke Suzuki found in /Volumes/Data/EWS/WebKit/JSTests/ChangeLog does not appear to be a valid reviewer according to contributors.json.
/Volumes/Data/EWS/WebKit/JSTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.webkit.org/results/7657434
Comment 45 Yusuke Suzuki 2018-05-11 20:43:27 PDT
(In reply to WebKit Commit Bot from comment #44)
> Comment on attachment 340240 [details]
> Patch for landing
> 
> Rejecting attachment 340240 [details] from commit-queue.
> 
> Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch',
> '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01',
> 'validate-changelog', '--check-oops', '--non-interactive', 340240,
> '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
> 
> NOBODY Yusuke Suzuki found in /Volumes/Data/EWS/WebKit/JSTests/ChangeLog
> does not appear to be a valid reviewer according to contributors.json.
> /Volumes/Data/EWS/WebKit/JSTests/ChangeLog neither lists a valid reviewer
> nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
> 
> Full output: http://webkit-queues.webkit.org/results/7657434

NOBODY remains.
Comment 46 Caio Lima 2018-05-11 20:53:10 PDT
Created attachment 340242 [details]
Patch for landing
Comment 47 WebKit Commit Bot 2018-05-11 21:32:18 PDT
Comment on attachment 340242 [details]
Patch for landing

Clearing flags on attachment: 340242

Committed r231733: <https://trac.webkit.org/changeset/231733>
Comment 48 WebKit Commit Bot 2018-05-11 21:32:20 PDT
All reviewed patches have been landed.  Closing bug.
Comment 49 Yusuke Suzuki 2018-05-31 18:52:31 PDT
Remember ArithMul can return number only. We need to add ValueMul too in DFG.
Comment 50 Caio Lima 2018-05-31 19:17:09 PDT
(In reply to Yusuke Suzuki from comment #49)
> Remember ArithMul can return number only. We need to add ValueMul too in DFG.

Yes. I created umbrella bug https://bugs.webkit.org/show_bug.cgi?id=186173 to keep track of each node we need to add Value<op>. Probably it is also going to be true for unary operators.