Bug 193523

Summary: Gigacages should start allocations from a slide
Product: WebKit Reporter: Keith Miller <keith_miller>
Component: New BugsAssignee: Keith Miller <keith_miller>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, commit-queue, dbates, ddkilzer, ews-watchlist, ggaren, guijemont, guijemont+jsc-armv7-ews, jlewis3, mark.lam, msaboff, ryanhaddad, saam, 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
none
Patch
none
Patch for landing
none
Patch to fix the builds when re-landing none

Description Keith Miller 2019-01-16 18:32:23 PST
Gigacages should start allocations from a slide
Comment 1 Keith Miller 2019-01-16 18:38:06 PST
Created attachment 359341 [details]
Patch
Comment 2 Keith Miller 2019-01-16 18:40:55 PST
rdar://problem/44958707
Comment 3 Geoffrey Garen 2019-01-16 19:13:35 PST
Comment on attachment 359341 [details]
Patch

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

r- because red bots, also a suggested improvement

> Source/bmalloc/bmalloc/Heap.cpp:68
> +            ptrdiff_t offset = ((random * vmPageSize()) % (gigacageSize() - Gigacage::minimumCageSizeAfterSlide));

This seems wrong. You're losing low bits of entropy by multiplying.

The best way to do this is probably to compute a random offset, allowing for minimumCageSizeAfterSlide, and then round down to page alignment at the very end. That way, you don't need an ASSERT about page alignment either. You can trade an assert for code that enforces the condition you want.
Comment 4 Yusuke Suzuki 2019-01-16 19:15:00 PST
Comment on attachment 359341 [details]
Patch

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

> Source/WTF/wtf/Gigacage.h:37
> +const size_t primitiveGigacageMask = gigacageSizeToMask(primitiveGigacageSize);
> +const size_t jsValueGigacageMask = gigacageSizeToMask(jsValueGigacageSize);
> +const size_t gigacageBasePtrsSize = 8 * 1024;

`constexpr size_t` would be nice. And `8 * Sizes::KB` would be more readable.

> Source/bmalloc/bmalloc/Gigacage.cpp:45
> +const size_t gigacageRunway = 32llu * 1024 * 1024 * 1024;

Ditto for `constexpr size_t`. And I think `32 * Sizes::GB` would be more readable.

> Source/bmalloc/bmalloc/Gigacage.h:43
> +const size_t primitiveGigacageSize = 0x80000000;
> +const size_t jsValueGigacageSize = 0x40000000;

It seems that primitiveGigacageSize is 2GB, and jsValueGigacageSize is 1GB. So, if minimumCageSizeAfterSlide is 2GB, it goes to the some funny state. Is my understanding correct?
And I think we can use `2 * bmalloc::Sizes::GB` nad `1 * bmalloc::Sizes::GB` here.

> Source/bmalloc/bmalloc/Gigacage.h:45
> +const size_t minimumCageSizeAfterSlide = 2 * Sizes::GB;

Ditto for `constexpr size_t`.

> Source/bmalloc/bmalloc/Gigacage.h:49
> +const size_t primitiveGigacageSize = 0x800000000;
> +const size_t jsValueGigacageSize = 0x400000000;

This is causing build failure in 32bit arch since size_t is 32bit while the value is larger than 32bit, while gigacage is not used in 32bit anyway.

> Source/bmalloc/bmalloc/Gigacage.h:51
> +const size_t minimumCageSizeAfterSlide = 4 * bmalloc::Sizes::GB;

Ditto for `constexpr size_t`.

> Source/bmalloc/bmalloc/Gigacage.h:68
> +const size_t primitiveGigacageMask = gigacageSizeToMask(primitiveGigacageSize);
> +const size_t jsValueGigacageMask = gigacageSizeToMask(jsValueGigacageSize);

Ditto for `constexpr size_t`.

> Source/bmalloc/bmalloc/Sizes.h:48
>      static const size_t kB = 1024;
>      static const size_t MB = kB * kB;
> +    static const size_t GB = kB * kB * kB;
>  
>      static const size_t alignment = 8;
>      static const size_t alignmentMask = alignment - 1ul;

`static constexpr` would be nice.
Comment 5 Keith Miller 2019-01-16 23:33:04 PST
Comment on attachment 359341 [details]
Patch

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

