Bug 181144 – [JSC] Implement BigInt.asIntN and BigInt.asUintN

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Caio Lima]

...

[Yusuke Suzuki]

Created attachment 398037 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 398342 [details]
Patch

[Yusuke Suzuki]

Created attachment 398349 [details]
Patch

[Caio Lima]

Comment on attachment 398349 [details]
Patch

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

Informal review there. LGTM with one comment regarding test coverage.

> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:144
> +    JSValue bigInt = toBigInt(globalObject, callFrame->argument(1));

I think we should add test cases that verifies if both `toIndex` and `toBigInt` are being properly called. I think that there are some cases of it on Test262, but it would be nice to add this coverage under JSC stress tests.

[Saam Barati]

Comment on attachment 398349 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        This patch implements BigInt.asIntN[1] and BigInt.asUintN[2] features.
> +        As the same to the other BigInt runtime C++ code, we port V8 code to JSC to implement both.

Can you describe here what they do?

[Yusuke Suzuki]

Comment on attachment 398349 [details]
Patch

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

Thanks

>> Source/JavaScriptCore/ChangeLog:9
>> +        As the same to the other BigInt runtime C++ code, we port V8 code to JSC to implement both.
> 
> Can you describe here what they do?

Added

>> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:144
>> +    JSValue bigInt = toBigInt(globalObject, callFrame->argument(1));
> 
> I think we should add test cases that verifies if both `toIndex` and `toBigInt` are being properly called. I think that there are some cases of it on Test262, but it would be nice to add this coverage under JSC stress tests.

Added

[Yusuke Suzuki]

Created attachment 398399 [details]
Patch

[Darin Adler]

Comment on attachment 398399 [details]
Patch

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

I didn’t carefully read the algorithms. I’m counting on the test cases instead.

> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:105
> +    throwTypeError(globalObject, scope, "Invalid primitive type in ToBigInt operation"_s);

Is this an improvement in the error message over "Invalid argument type"? Or is it just a little too much global replacing?

> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:141
> +    uint32_t bits = callFrame->argument(0).toIndex(globalObject, "bits");

This is the kind of place where I love "auto". Because Otherwise I ask myself, "is uint32_t the right type for the result of toIndex?"

I know the standard calls this argument "bits" but I would call it "number of bits" or "bit count". Because this is not actually "bits".

> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:145
> +    JSValue bigInt = toBigInt(globalObject, callFrame->argument(1));
> +    RETURN_IF_EXCEPTION(scope, { });

Do we want a fast case that avoids ever creating the BigInt? Seems like a long round trip for, say, a boolean or an integer. I guess, on reflection, BigInt32 is that optimization. Still seems a shame to me, though.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:396
> +ALWAYS_INLINE JSBigInt::ImplResult JSBigInt::zeroImpl(VM& vm)

Does this function really need "impl" in its name?

ALWAYS_INLINE seems like it might be overkill, but I guess it will do no harm.

> Source/JavaScriptCore/runtime/JSBigInt.h:421
> +    static JSValue asIntN(JSGlobalObject*, uint64_t, JSBigInt*);
> +    static JSValue asUintN(JSGlobalObject*, uint64_t, JSBigInt*);
> +#if USE(BIGINT32)
> +    static JSValue asIntN(JSGlobalObject*, uint64_t, int32_t);
> +    static JSValue asUintN(JSGlobalObject*, uint64_t, int32_t);
> +#endif

I don’t think these arguments are really self-explanatory. Maybe give the uint64_t one a name, and the int32_t one.

Do these really need to be member functions? Why not file-local functions instead?

> Source/JavaScriptCore/runtime/JSBigInt.h:559
> +    template <typename BigIntImpl>
> +    static ImplResult asIntNImpl(JSGlobalObject*, uint64_t, BigIntImpl);
> +    template <typename BigIntImpl>
> +    static ImplResult asUintNImpl(JSGlobalObject*, uint64_t, BigIntImpl);
> +    template <typename BigIntImpl>
> +    static ImplResult truncateToNBits(VM&, int32_t, BigIntImpl);
> +    template <typename BigIntImpl>
> +    static ImplResult truncateAndSubFromPowerOfTwo(VM&, int32_t, BigIntImpl, bool resultSign);

