Bug 65661 - JSC GC does not coalesce blocks during minor collections
Summary: JSC GC does not coalesce blocks during minor collections
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-03 20:03 PDT by Filip Pizlo
Modified: 2011-12-21 14:29 PST (History)
5 users (show)

See Also:


Attachments
the patch (work in progress) (20.59 KB, patch)
2011-08-03 20:20 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (work in progress, fix style) (20.58 KB, patch)
2011-08-03 22:52 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (20.60 KB, patch)
2011-08-04 14:59 PDT, Filip Pizlo
fpizlo: review+
fpizlo: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2011-08-03 20:03:25 PDT
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.
Comment 1 Filip Pizlo 2011-08-03 20:20:01 PDT
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.
Comment 2 WebKit Review Bot 2011-08-03 20:22:31 PDT
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.
Comment 3 Filip Pizlo 2011-08-03 22:52:06 PDT
Created attachment 102877 [details]
the patch (work in progress, fix style)

Still testing...
Comment 4 Filip Pizlo 2011-08-03 23:22:42 PDT
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.
Comment 5 Oliver Hunt 2011-08-04 13:13:34 PDT
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?
Comment 6 Filip Pizlo 2011-08-04 13:25:54 PDT
(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. :-)
Comment 7 Filip Pizlo 2011-08-04 14:59:13 PDT
Created attachment 102987 [details]
the patch
Comment 8 Filip Pizlo 2011-08-04 15:23:19 PDT
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.
Comment 9 Eric Seidel (no email) 2011-09-06 15:28:39 PDT
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.
Comment 10 Filip Pizlo 2011-10-12 16:04:15 PDT
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.
Comment 11 Eric Seidel (no email) 2011-12-21 14:29:58 PST
Attachment 102987 [details] was posted by a committer and has review+, assigning to Filip Pizlo for commit.