[webkit-reviews] review granted: [Bug 85429] Heap should sweep incrementally : [Attachment 144941] 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 85429: Heap should sweep incrementally
https://bugs.webkit.org/show_bug.cgi?id=85429

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

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


r=me -- with these suggestions:

> Source/JavaScriptCore/heap/Heap.h:169
> +	   void incrementalSweep();
> +	   void didFinishSweeping();

These functions don't exist anymore, so you can remove the declaration.

> Source/JavaScriptCore/heap/IncrementalSweeper.cpp:65
> +	   if (nextBlock->needsSweeping()) {

The WebKit style prefers "if (!nextBlock->needsSweeping()) continue;" because
it reduces the indentation of subsequent code.

> Source/JavaScriptCore/heap/IncrementalSweeper.cpp:74
> +    m_lengthOfLastSweepIncrement = WTF::monotonicallyIncreasingTime() -
sweepBeginTime;

I think you should remove this -- if we're done, we didn't sweep, so we
shouldn't record an increment.

> Source/JavaScriptCore/heap/IncrementalSweeper.cpp:75
> +    cancelTimer();

It would be nice to call m_blocksToSweep.clear() to give back the memory for
the vector, in case the heap is very big.