>> Source/WTF/wtf/Gigacage.h:37
>> +const size_t gigacageBasePtrsSize = 8 * 1024;
> 
> `constexpr size_t` would be nice. And `8 * Sizes::KB` would be more readable.

Is there an advantage of constexpr over const? I can make the change though.

>> Source/bmalloc/bmalloc/Gigacage.cpp:45
>> +const size_t gigacageRunway = 32llu * 1024 * 1024 * 1024;
> 
> Ditto for `constexpr size_t`. And I think `32 * Sizes::GB` would be more readable.

Sure.

>> Source/bmalloc/bmalloc/Gigacage.h:43
>> +const size_t jsValueGigacageSize = 0x40000000;
> 
> It seems that primitiveGigacageSize is 2GB, and jsValueGigacageSize is 1GB. So, if minimumCageSizeAfterSlide is 2GB, it goes to the some funny state. Is my understanding correct?
> And I think we can use `2 * bmalloc::Sizes::GB` nad `1 * bmalloc::Sizes::GB` here.

Good catch! it appears that I messed up the math. I thought it was 4GB and 2GB... Whoops!

>> Source/bmalloc/bmalloc/Gigacage.h:49
>> +const size_t jsValueGigacageSize = 0x400000000;
> 
> This is causing build failure in 32bit arch since size_t is 32bit while the value is larger than 32bit, while gigacage is not used in 32bit anyway.

Yeah, I'll fix that.

>> Source/bmalloc/bmalloc/Heap.cpp:68
>> +            ptrdiff_t offset = ((random * vmPageSize()) % (gigacageSize() - Gigacage::minimumCageSizeAfterSlide));
> 
> This seems wrong. You're losing low bits of entropy by multiplying.
> 
> The best way to do this is probably to compute a random offset, allowing for minimumCageSizeAfterSlide, and then round down to page alignment at the very end. That way, you don't need an ASSERT about page alignment either. You can trade an assert for code that enforces the condition you want.

Yeah, I was thinking that it would be the same as rounding down. But I think you are right since gigacageSize() - Gigacage::minimumCageSizeAfterSlide may not be a power of 2.
Comment 6 Yusuke Suzuki 2019-01-16 23:42:35 PST
Comment on attachment 359341 [details]
Patch

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

>>> Source/WTF/wtf/Gigacage.h:37
>>> +const size_t gigacageBasePtrsSize = 8 * 1024;
>> 
>> `constexpr size_t` would be nice. And `8 * Sizes::KB` would be more readable.
> 
> Is there an advantage of constexpr over const? I can make the change though.

In C++14, they are basically the same.
But in C++17, constexpr+static member variable will become `inline` variable. It means that, finally, we can ensure that constant variables are compiled as compile time constant, instead of the reference to the constant variable.
This prevents us from "undefined reference to the variable XXX" error (and sometimes, we used `+variable` hack like https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/b3/B3Common.cpp#L71).

While `inline` will be required for these namespace variables in C++17, I like using `constexpr` for all constant variables basically.
Comment 7 Keith Miller 2019-01-17 11:43:35 PST
Created attachment 359396 [details]
Patch
Comment 8 EWS Watchlist 2019-01-17 11:46:45 PST
Attachment 359396 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/Gigacage.h:215:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/Gigacage.h:219:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Keith Miller 2019-01-17 13:14:33 PST
Created attachment 359409 [details]
Patch
Comment 10 EWS Watchlist 2019-01-17 13:17:24 PST
Attachment 359409 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/Gigacage.h:215:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/Gigacage.h:219:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Keith Miller 2019-01-17 13:45:30 PST
Created attachment 359410 [details]
Patch
Comment 12 EWS Watchlist 2019-01-17 13:49:45 PST
Attachment 359410 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/Gigacage.h:215:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/Gigacage.h:219:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Keith Miller 2019-01-17 16:47:13 PST
Created attachment 359425 [details]
Patch
Comment 14 EWS Watchlist 2019-01-17 16:50:14 PST
Attachment 359425 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/Gigacage.h:215:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/Gigacage.h:219:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Keith Miller 2019-01-17 17:10:28 PST
Created attachment 359430 [details]
Patch
Comment 16 EWS Watchlist 2019-01-17 17:11:58 PST
Attachment 359430 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/Gigacage.h:215:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/Gigacage.h:219:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Mark Lam 2019-01-17 17:14:22 PST
Comment on attachment 359430 [details]
Patch

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

> Source/bmalloc/bmalloc/Sizes.h:126
>  }

