[webkit-reviews] review denied: [Bug 86692] Frequent VM allocations can be very slow : [Attachment 142390] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 86692: Frequent VM allocations can be very slow
https://bugs.webkit.org/show_bug.cgi?id=86692

Attachment 142390: Patch
https://bugs.webkit.org/attachment.cgi?id=142390&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=142390&action=review


Please post performance numbers for this change.

> Source/JavaScriptCore/heap/BlockAllocator.cpp:57
> +#if HAVE(MADV_FREE_REUSE)
> +    m_hasBeenReleased = true;
> +    while (madvise(m_allocation.base(), m_allocation.size(),
MADV_FREE_REUSABLE) == -1 && errno == EAGAIN) { }
> +#else
> +    delete this;
> +#endif

It's not so good to have two different models for handling our blocks, where
one model is not tested on our most tested platforms. 

I'd prefer to see this code have one model, which assumes that the platform
knows how to return memory to the OS. Then, use the OSAllocator functions,
rather than hand-coding the #ifdefs here, to get the right behavior.

> Source/JavaScriptCore/heap/BlockAllocator.cpp:76
> +    if (m_nextBlock >= static_cast<char*>(m_allocation.base()) +
m_allocation.size())

This calculation for what amounts to "end" happens in a lot of places, so you
should factor it out into a helper function.

> Source/JavaScriptCore/heap/BlockAllocator.cpp:91
> +bool HeapChunk::hasMoreBlocks()

This is the opposite of what you've called "active" blocks, so it should be
called "inactive" blocks, not "more" blocks.

> Source/JavaScriptCore/heap/BlockAllocator.cpp:101
> +void HeapChunk::moveToEndOf(DoublyLinkedList<HeapChunk>& chunks,
DoublyLinkedList<HeapBlock>& blocks)

This function seems to do some moving to the end and some plain removing. It
could use some clarification.

> Source/JavaScriptCore/heap/CopiedBlock.h:51
> +    CopiedBlock(PageAllocationAligned& allocation)

Can you figure out a way to have just one constructor path for CopiedBlock and
MarkedBlock now? All memory chunks are being allocated out of the block
allocator, so it doesn't seems meaningful to have different constructor paths.