Bug 171690 - JSC::Heap should expose a richer API for requesting GCs
Summary: JSC::Heap should expose a richer API for requesting GCs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (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-04 14:11 PDT by Filip Pizlo
Modified: 2017-05-04 16:38 PDT (History)
7 users (show)

See Also:


Attachments
the patch (44.33 KB, patch)
2017-05-04 14:18 PDT, Filip Pizlo
fpizlo: review+
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-04 14:11:23 PDT
In the process of experimenting with https://bugs.webkit.org/show_bug.cgi?id=171689, I found the need to do these things:

- Request that JSC perform some action after GC finishes.
- Request that incremental sweeping releases fastMalloc memory after it finishes.
- Request async collections that run immediately if possible.
- Request sync collections that synchronously sweep.

This bug is about introducing some nicer APIs to do all of these things.
Comment 1 Filip Pizlo 2017-05-04 14:18:20 PDT
Created attachment 309101 [details]
the patch
Comment 2 Geoffrey Garen 2017-05-04 15:21:12 PDT
Comment on attachment 309101 [details]
the patch

r=me
Comment 3 Saam Barati 2017-05-04 15:24:49 PDT
Comment on attachment 309101 [details]
the patch

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

> Source/JavaScriptCore/heap/Heap.cpp:984
> +    } }

style nit: I think there should be a newline between braces here.

> Source/JavaScriptCore/heap/Heap.cpp:997
> +            if (request.subsumedBy(request)) {

This confuses me. Why are we calling subsumedBy always passing in the self object?

> Source/JavaScriptCore/heap/Synchronousness.h:30
> +enum Synchronousness {

style nit: How about an enum class here since this only used in a handful of locations
Comment 4 Saam Barati 2017-05-04 15:25:04 PDT
Sorry did not mean to clear Geoff's r+
Comment 5 Filip Pizlo 2017-05-04 16:16:00 PDT
Comment on attachment 309101 [details]
the patch

r=ggaren+saam
Comment 6 Filip Pizlo 2017-05-04 16:16:30 PDT
(In reply to Saam Barati from comment #3)
> Comment on attachment 309101 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=309101&action=review
> 
> > Source/JavaScriptCore/heap/Heap.cpp:984
> > +    } }
> 
> style nit: I think there should be a newline between braces here.
> 
> > Source/JavaScriptCore/heap/Heap.cpp:997
> > +            if (request.subsumedBy(request)) {
> 
> This confuses me. Why are we calling subsumedBy always passing in the self
> object?

oops.  That's a bug!  I'll test the fix.

> 
> > Source/JavaScriptCore/heap/Synchronousness.h:30
> > +enum Synchronousness {
> 
> style nit: How about an enum class here since this only used in a handful of
> locations
Comment 7 Filip Pizlo 2017-05-04 16:38:21 PDT
Landed in http://trac.webkit.org/changeset/216218/webkit