RESOLVED FIXED 85429
Heap should sweep incrementally
https://bugs.webkit.org/show_bug.cgi?id=85429
Summary Heap should sweep incrementally
Mark Hahnenberg
Reported 2012-05-02 16:09:28 PDT
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.
Attachments
Patch (20.42 KB, patch)
2012-05-29 17:14 PDT, Mark Hahnenberg
no flags
Patch (20.31 KB, patch)
2012-05-30 15:28 PDT, Mark Hahnenberg
no flags
Patch (18.56 KB, patch)
2012-05-30 15:31 PDT, Mark Hahnenberg
ggaren: review+
Mark Hahnenberg
Comment 1 2012-05-02 16:12:12 PDT
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
Comment 2 2012-05-29 17:14:51 PDT
Early Warning System Bot
Comment 3 2012-05-29 17:46:11 PDT
Geoffrey Garen
Comment 4 2012-05-29 17:52:00 PDT
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 5 2012-05-29 17:55:31 PDT
Mark Hahnenberg
Comment 6 2012-05-29 18:07:10 PDT
(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 7 2012-05-29 18:16:32 PDT
Geoffrey Garen
Comment 8 2012-05-29 19:25:53 PDT
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
Comment 9 2012-05-29 19:34:31 PDT
> > (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
Comment 10 2012-05-30 15:28:07 PDT
Mark Hahnenberg
Comment 11 2012-05-30 15:31:36 PDT
Geoffrey Garen
Comment 12 2012-05-30 17:05:57 PDT
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 13 2012-05-30 17:14:52 PDT
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
Comment 14 2012-05-30 20:05:40 PDT
Note You need to log in before you can comment on or make changes to this bug.