[webkit-reviews] review granted: [Bug 180636] It should be possible to flag a cell for unconditional finalization : [Attachment 329129] the patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Filip Pizlo <fpizlo at apple.com>'s
request for review:
Bug 180636: It should be possible to flag a cell for unconditional finalization
https://bugs.webkit.org/show_bug.cgi?id=180636

Attachment 329129: the patch

https://bugs.webkit.org/attachment.cgi?id=329129&action=review




--- Comment #28 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 329129
  --> https://bugs.webkit.org/attachment.cgi?id=329129
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=329129&action=review

r=me
Patch LGTM. I have a couple comments w.r.t documenting some return types. And I
have a question about when sweepToFreeList can run and potential races.

> Source/JavaScriptCore/heap/IsoCellSet.cpp:75
> +void IsoCellSet::sweepToFreeList(MarkedBlock::Handle* block)

Can this run concurrently to add()/remove()? I ask because the various filter()
calls don't synchronize with the CAS, and also the block->isEmpty()/stale case
seems like it could be racy in the same way that forEachCell can't run
concurrently to add()/remove()

> Source/JavaScriptCore/heap/IsoCellSet.h:47
> +    bool add(HeapCell* cell);
> +    
> +    bool remove(HeapCell* cell);

Can you add a comment saying if what these return. For example:
add() -> return true when item was added (which is what I think this code does,
but I wasn't 100% sure because of the bitmap code)

> Source/JavaScriptCore/heap/IsoCellSetInlines.h:51
> +    return bits->concurrentTestAndClear(atomIndices.atomNumber);

Don't you want ! here so that it returns true if you removed the item? Honestly
I might be misreading the code inside BitMap/Atomics. I really think it'd be
helpful to have a comment in concurrentTestAnd* saying what the return values
mean.

> Source/JavaScriptCore/heap/IsoCellSetInlines.h:64
> +void IsoCellSet::forEachMarkedCell(const Func& func)

I think it might be worth a comment in the header where you say at what points
this is legal to run at (or at what points it's illegal to run at). For
example, I see that when we start to do a full collection, we clear these bits
inside MarkedAllocator.

> Source/JavaScriptCore/heap/IsoCellSetInlines.h:67
> +    (allocator.m_markingNotEmpty & m_blocksWithBits).forEachSetBit(

Is it worth doing something like:
allocator.m_markingNotEmpty.forEachSetBit([&] (size_t index) { if
(!m_blocksWithBits[index]) return; ..... rest of function

that way we don't allocate an intermediate bit vector here?

> Source/JavaScriptCore/runtime/InferredType.h:256
> +    // FIXME: This should be Poisoned.

Please link to a bug #

> Source/WTF/wtf/ConcurrentBuffer.h:65
> +	   Array* newArray = createArray(newSize);

This is implicitly casting to unsigned which scares me on 64-bit platforms. I
think we should crash if we're shaving off bits here.

> Source/WTF/wtf/ConcurrentBuffer.h:82
> +	   growExact(std::max(newSize, size * 2));

Maybe worth overflow check on size*2 (at least for 32-bit)?

> Source/WTF/wtf/ConcurrentBuffer.h:98
> +	   Array* result =
static_cast<Array*>(fastMalloc(OBJECT_OFFSETOF(Array, data) + sizeof(T) *
size));

Maybe it's worth an overflow check here (at least for 32-bit this might make
sense)