[webkit-reviews] review granted: [Bug 75181] Implement a new allocator for backing stores : [Attachment 120993] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 75181: Implement a new allocator for backing stores
https://bugs.webkit.org/show_bug.cgi?id=75181

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

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=120993&action=review


R=me.  Can you post the overall performance results?  For something like this
it would be good to do bencher and also in-browser tests.  Also, clarify the
thread safe comments.  For the other comments if there is something obvious
that can be done now, then do it; otherwise post a Radar or bugzilla bug
regarding future direction (i.e. tightening up the fast path code for the
allocator).

> Source/JavaScriptCore/heap/BumpSpaceInlineMethods.h:63
> +// Needs to be threadsafe.

It would be good to clarify these comments.  Does this mean that it needs to be
called under a lock?  Or something else?

> Source/JavaScriptCore/heap/BumpSpaceInlineMethods.h:244
> +inline void* BumpSpace::allocate(size_t bytes)
> +{
> +    ASSERT(!m_heap->globalData()->isInitializingObject());
> +
> +    if (isOversize(bytes))
> +	   return allocateOversize(bytes);
> +    
> +    if (!fitsInCurrentBlock(bytes)) {
> +	   addNewBlock();
> +	   m_toSpaceFilter.add(reinterpret_cast<Bits>(m_currentBlock));
> +	   m_toSpaceSet.add(m_currentBlock);
> +    }
> +    
> +    m_totalMemoryUtilized += bytes;
> +    return allocateFromBlock(m_currentBlock, bytes);
> +}

Is this your fast path?  If so, it would be good if it could be turned into
something like:

if (isOversize(bytes) || !fitsInCurrentBlock(bytes)) return
callSomeOutOfLineSlowPath(bytes);
return allocateFromBlock(m_currentBlock, bytes);

Also, it would be good if:

- m_totalMemoryUtilized was lazily updated when a block is done being allocated
in, since your eager update of this value currently means an extra load and
store.

- The data needed to allocate in the current block was kept in BumpSpace, or in
something like a BumpAllocator class.  I.e. if you want to allocate stuff (so
you're either a GC thread or you're the application) then you have an instance
of BumpAllocator that is easily reachable (say by offsetting into JSGlobalData,
if you're the application), and then all of the fields (i.e. m_offset) needed
to allocate are inside of BumpAllocator.  Right now you're taking an extra load
to get to the BumpBlock::m_offset.  The BumpBlock::m_offset field can then be
updated after you switch from one block to another.

It's OK if this initial patch doesn't do these things, but it would be great to
add a FIXME somewhere documenting this, or else create a Radar that indicates
that this is what should be done.

The point is that it will (a) simplify the inline asm code that the JIT will
use for allocation and (b) speed things up a fair bit as we start relying on
this allocator more.

> Source/JavaScriptCore/heap/MarkStack.cpp:407
> +			   doneCopying();

Does this mean that we call doneCopying twice?	This is for the master
SlotVisitor, which you call doneCopying() for in markRoots().