nit: can you add a // namespace Sizes here?
Comment 18 Mark Lam 2019-01-17 18:09:45 PST
Comment on attachment 359430 [details]
Patch

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

r=me with suggestions and fixes.

> Source/bmalloc/ChangeLog:8
> +        This patch makes is so that Gigacage Heaps slide the start of the

typo: /makes is so/makes it so/.

> Source/bmalloc/bmalloc/Gigacage.cpp:47
> +constexpr uint64_t gigacageRunway = 32llu * 1024 * 1024 * 1024;

Why not just use a size_t gigacageRunway to begin with?

> Source/bmalloc/bmalloc/Gigacage.cpp:112
> +        return static_cast<size_t>(gigacageRunway);

You can remove this cast since we can define gigacageRunway as a size_t to begin with.

> Source/bmalloc/bmalloc/Heap.cpp:67
> +            RELEASE_BASSERT(gigacageSize() > Gigacage::minimumCageSizeAfterSlide);

Can you just make this a static_assert in Source/bmalloc/bmalloc/Gigacage.h instead?

    static_assert(primitiveGigacageSize > minimumCageSizeAfterSlide);
    static_assert(jsValueGigacageSize > minimumCageSizeAfterSlide);

That way, we can catch this configuration error at compile time instead.
Comment 19 Guillaume Emont 2019-01-18 06:56:37 PST
Comment on attachment 359430 [details]
Patch

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

We see a lot of crashes on arm/linux (where Gigacage is disabled) with this patch. Doing the changes I suggest below seem to fix them all.

> Source/bmalloc/bmalloc/Gigacage.h:218
> +BINLINE bool isCaged(Kind, const void*) { return false; }

We have a few cases of:
  RELEASE_ASSERT(isCaged(kind, p));
in wtf/Gigacage.cpp as well as similar code in wtf/JSValueMalloc.cpp. I believe these should be modified to something like:
  RELEASE_ASSERT(!wasEnabled() || isCaged(kind, p));

What do you think?

> Source/bmalloc/bmalloc/Gigacage.h:219
> +template<typename T> BINLINE T* caged(Kind, T* ptr) { BCRASH(); return ptr; }

This BCRASH() triggers a lot of crashes on arm/linux, and removing it seems to have no ill effect, so I feel like it should be removed, unless there is a strong reason to keep it. The alternative to removing it would be to modify all the code that calls caged() to have an alternative case if Gigacage is disabled.
Comment 20 Keith Miller 2019-01-18 10:06:49 PST
Created attachment 359501 [details]
Patch for landing
Comment 21 Keith Miller 2019-01-18 10:08:09 PST
Comment on attachment 359430 [details]
Patch

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

>> Source/bmalloc/bmalloc/Gigacage.cpp:47
>> +constexpr uint64_t gigacageRunway = 32llu * 1024 * 1024 * 1024;
> 
> Why not just use a size_t gigacageRunway to begin with?

I changed it I think because this originally needed to compile on 32-bit. Since it doesn't anymore I can change it back.

>> Source/bmalloc/bmalloc/Gigacage.cpp:112
>> +        return static_cast<size_t>(gigacageRunway);
> 
> You can remove this cast since we can define gigacageRunway as a size_t to begin with.

Done.

>> Source/bmalloc/bmalloc/Gigacage.h:218
>> +BINLINE bool isCaged(Kind, const void*) { return false; }
> 
> We have a few cases of:
>   RELEASE_ASSERT(isCaged(kind, p));
> in wtf/Gigacage.cpp as well as similar code in wtf/JSValueMalloc.cpp. I believe these should be modified to something like:
>   RELEASE_ASSERT(!wasEnabled() || isCaged(kind, p));
> 
> What do you think?

I'll just have this always return true if caging is disabled.

>> Source/bmalloc/bmalloc/Gigacage.h:219
>> +template<typename T> BINLINE T* caged(Kind, T* ptr) { BCRASH(); return ptr; }
> 
> This BCRASH() triggers a lot of crashes on arm/linux, and removing it seems to have no ill effect, so I feel like it should be removed, unless there is a strong reason to keep it. The alternative to removing it would be to modify all the code that calls caged() to have an alternative case if Gigacage is disabled.

