Bug 194888 - BigInt should store its data in the primitive gigacage.
Summary: BigInt should store its data in the primitive gigacage.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-20 17:53 PST by Keith Miller
Modified: 2019-11-18 00:25 PST (History)
7 users (show)

See Also:


Attachments
Patch (4.89 KB, patch)
2019-02-27 09:56 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (5.18 KB, patch)
2019-02-27 10:01 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (5.19 KB, patch)
2019-02-27 10:05 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (5.38 KB, patch)
2019-02-28 11:38 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (8.28 KB, patch)
2019-11-17 23:03 PST, Yusuke Suzuki
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>