Bug 171794 – GCController::garbageCollectNow() should be async

• Status: RESOLVED
• Resolution: LATER
• Component: WebCore JavaScript

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 309328 [details]
the patch

[Build Bot]

Comment on attachment 309328 [details]
the patch

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

New failing tests:
accessibility/accessibility-node-memory-management.html
svg/animations/smil-leak-list-property-instances.svg
svg/animations/smil-leak-dynamically-added-element-instances.svg
fast/dom/gc-dom-tree-lifetime.html
svg/animations/smil-leak-elements.svg
svg/animations/smil-leak-element-instances-noBaseValRef.svg
fast/workers/dedicated-worker-lifecycle.html
svg/animations/smil-leak-element-instances.svg
plugins/refcount-leaks.html

[Build Bot]

Created attachment 309329 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 309328 [details]
the patch

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

New failing tests:
accessibility/accessibility-node-memory-management.html
svg/animations/smil-leak-list-property-instances.svg
svg/animations/smil-leak-dynamically-added-element-instances.svg
fast/dom/gc-dom-tree-lifetime.html
svg/animations/smil-leak-elements.svg
svg/animations/smil-leak-element-instances-noBaseValRef.svg
fast/workers/dedicated-worker-lifecycle.html
svg/animations/smil-leak-element-instances.svg

[Build Bot]

Created attachment 309330 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Filip Pizlo]

Those failures are "real" - I need to make the debugging GC calls in our tests do a sync GC.

[Build Bot]

Comment on attachment 309328 [details]
the patch

Attachment 309328 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3695552

New failing tests:
svg/animations/smil-leak-list-property-instances.svg
svg/animations/smil-leak-dynamically-added-element-instances.svg
fast/dom/gc-dom-tree-lifetime.html
svg/animations/smil-leak-elements.svg
svg/animations/smil-leak-element-instances-noBaseValRef.svg
fast/workers/dedicated-worker-lifecycle.html
svg/animations/smil-leak-element-instances.svg

[Build Bot]

Created attachment 309332 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Filip Pizlo]

Created attachment 309334 [details]
the patch

Makes tests still use sync GCs

[Andreas Kling]

Comment on attachment 309334 [details]
the patch

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

> Source/WebCore/page/MemoryRelease.cpp:99
>      if (synchronous == Synchronous::Yes) {
> -        GCController::singleton().garbageCollectNow();
> +        GCController::singleton().garbageCollectNowAsync();

This doesn't read quite right (if wantSync then doAsync).
Will this work correctly when we're here because the process/app is suspending (iOS) and we have limited time to shrink as small as possible?

[Filip Pizlo]

(In reply to Andreas Kling from comment #10)
> Comment on attachment 309334 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=309334&action=review
> 
> > Source/WebCore/page/MemoryRelease.cpp:99
> >      if (synchronous == Synchronous::Yes) {
> > -        GCController::singleton().garbageCollectNow();
> > +        GCController::singleton().garbageCollectNowAsync();
> 
> This doesn't read quite right (if wantSync then doAsync).
> Will this work correctly when we're here because the process/app is
> suspending (iOS) and we have limited time to shrink as small as possible?

The GC itself won't be any slower, but the runloop will yield during long GCs. 

The incremental sweep will be slower than the synchronous sleep, which might be a bad thing. 

Let's consider the terminology separately. It seems that this code uses "asynchronous" to mean something different than what it means in JSC. I think this uses asynchronous to mean that it doesn't want the GC that badly. Eventually we might want to use some other terminology here.

[Filip Pizlo]

(In reply to Filip Pizlo from comment #11)
> (In reply to Andreas Kling from comment #10)
> > Comment on attachment 309334 [details]
> > the patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=309334&action=review
> > 
> > > Source/WebCore/page/MemoryRelease.cpp:99
> > >      if (synchronous == Synchronous::Yes) {
> > > -        GCController::singleton().garbageCollectNow();
> > > +        GCController::singleton().garbageCollectNowAsync();
> > 
> > This doesn't read quite right (if wantSync then doAsync).
> > Will this work correctly when we're here because the process/app is
> > suspending (iOS) and we have limited time to shrink as small as possible?
> 
> The GC itself won't be any slower, but the runloop will yield during long
> GCs. 
> 
> The incremental sweep will be slower than the synchronous sleep, which might
> be a bad thing. 
> 
> Let's consider the terminology separately. It seems that this code uses
> "asynchronous" to mean something different than what it means in JSC. I
> think this uses asynchronous to mean that it doesn't want the GC that badly.
> Eventually we might want to use some other terminology here.

Thought about this more.  I don't think that this patch is right.  The only places where it uses the Async thing are the notifications we get that require us to GC ASAP.

To do this right we'd need a much better story for sweeping.

[Sam Weinig]

Comment on attachment 309334 [details]
the patch

Removing review request to remove from review queue.