Fair enough, I'll remove the assert.

>> Source/bmalloc/bmalloc/Heap.cpp:67
>> +            RELEASE_BASSERT(gigacageSize() > Gigacage::minimumCageSizeAfterSlide);
> 
> Can you just make this a static_assert in Source/bmalloc/bmalloc/Gigacage.h instead?
> 
>     static_assert(primitiveGigacageSize > minimumCageSizeAfterSlide);
>     static_assert(jsValueGigacageSize > minimumCageSizeAfterSlide);
> 
> That way, we can catch this configuration error at compile time instead.

Sure.

>> Source/bmalloc/bmalloc/Sizes.h:126
>>  }
> 
> nit: can you add a // namespace Sizes here?

Done.
Comment 22 Mark Lam 2019-01-18 10:58:18 PST
(In reply to Keith Miller from comment #21)
> >> Source/bmalloc/bmalloc/Gigacage.h:218
> >> +BINLINE bool isCaged(Kind, const void*) { return false; }
> > 
> > We have a few cases of:
> >   RELEASE_ASSERT(isCaged(kind, p));
> > in wtf/Gigacage.cpp as well as similar code in wtf/JSValueMalloc.cpp. I believe these should be modified to something like:
> >   RELEASE_ASSERT(!wasEnabled() || isCaged(kind, p));
> > 
> > What do you think?
> 
> I'll just have this always return true if caging is disabled.

I was going to disagree with this because I think that it's misleading for isCaged() to return true all the time.  I was thinking that explicitly adding the !wasEnabled() check in the RELEASE_ASSERTs would be clearer about what is happening.

However, I see that the GIGACAGE_ENABLED version of caged() effectively always returns the ptr if !wasEnabled(), and isCaged() always returns true if caged(kind, ptr) == ptr.  Hence, if !GIGACAGE_ENABLED, this reduces to isCaged() always returning true.

I withdraw my objection.  Sorry for the noise.
Comment 23 WebKit Commit Bot 2019-01-18 11:39:03 PST
Comment on attachment 359501 [details]
Patch for landing

Clearing flags on attachment: 359501

Committed r240160: <https://trac.webkit.org/changeset/240160>
Comment 24 WebKit Commit Bot 2019-01-18 11:39:05 PST
All reviewed patches have been landed.  Closing bug.
Comment 25 David Kilzer (:ddkilzer) 2019-01-18 13:23:02 PST
(In reply to WebKit Commit Bot from comment #23)
> Comment on attachment 359501 [details]
> Patch for landing
> 
> Clearing flags on attachment: 359501
> 
> Committed r240160: <https://trac.webkit.org/changeset/240160>

This appears to have broken the internal tvOS build.
Comment 26 David Kilzer (:ddkilzer) 2019-01-18 13:31:35 PST
(In reply to David Kilzer (:ddkilzer) from comment #25)
> (In reply to WebKit Commit Bot from comment #23)
> > Comment on attachment 359501 [details]
> > Patch for landing
> > 
> > Clearing flags on attachment: 359501
> > 
> > Committed r240160: <https://trac.webkit.org/changeset/240160>
> 
> This appears to have broken the internal tvOS build.

I have a fix.
Comment 27 Matt Lewis 2019-01-18 13:32:42 PST
Reverted r240160 for reason:

This broke multiple internal builds.

Committed r240171: <https://trac.webkit.org/changeset/240171>
Comment 28 David Kilzer (:ddkilzer) 2019-01-18 13:36:47 PST
Created attachment 359532 [details]
Patch to fix the builds when re-landing

This should have fixed the builds had r240160 not been rolled out.
Comment 29 Keith Miller 2019-01-18 14:48:35 PST
Committed r240175: <https://trac.webkit.org/changeset/240175>
Comment 30 David Kilzer (:ddkilzer) 2019-01-18 16:09:30 PST
(In reply to Keith Miller from comment #29)
> Committed r240175: <https://trac.webkit.org/changeset/240175>

Follow-up fix when GIGACAGE_ENABLED is 0:

Committed r240186: <https://trac.webkit.org/changeset/240186>