On critical memory pressure warning, we currently call GCController::garbageCollectSoon(), which does not offer any guarantee on when the garbage collection will actually take place. On critical memory pressure, we need to free up memory as soon as possible to avoid getting killed so this is an issue. Also, the fact that we clear the PageCache on critical memory pressure means a GC would likely be useful, even it is did not do much work the last time the gcTimer fired. As a result, the proposal is to allow one synchronous GC per gcTimer interval on critical memory pressure warning. This makes us more responsive to critical memory pressure and avoids doing synchronous GCs too often.
Created attachment 254539 [details] Patch
Created attachment 254541 [details] Patch
Comment on attachment 254541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254541&action=review > Source/JavaScriptCore/heap/Heap.cpp:1422 > + collectAllGarbage(); What is the rationale for not setting m_didSyncGCRecently true here?
Comment on attachment 254541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254541&action=review >> Source/JavaScriptCore/heap/Heap.cpp:1422 >> + collectAllGarbage(); > > What is the rationale for not setting m_didSyncGCRecently true here? m_didSyncGCRecently is only reset to false when the m_fullActivityCallback timer fires so if this timer is disabled (m_fullActivityCallback is null) it will never be reset to false. Geoff can correct me but I don't think this (m_fullActivityCallback being null) normally happens in practice though as I don't see any call sites to JSDisableGCTimer().
r=me Perhaps it would be nice to refactor this to be a feature of the timer, rather than a feature of the Heap object. That would help clarify the behavior in the case where there is not timer.
Created attachment 254600 [details] Patch
(In reply to comment #5) > r=me > > Perhaps it would be nice to refactor this to be a feature of the timer, > rather than a feature of the Heap object. That would help clarify the > behavior in the case where there is not timer. I reuploaded another iteration doing this. Please take another look.
Comment on attachment 254600 [details] Patch r=me
v.nice
Comment on attachment 254600 [details] Patch Clearing flags on attachment: 254600 Committed r185387: <http://trac.webkit.org/changeset/185387>
All reviewed patches have been landed. Closing bug.
Comment on attachment 254600 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254600&action=review > Source/WebCore/bindings/js/GCController.cpp:101 > +#if USE(CF) > + JSLockHolder lock(JSDOMWindow::commonVM()); > + if (!JSDOMWindow::commonVM().heap.isBusy()) > + JSDOMWindow::commonVM().heap.collectAllGarbageIfNotDoneRecently(); > +#else > + garbageCollectSoon(); > +#endif Why is this different for USE(CF) and not? Heap::collectAllGarbageIfNotDoneRecently appears to be available for all ports so it seems they could both have the same implementation, is there a reason I'm overlooking?
Comment on attachment 254600 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254600&action=review >> Source/WebCore/bindings/js/GCController.cpp:101 >> +#endif > > Why is this different for USE(CF) and not? Heap::collectAllGarbageIfNotDoneRecently appears to be available for all ports so it seems they could both have the same implementation, is there a reason I'm overlooking? Maybe Heap::collectAllGarbageIfNotDoneRecently() should be behind USE(CF) then. What we do in Heap::collectAllGarbageIfNotDoneRecently(): if (wasGCdRecenty) garbageCollectSoon(); else garbageCollectNow(); Note that in Heap::collectAllGarbageIfNotDoneRecently(), we call reportAbandonedObjectGraph() to do the garbageCollectSoon(). However, if you look at the implementation of GCController::garbageCollectSoon(), it looks like: // We only use reportAbandonedObjectGraph on systems with CoreFoundation // since it uses a run-loop-based timer that is currently only available on // systems with CoreFoundation. If and when the notion of a run loop is pushed // down into WTF so that more platforms can take advantage of it, we will be // able to use reportAbandonedObjectGraph on more platforms. #if USE(CF) JSLockHolder lock(JSDOMWindow::commonVM()); JSDOMWindow::commonVM().heap.reportAbandonedObjectGraph(); #else garbageCollectOnNextRunLoop(); #endif So to make Heap::collectAllGarbageIfNotDoneRecently() work for !USE(CF), it looks like we would need to use a Timer instead of calling simply reportAbandonedObjectGraph().