Do these really need to be member function templates? Why not file-local function templates instead?

> Source/JavaScriptCore/runtime/JSBigInt.h:561
> +    static ImplResult zeroImpl(VM&);

Ditto.

[Yusuke Suzuki]

Comment on attachment 398399 [details]
Patch

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

>> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:105
>> +    throwTypeError(globalObject, scope, "Invalid primitive type in ToBigInt operation"_s);
> 
> Is this an improvement in the error message over "Invalid argument type"? Or is it just a little too much global replacing?

Oops, this is replace-error. Fixed.

>> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:141
>> +    uint32_t bits = callFrame->argument(0).toIndex(globalObject, "bits");
> 
> This is the kind of place where I love "auto". Because Otherwise I ask myself, "is uint32_t the right type for the result of toIndex?"
> 
> I know the standard calls this argument "bits" but I would call it "number of bits" or "bit count". Because this is not actually "bits".

number of bits sounds nice. Fixed.
Changed to auto too.

>> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:145
>> +    RETURN_IF_EXCEPTION(scope, { });
> 
> Do we want a fast case that avoids ever creating the BigInt? Seems like a long round trip for, say, a boolean or an integer. I guess, on reflection, BigInt32 is that optimization. Still seems a shame to me, though.

For integers, toBigInt should throw an error. So it does not matter.
For boolean, we already have a fast path in toBigInt, which creates BigInt32 instead of heap JSBigInt. So it does not allocate heap object. I think this is enough efficient for now.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:396
>> +ALWAYS_INLINE JSBigInt::ImplResult JSBigInt::zeroImpl(VM& vm)
> 
> Does this function really need "impl" in its name?
> 
> ALWAYS_INLINE seems like it might be overkill, but I guess it will do no harm.

The name "Impl" is used since this function is returning ImplResult, so, this is zero-impl.
If we want to have `JSBigInt::zero(VM&)`, then we should wrap this function, and return JSValue instead.
For now, I left ALWAYS_INLINE to make sure this is inlined since it is quite small enough.

>> Source/JavaScriptCore/runtime/JSBigInt.h:421
>> +#endif
> 
> I don’t think these arguments are really self-explanatory. Maybe give the uint64_t one a name, and the int32_t one.
> 
> Do these really need to be member functions? Why not file-local functions instead?

Add names to these variables.
They are called from BigIntConstructor, so they need to be exposed as non-file-local functions.

>> Source/JavaScriptCore/runtime/JSBigInt.h:559
>> +    static ImplResult truncateAndSubFromPowerOfTwo(VM&, int32_t, BigIntImpl, bool resultSign);
> 
> Do these really need to be member function templates? Why not file-local function templates instead?

This is necessary since these functions are reading static private variables / calling private functions of JSBigInt.
In addition, we are doing the same thing for the other JSBigInt functions. So doing this is good in terms of consistency.

>> Source/JavaScriptCore/runtime/JSBigInt.h:561
>> +    static ImplResult zeroImpl(VM&);
> 
> Ditto.

zeroImpl can be a static function. Fixed.

[Yusuke Suzuki]

Committed r261156: <https://trac.webkit.org/changeset/261156>

[Radar WebKit Bug Importer]

<rdar://problem/62879642>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 211447

[Yusuke Suzuki]

Reverted r261156 for reason:

Break ARM64_32 build due to existing bug

Committed r261167: <https://trac.webkit.org/changeset/261167>

[Yusuke Suzuki]

Created attachment 398519 [details]
Patch

Patch for relanding

[EWS]

Committed r261199: <https://trac.webkit.org/changeset/261199>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398519 [details].