WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(20.31 KB, patch)
2012-05-30 15:28 PDT
,
Mark Hahnenberg
no flags
Details
Formatted Diff
Diff
Patch
(18.56 KB, patch)
2012-05-30 15:31 PDT
,
Mark Hahnenberg
ggaren
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 144651
[details]
Patch
Early Warning System Bot
Comment 3
2012-05-29 17:46:11 PDT
Comment on
attachment 144651
[details]
Patch
Attachment 144651
[details]
did not pass qt-ews (qt): Output:
http://queues.webkit.org/results/12853397
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
Comment on
attachment 144651
[details]
Patch
Attachment 144651
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/12838783
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
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 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
Created
attachment 144939
[details]
Patch
Mark Hahnenberg
Comment 11
2012-05-30 15:31:36 PDT
Created
attachment 144941
[details]
Patch
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
Landed in
http://trac.webkit.org/changeset/119028
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug