Bug 145773 – Allow one sync GC per gcTimer interval on critical memory pressure warning

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 254539 [details]
Patch

[Chris Dumez]

Created attachment 254541 [details]
Patch

[Darin Adler]

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?

[Chris Dumez]

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().

[Geoffrey Garen]

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.

[Chris Dumez]

Created attachment 254600 [details]
Patch

[Chris Dumez]

(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.

[Geoffrey Garen]

Comment on attachment 254600 [details]
Patch

r=me

[Andreas Kling]

v.nice

[WebKit Commit Bot]

Comment on attachment 254600 [details]
Patch

Clearing flags on attachment: 254600

Committed r185387: <http://trac.webkit.org/changeset/185387>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

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?

[Chris Dumez]

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().