Bug 183721 – [ESNext][BigInt] Implement support for "*" operation

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

[Caio Lima]

Support BigInts into times operation.

[Caio Lima]

Created attachment 336016 [details]
WIP - Patch

It starts. Need to write tests and ChangeLog yet.

[EWS Watchlist]

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.

[Caio Lima]

Created attachment 336027 [details]
Patch

[Caio Lima]

Ping Review

[Saam Barati]

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?

[Caio Lima]

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.

[Caio Lima]

Created attachment 336955 [details]
Patch

[Caio Lima]

Ping Review

[Caio Lima]

Png review

[Caio Lima]

Ping Review

[Caio Lima]

Created attachment 338110 [details]
Patch

[Caio Lima]

Ping Review

[Caio Lima]

Ping Review

[Saam Barati]

Comment on attachment 338110 [details]
Patch

r=me

[Caio Lima]

Created attachment 338959 [details]
Patch for landing

[Caio Lima]

Created attachment 338960 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 338960 [details]
Patch for landing

Clearing flags on attachment: 338960

Committed r231086: <https://trac.webkit.org/changeset/231086>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39781587>

[Ryan Haddad]

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

[Ryan Haddad]

Reverted r231086 for reason:

Caused JSC test failures due to an unchecked exception.

Committed r231104: <https://trac.webkit.org/changeset/231104>

[Caio Lima]

Created attachment 339014 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 339014 [details]
Patch

Clearing flags on attachment: 339014

Committed r231131: <https://trac.webkit.org/changeset/231131>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 185112

[Caio Lima]

Created attachment 339068 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 339068 [details]
Patch

Clearing flags on attachment: 339068

Committed r231137: <https://trac.webkit.org/changeset/231137>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 185118

[Caio Lima]

Created attachment 339214 [details]
Patch

[Caio Lima]

Created attachment 339218 [details]
Benchmark

This new Path is perf neutral.

[Caio Lima]

Ping review?

[Caio Lima]

Created attachment 339413 [details]
Patch

[Caio Lima]

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

[Saam Barati]

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.

[Saam Barati]

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*

[Caio Lima]

Created attachment 339634 [details]
Patch

[Caio Lima]

Ping Review

[Caio Lima]

Created attachment 340159 [details]
Patch

[Caio Lima]

Created attachment 340160 [details]
Patch

[Yusuke Suzuki]

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.`

[Caio Lima]

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.

[Caio Lima]

Created attachment 340240 [details]
Patch for landing

[WebKit Commit Bot]

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

[Yusuke Suzuki]

(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.

[Caio Lima]

Created attachment 340242 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 340242 [details]
Patch for landing

Clearing flags on attachment: 340242

Committed r231733: <https://trac.webkit.org/changeset/231733>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yusuke Suzuki]

Remember ArithMul can return number only. We need to add ValueMul too in DFG.

[Caio Lima]

(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.