[webkit-reviews] review granted: [Bug 92819] MarkedAllocator::tryAllocateHelper() should sweep another block if it can't sweep a Structure block : [Attachment 155698] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted  review:
Bug 92819: MarkedAllocator::tryAllocateHelper() should sweep another block if
it can't sweep a Structure block
https://bugs.webkit.org/show_bug.cgi?id=92819

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

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


Please add the "why" comment before landing.

>>>> Source/JavaScriptCore/heap/IncrementalSweeper.cpp:92
>>>> +	  while (m_currentBlockToSweepIndex < m_blocksToSweep.size()) {
>>> 
>>> I don't think you want this loop here, do you? The goal of this function is
to sweep just one.
>> 
>> The loop needs to be there in case the next block doesn't need sweeping
(i.e. block->needsSweeping() == false).
> 
> OK, I see where you're going with that. I still think it would be simpler
(and slightly faster) to remove the extra loop. I see sweeping a block with no
destructors as just a special fast case, and not a reason to keep sweeping.

Eh, you convinced me. Let's keep it this way to keep things simple.