Bug 85429 – Heap should sweep incrementally

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Hahnenberg]

We shouldn't have to wait for the opportunistic GC timer to fire in order to call object destructors. Instead, we should incrementally sweep some subset of the blocks requiring sweeping with each collection. This way, our memory usage scales smoothly with actual use, regardless of whether we've recently done an opportunistic GC or not.

[Mark Hahnenberg]

Actually, we don't want to depend on collections at all for sweeping. We should have a run loop timer that fires every so often to do some sweeping.

[Mark Hahnenberg]

Created attachment 144651 [details]
Patch

[Early Warning System Bot]

Comment on attachment 144651 [details]
Patch

Attachment 144651 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12853397

[Geoffrey Garen]

Some high-level thoughts here:

(1) Is it OK for the sweep timer to compete against the GC timer? 

(2) Should the incremental sweeper free empty blocks?

(3) Would be nice to save the resulting free list in the MarkedBlock, so we don't have to reproduce it later.

(4) It's not so good to fire every 100ms. If a given sweep operation took a long time, we could saturate the timer interval. I think it would be better to sweep the minimum possible in each interval -- one block -- and, after we're done, then schedule the next timer relative to the current time, with an interval equal to however long we took * constant.

[Build Bot]

Comment on attachment 144651 [details]
Patch

Attachment 144651 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12838783

[Mark Hahnenberg]

(In reply to comment #4)
> Some high-level thoughts here:
> 
> (1) Is it OK for the sweep timer to compete against the GC timer? 

I'm not sure what you mean by "compete". Do you mean does the order in which they run matter?

> (2) Should the incremental sweeper free empty blocks?

I think it should, but I was going to add that when we can also do finalization incrementally so that we sweep, finalize, then free. I guess we could technically add freeing blocks to this patch though. 

> 
> (3) Would be nice to save the resulting free list in the MarkedBlock, so we don't have to reproduce it later.

Okiedokie.

> (4) It's not so good to fire every 100ms. If a given sweep operation took a long time, we could saturate the timer interval. I think it would be better to sweep the minimum possible in each interval -- one block -- and, after we're done, then schedule the next timer relative to the current time, with an interval equal to however long we took * constant.

Okay. Perhaps the constant should be based on how quickly we want to give memory back? It's probably difficult to gauge how much memory we actually free during each sweep though :-(

I also tested 1 block/100 ms and it took quite a while (~3 minutes) to give all our memory back in high-load situations (e.g. > 1 GB of dead stuff).

[Early Warning System Bot]

Comment on attachment 144651 [details]
Patch

Attachment 144651 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12850633

[Geoffrey Garen]

Comment on attachment 144651 [details]
Patch

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

> Source/JavaScriptCore/heap/Heap.cpp:711
> +    m_objectSpace.didFinishMarking();
> +    m_sweeper->didFinishMarking();

I'd prefer to see a more explicit interface: m_sweeper->startSweeping(m_objectSpace.copyToVector()).

> Source/JavaScriptCore/heap/Heap.cpp:794
> +    m_bytesAbandoned = 0;

I tend to think that any GC should recent m_bytesAbandoned to 0, no?

> Source/JavaScriptCore/heap/MarkedSpace.cpp:239
> +    unsigned blocksSwept = 0;
> +    for (; m_currentBlockToSweepIndex < m_blocksToSweep.size(); m_currentBlockToSweepIndex++) {

I think it would make more sense to put this state and functionality into the incremental sweeper.

[Geoffrey Garen]

> > (1) Is it OK for the sweep timer to compete against the GC timer? 
> 
> I'm not sure what you mean by "compete". Do you mean does the order in which they run matter?

Yeah -- like, is it OK that they race? I guess there's no correctness harm. A GC will reset the sweeper.

> > (2) Should the incremental sweeper free empty blocks?
> 
> I think it should, but I was going to add that when we can also do finalization incrementally so that we sweep, finalize, then free. I guess we could technically add freeing blocks to this patch though. 

Separate patch is fine.

> > (3) Would be nice to save the resulting free list in the MarkedBlock, so we don't have to reproduce it later.
> 
> Okiedokie.

Separate patch is fine for this, too.

> > (4) It's not so good to fire every 100ms. If a given sweep operation took a long time, we could saturate the timer interval. I think it would be better to sweep the minimum possible in each interval -- one block -- and, after we're done, then schedule the next timer relative to the current time, with an interval equal to however long we took * constant.
> 
> Okay. Perhaps the constant should be based on how quickly we want to give memory back? It's probably difficult to gauge how much memory we actually free during each sweep though :-(
> 
> I also tested 1 block/100 ms and it took quite a while (~3 minutes) to give all our memory back in high-load situations (e.g. > 1 GB of dead stuff).

I think some resolution to this is a blocker to landing, since this will be a regression over the current GC timer in some cases.

I'd be interested to know how a proportional time heuristic worked in practice -- e.g., 1% CPU time instead of 1 block / 100 ms. We can always up the % if we think it's valuable, but we don't want an absolute time tick (100ms) because that can regress to 100% CPU time, which is no advantage over the status quo.

[Mark Hahnenberg]

Created attachment 144939 [details]
Patch

[Mark Hahnenberg]

Created attachment 144941 [details]
Patch

[Geoffrey Garen]

Comment on attachment 144941 [details]
Patch

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.

[Geoffrey Garen]

Comment on attachment 144941 [details]
Patch

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

> Source/JavaScriptCore/heap/Heap.cpp:706
> -        m_objectSpace.sweep();
>          m_objectSpace.shrink();

This is a memory leak: You can't shrink without sweeping first. Pending destructors will not have run.

[Mark Hahnenberg]

Landed in http://trac.webkit.org/changeset/119028.