Bug 194888

Summary: BigInt should store its data in the primitive gigacage.
Product: WebKit Reporter: Keith Miller <keith_miller>
Component: JavaScriptCoreAssignee: Keith Miller <keith_miller>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, mark.lam, msaboff, sbarati, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch mark.lam: review+

Description Keith Miller 2019-02-20 17:53:05 PST
BigInt should store its data in the primitive gigacage.
Comment 1 Keith Miller 2019-02-27 09:56:29 PST
Created attachment 363098 [details]
Patch
Comment 2 Keith Miller 2019-02-27 10:01:03 PST
Created attachment 363099 [details]
Patch
Comment 3 Keith Miller 2019-02-27 10:05:41 PST
Created attachment 363100 [details]
Patch
Comment 4 Yusuke Suzuki 2019-02-27 10:56:22 PST
Comment on attachment 363100 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSBigInt.h:245
> +    CagedBarrierPtr<Gigacage::Primitive, Digit> m_data;

Do we need to use CagedBarrierPtr instead of CagedPtr here?
Comment 5 Yusuke Suzuki 2019-02-27 11:00:20 PST
Comment on attachment 363100 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSBigInt.cpp:74
> +    Gigacage::free(Gigacage::Primitive, thisObject->dataStorage());

If we use CagedUniquePtr, we do not need to call this in JSBigInt::destroy. We can call thisObject-~JSBigInt().
Comment 6 Keith Miller 2019-02-28 11:07:43 PST
Comment on attachment 363100 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:74
>> +    Gigacage::free(Gigacage::Primitive, thisObject->dataStorage());
> 
> If we use CagedUniquePtr, we do not need to call this in JSBigInt::destroy. We can call thisObject-~JSBigInt().

Ohh, nice, I'll change.
Comment 7 Keith Miller 2019-02-28 11:38:54 PST
Created attachment 363244 [details]
Patch
Comment 8 Saam Barati 2019-02-28 12:41:09 PST
Comment on attachment 363244 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSBigInt.cpp:119
>  JSBigInt* JSBigInt::createWithLengthUnchecked(VM& vm, unsigned length)
>  {
>      ASSERT(length <= maxLength);
> -    JSBigInt* bigInt = new (NotNull, allocateCell<JSBigInt>(vm.heap, allocationSize(length))) JSBigInt(vm, vm.bigIntStructure.get(), length);
> +    void* data = Gigacage::tryMalloc(Gigacage::Primitive, length * sizeof(Digit));
> +    if (!data)
> +        return nullptr;
> +
> +    JSBigInt* bigInt = new (NotNull, allocateCell<JSBigInt>(vm.heap)) JSBigInt(vm, vm.bigIntStructure.get(), reinterpret_cast<Digit*>(data), length);
>      bigInt->finishCreation(vm);
>      return bigInt;
>  }

The point of this function is that we don't return nullptr. So this shouldn't be using tryMalloc. Perhaps somewhere earlier we should be tryMallocing and throwing an OOM? If you grep for callers of this, you'll see that we just assume it's non-null.
Comment 9 Yusuke Suzuki 2019-11-17 23:03:21 PST
Created attachment 383732 [details]
Patch
Comment 10 Mark Lam 2019-11-17 23:56:33 PST
Comment on attachment 383732 [details]
Patch

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

r=me with fix.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:111
> +    ASSERT(data);

This is not strictly needed because malloc() should crash on failure to allocate, but I suppose it doesn't hurt.

> Source/JavaScriptCore/runtime/JSBigInt.h:56
>      JSBigInt(VM&, Structure*, unsigned length);

Shouldn't this line be removed?  This constructor doesn't exist anymore.
Comment 11 Yusuke Suzuki 2019-11-18 00:21:00 PST
Comment on attachment 383732 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:111
>> +    ASSERT(data);
> 
> This is not strictly needed because malloc() should crash on failure to allocate, but I suppose it doesn't hurt.

Yeah, removing this is also OK.

>> Source/JavaScriptCore/runtime/JSBigInt.h:56
>>      JSBigInt(VM&, Structure*, unsigned length);
> 
> Shouldn't this line be removed?  This constructor doesn't exist anymore.

Yes, we should remove it. Fixed.
Comment 12 Yusuke Suzuki 2019-11-18 00:24:07 PST
Committed r252538: <https://trac.webkit.org/changeset/252538>
Comment 13 Radar WebKit Bug Importer 2019-11-18 00:25:19 PST
<rdar://problem/57274577>