[webkit-reviews] review granted: [Bug 121007] Clearing MarkedBlock::m_newlyAllocated should be separate from MarkedBlock::clearMarks : [Attachment 210981] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 121007: Clearing MarkedBlock::m_newlyAllocated should be separate from
MarkedBlock::clearMarks
https://bugs.webkit.org/show_bug.cgi?id=121007

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

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


> Source/JavaScriptCore/heap/Heap.cpp:471
> +	   m_objectSpace.clearNewlyAllocated();

I think it would be a little clearer to do this after the mark phase, in
Heap::collect(). The newly allocated bits are the inverse of the mark phase:
you need them in order to mark (for the conservative phase), and immediately
after marking, they're null by definition. So, I think it's clearer to set them
before marking (which we do), and then unset them right after marking.

> Source/JavaScriptCore/heap/MarkedAllocator.h:30
> +    MarkedBlock* getAndClearCanonicalizedBlock()

We usually call "getAndClear" "take": takeCanonicalizedBlock.

Also, let's name this accessor the same as its data member. So, either
takeCanonicalizedBlock, and rename the data member to m_canonicalizedBlock, or
takeCurrentBlockAtLastCanonicalize. My preference is to rename the data member.
It's a mouthful.

> Source/JavaScriptCore/heap/MarkedSpace.cpp:253
> +    // We have to iterate all of the blocks in the large allocators because
they are
> +    // canonicalized as they are used up (see
MarkedAllocator::tryAllocateHelper)
> +    // which creates the m_newlyAllocated bitmap.

Why do we do that? I think it would be a bit clearer just to canonicalize all
large blocks at the start of GC, with the rest of our blocks.