Bug 194888 – BigInt should store its data in the primitive gigacage.

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

[Keith Miller]

BigInt should store its data in the primitive gigacage.

[Keith Miller]

Created attachment 363098 [details]
Patch

[Keith Miller]

Created attachment 363099 [details]
Patch

[Keith Miller]

Created attachment 363100 [details]
Patch

[Yusuke Suzuki]

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?

[Yusuke Suzuki]

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

[Keith Miller]

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.

[Keith Miller]

Created attachment 363244 [details]
Patch

[Saam Barati]

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.

[Yusuke Suzuki]

Created attachment 383732 [details]
Patch

[Mark Lam]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r252538: <https://trac.webkit.org/changeset/252538>

[Radar WebKit Bug Importer]

<rdar://problem/57274577>