Bug 171690

Summary: JSC::Heap should expose a richer API for requesting GCs
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: ggaren, jfbastien, keith_miller, mark.lam, msaboff, saam, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 171689    
Attachments:
Description Flags
the patch fpizlo: review+

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