Summary: | Reland StructureID overhaul | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keith Miller <keith_miller> | ||||||
Component: | New Bugs | Assignee: | Keith Miller <keith_miller> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | annulen, benjamin, cdumez, cmarcelo, ews-watchlist, gyuyoung.kim, mark.lam, msaboff, ryuan.choi, saam, sergio, tzagallo, webkit-bug-importer, ysuzuki | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=233379 | ||||||||
Attachments: |
|
Description
Keith Miller
2022-01-27 10:04:00 PST
Created attachment 450155 [details]
Patch
Comment on attachment 450155 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450155&action=review r=me > Source/JavaScriptCore/heap/StructureAlignedMemoryAllocator.cpp:105 > + MarkedBlock* block = reinterpret_cast<MarkedBlock*>(g_jscConfig.startOfStructureHeap) + freeIndex * MarkedBlock::blockSize; > + constexpr bool writable = true; > + constexpr bool executable = false; > + OSAllocator::commit(block, MarkedBlock::blockSize, writable, executable); Let's mprotect READ | WRITE in debug build. > Source/JavaScriptCore/heap/StructureAlignedMemoryAllocator.cpp:111 > + OSAllocator::decommit(blockPtr, MarkedBlock::blockSize); Let's mprotect NONE in debug build. > Source/JavaScriptCore/heap/StructureAlignedMemoryAllocator.cpp:149 > +void StructureAlignedMemoryAllocator::commitBlock(void* block) > +{ > + constexpr bool writable = true; > + constexpr bool executable = false; > + OSAllocator::commit(block, MarkedBlock::blockSize, writable, executable); > +} Let's mprotect READ | WRITE in debug build. > Source/JavaScriptCore/heap/StructureAlignedMemoryAllocator.cpp:154 > +void StructureAlignedMemoryAllocator::decommitBlock(void* block) > +{ > + OSAllocator::decommit(block, MarkedBlock::blockSize); > +} Let's mprotect NONE in debug build. Created attachment 450407 [details]
Patch for landing
Committed r288815 (246591@main): <https://commits.webkit.org/246591@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 450407 [details]. Comment on attachment 450407 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=450407&action=review > Source/JavaScriptCore/heap/StructureAlignedMemoryAllocator.cpp:85 > + ASSERT((g_jscConfig.startOfStructureHeap & ~structureIDMask) == g_jscConfig.startOfStructureHeap); nits: - Should be RELEASE_ASSERT - And let's also RELEASE_ASSERT that startOfStructureHeap isn't null. > Source/JavaScriptCore/heap/StructureAlignedMemoryAllocator.cpp:95 > + ASSERT(freeIndex <= m_usedBlocks.bitCount()); This code is also quite subtle. You grow the bit vector by "not finding a bit", and having find return bitCount(). Might be worth a comment. > Source/JavaScriptCore/heap/StructureAlignedMemoryAllocator.cpp:102 > + MarkedBlock* block = reinterpret_cast<MarkedBlock*>(g_jscConfig.startOfStructureHeap) + freeIndex * MarkedBlock::blockSize; same nit from last patch: use uint8_t* here instead of MarkedBlocked* Comment on attachment 450407 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=450407&action=review > Source/JavaScriptCore/heap/StructureAlignedMemoryAllocator.cpp:92 > + Locker locker(m_lock); Not sure we need a lock here, given the IsoMemoryAllocatorBase already locks. We could pass around a locker if we wanted to indicate this. |