Bug 171690 – JSC::Heap should expose a richer API for requesting GCs

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

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 309101 [details]
the patch

[Geoffrey Garen]

Comment on attachment 309101 [details]
the patch

r=me

[Saam Barati]

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

[Saam Barati]

Sorry did not mean to clear Geoff's r+

[Filip Pizlo]

Comment on attachment 309101 [details]
the patch

r=ggaren+saam

[Filip Pizlo]

(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

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/216218/webkit