Bug 181144

Summary: [JSC] Implement BigInt.asIntN and BigInt.asUintN
Product: WebKit Reporter: Caio Lima <ticaiolima>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, ews-watchlist, keith_miller, mark.lam, msaboff, rmorisset, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 206182, 211447    
Bug Blocks: 179001    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
darin: review+
Patch none

Description Caio Lima 2017-12-23 13:11:49 PST
...
Comment 1 Yusuke Suzuki 2020-04-29 22:54:34 PDT
Created attachment 398037 [details]
Patch

WIP
Comment 2 Yusuke Suzuki 2020-05-03 18:49:27 PDT
Created attachment 398342 [details]
Patch
Comment 3 Yusuke Suzuki 2020-05-04 00:20:09 PDT
Created attachment 398349 [details]
Patch
Comment 4 Caio Lima 2020-05-04 08:12:32 PDT
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.
Comment 5 Saam Barati 2020-05-04 10:53:47 PDT
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?
Comment 6 Yusuke Suzuki 2020-05-04 12:23:02 PDT
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
Comment 7 Yusuke Suzuki 2020-05-04 12:30:35 PDT
Created attachment 398399 [details]
Patch
Comment 8 Darin Adler 2020-05-05 00:04:09 PDT
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.
Comment 9 Yusuke Suzuki 2020-05-05 01:25:52 PDT
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.
Comment 10 Yusuke Suzuki 2020-05-05 01:30:08 PDT
Committed r261156: <https://trac.webkit.org/changeset/261156>
Comment 11 Radar WebKit Bug Importer 2020-05-05 01:31:16 PDT
<rdar://problem/62879642>
Comment 12 WebKit Commit Bot 2020-05-05 07:34:49 PDT
Re-opened since this is blocked by bug 211447
Comment 13 Yusuke Suzuki 2020-05-05 07:40:20 PDT
Reverted r261156 for reason:

Break ARM64_32 build due to existing bug

Committed r261167: <https://trac.webkit.org/changeset/261167>
Comment 14 Yusuke Suzuki 2020-05-05 09:30:21 PDT
Created attachment 398519 [details]
Patch

Patch for relanding
Comment 15 EWS 2020-05-05 14:15:11 PDT
Committed r261199: <https://trac.webkit.org/changeset/261199>

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