Bug 235720 – Reland StructureID overhaul

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

Reland StructureID overhaul

[Keith Miller]

Created attachment 450155 [details]
Patch

[Yusuke Suzuki]

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.

[Keith Miller]

Created attachment 450407 [details]
Patch for landing

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/88270829>

[Saam Barati]

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*

[Saam Barati]

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.