Bug 171794 - GCController::garbageCollectNow() should be async
Summary: GCController::garbageCollectNow() should be async
Status: RESOLVED LATER
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks: 171689
  Show dependency treegraph
 
Reported: 2017-05-07 15:25 PDT by Filip Pizlo
Modified: 2017-05-10 07:14 PDT (History)
12 users (show)

See Also:


Attachments
the patch (2.35 KB, patch)
2017-05-07 15:27 PDT, Filip Pizlo
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (846.71 KB, application/zip)
2017-05-07 16:32 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.19 MB, application/zip)
2017-05-07 16:38 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (9.66 MB, application/zip)
2017-05-07 17:36 PDT, Build Bot
no flags Details
the patch (8.48 KB, patch)
2017-05-07 17:49 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2017-05-07 15:25:26 PDT
Patch forthcoming.
Comment 1 Filip Pizlo 2017-05-07 15:27:03 PDT
Created attachment 309328 [details]
the patch
Comment 2 Build Bot 2017-05-07 16:32:52 PDT
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
Comment 3 Build Bot 2017-05-07 16:32:53 PDT
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
Comment 4 Build Bot 2017-05-07 16:38:24 PDT
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
Comment 5 Build Bot 2017-05-07 16:38:26 PDT
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
Comment 6 Filip Pizlo 2017-05-07 16:52:14 PDT
Those failures are "real" - I need to make the debugging GC calls in our tests do a sync GC.
Comment 7 Build Bot 2017-05-07 17:36:06 PDT
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
Comment 8 Build Bot 2017-05-07 17:36:07 PDT
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
Comment 9 Filip Pizlo 2017-05-07 17:49:49 PDT
Created attachment 309334 [details]
the patch

Makes tests still use sync GCs
Comment 10 Andreas Kling 2017-05-08 05:58:52 PDT
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?
Comment 11 Filip Pizlo 2017-05-08 06:57:02 PDT
(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.
Comment 12 Filip Pizlo 2017-05-08 10:27:09 PDT
(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.
Comment 13 Sam Weinig 2017-05-10 07:14:36 PDT
Comment on attachment 309334 [details]
the patch

Removing review request to remove from review queue.