RFR (S): 8048112: G1 Full GC needs to support the case when the very first region is not available
thomas.schatzl at oracle.com
Thu Jul 10 09:18:08 UTC 2014
thanks for the review. Comments and new webrev inline.
On Mon, 2014-07-07 at 17:24 -0400, Kim Barrett wrote:
> On Jul 7, 2014, at 9:53 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> > for support to uncommit regions (JDK-8038423: G1: Uncommit regions
> > within the heap) within the heap G1 Full GC needs to support the case
> > when the first region is not available (uncommitted).
> > Otherwise the VM simply crashes trying to move data into the
> > non-available region.
> > So to allow uncommit of regions within the heap, G1 Full GC should
> > correctly handle the case when the very first region is not available
> > (uncommitted).
> > The change is baesed on two ideas: lazily initialize the compaction
> > point during iteration of the list of heap regions - as the (serial)
> > heap iteration is in-order by definition and will never use
> > non-available regions. Further refactor the code to let the
> > G1CollectedHeap handle finding the next region to compact into as imo
> > this is the correct place to find that next region (as it "owns" the
> > region list at this time).
> > Without JDK-8038423 this is merely some imo reasonable refactoring.
> > Thanks,
> > Thomas
> Functionally looks ok.
>A few comments:
> I assume the added const qualifier for n_regions() is because there is
> some caller that now requires this. There are several nearby
> functions that look like they ought to be similarly qualified, but
> aren't, and that looks strange and confusing when first coming upon
> this code.
Done. Thanks for thinking of fitting code improvements beyond the
> I think next_compaction_region() would be more clearly written using
> an actual for-loop, rather than synthesizing a for-loop from a
> declaration, a while-loop, and an increment statement.
Okay :) I just copy&pasted it from previous code...
> In doHeapRegion() there was added a test for continuesHumongous().
> Wouldn't it be better to just test for isHumongous(), rather than
> separately testing for either of startsHumongous() or
> continuesHumongous() ?
That added test can be removed. ContinuesHumongous() is checked for at
the top of doHeapRegion(). I kept the original code.
> g1MarkSweep.cpp & space.hpp
> There appear to be only two places that construct a CompactPoint:
> - G1PrepareCompactClosure, which is being modified by this changeset
> to call new default constructor for CompactPoint.
> - GenCollectedHeap::prepare_for_compaction(). This one passes a
> generation, NULL, and NULL (?shouldn't that last NULL actually be
> Rather than adding a default constructor for CompactPoint, I think it
> would be better for G1PrepareCompactClosure to use the existing
> constructor. A possible cleanup would be to change that existing
> constructor to only take a generation argument, with space initialized
> to NULL and threshold initialized to 0, since (with this change set)
> no callers provide other values. This would also eliminate the NULL
> confusion in GenCollectedHeap::prepare_for_compaction().
Done. Thanks, nice.
A new webrev that incorporates all your ideas is available at
and just a webrev with the difference to the last is at
More information about the hotspot-gc-dev