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

[Kim Barrett]

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.
> 
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8048112/webrev/
> 
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8048112
> 
> Testing:
> JPRT, many full ad-hoc runs with JDK-8038423
> 
> 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.

----------

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.

----------

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() ?

----------

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().