Bug 65661 – JSC GC does not coalesce blocks during minor collections

• Status: NEW
• Component: JavaScriptCore

[Filip Pizlo]

The JSC GC uses a segregated-free list, big-bag-of-pages strategy, where separate pages (internally called blocks) are used for separate classes of object size.  Ordinarily, such a strategy implies that when a page (block in our case) becomes entirely vacant, it is disassociated with its size class so that any size class may use it.  This minimizes fragmentation, since it means that if an application changes its mind about what sizes to allocate then in the common case it will be able to reuse space previously used by the different sizes.  As well, this GC strategy also typically puts priority on filling in fragmented pages during early post-GC allocations, reverting to filling entirely-vacant pages only after the fragmented ones are fully occupied.  This can be thought of as a kind of best-fit, and in practice means that vacant pages are held in reserve for as long as possible in case the application changes its mind about what sizes to allocation late in the current mutation cycle.

The JSC GC uses exactly this strategy during full (eager-sweep) garbage collections.  But lazy-sweep collections - the ones triggered by heap exhaustion during allocation - do not do this.  They keep entirely-vacant blocks associated with whatever size class they were initialized for, and allocate into blocks in no particular order.  

The JSC GC should disassociate blocks from size classes when they become vacant, even on a lazy sweep garbage collection.

[Filip Pizlo]

Created attachment 102870 [details]
the patch (work in progress)

Tests still running.  It would be good if I could get a confirmation that the performance is not regressed, and that memory usage is good, since this patch does a lot of damage to the sweeping code.

[WebKit Review Bot]

Attachment 102870 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/heap/NewSpace.h:174:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/heap/NewSpace.h:176:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 102877 [details]
the patch (work in progress, fix style)

Still testing...

[Filip Pizlo]

Comment on attachment 102877 [details]
the patch (work in progress, fix style)

This appears to pass tests.  I'd still like to get a confirmation that the performance and memory usage is in good shape but other than that, it's good to review.

[Oliver Hunt]

Comment on attachment 102877 [details]
the patch (work in progress, fix style)

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

> Source/JavaScriptCore/ChangeLog:50
> +        objects are 64MB, this change is not currently expected to be a big

64k maybe?

[Filip Pizlo]

(In reply to comment #5)
> (From update of attachment 102877 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102877&action=review
> 
> > Source/JavaScriptCore/ChangeLog:50
> > +        objects are 64MB, this change is not currently expected to be a big
> 
> 64k maybe?

Good point. :-)

[Filip Pizlo]

Created attachment 102987 [details]
the patch

[Filip Pizlo]

I'm going to postpone committing this patch until some more tests happen, since this patch does potentially risky things to the way we call destructors, and the way we reclaim memory.

[Eric Seidel (no email)]

Comment on attachment 102877 [details]
the patch (work in progress, fix style)

Cleared Oliver Hunt's review+ from obsolete attachment 102877 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Filip Pizlo]

Comment on attachment 102987 [details]
the patch

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

> Source/JavaScriptCore/heap/NewSpace.h:-179
> -                functor(block);

Just testing.

> Source/JavaScriptCore/heap/NewSpace.h:277
> +        sizeClass.currentBlock = block;

Yeah.  Testing testing 1-2-3.

[Eric Seidel (no email)]

Attachment 102987 [details] was posted by a committer and has review+, assigning to Filip Pizlo for commit.