Summary: | Gigacages should start allocations from a slide | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keith Miller <keith_miller> | ||||||||||||||||||
Component: | New Bugs | Assignee: | 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
Keith Miller
2019-01-16 18:32:23 PST
Created attachment 359341 [details]
Patch
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 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 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 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. Created attachment 359396 [details]
Patch
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.
Created attachment 359409 [details]
Patch
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.
Created attachment 359410 [details]
Patch
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.
Created attachment 359425 [details]
Patch
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.
Created attachment 359430 [details]
Patch
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 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 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 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. Created attachment 359501 [details]
Patch for landing
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. (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 on attachment 359501 [details] Patch for landing Clearing flags on attachment: 359501 Committed r240160: <https://trac.webkit.org/changeset/240160> All reviewed patches have been landed. Closing bug. (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. (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. Reverted r240160 for reason: This broke multiple internal builds. Committed r240171: <https://trac.webkit.org/changeset/240171> Created attachment 359532 [details] Patch to fix the builds when re-landing This should have fixed the builds had r240160 not been rolled out. Committed r240175: <https://trac.webkit.org/changeset/240175> (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> |