RFR (S): 8048112: G1 Full GC needs to support the case when the very first region is not available

[Thomas Schatzl]

Hi Kim,

  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:
> 
> ----------
>
> g1CollectedHeap.hpp
> 
> 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
change.

> 
> ----------
> 
> g1CollectedHeap.hpp
> 
> 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...

> 
> ----------
> 
> g1CollectedHeap.cpp
> 
> 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
>   0?). 
> 
> 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

http://cr.openjdk.java.net/~tschatzl/8048112/webrev.1/

and just a webrev with the difference to the last is at

http://cr.openjdk.java.net/~tschatzl/8048112/webrev.0_to_1/

Thanks,
  Thomas