RFR (M): 8027959: Investigate early reclamation of large objects in G1

[Thomas Schatzl]

Hi Mikael,

  thanks for the review again,

On Tue, 2014-07-22 at 13:28 +0200, Mikael Gerdin wrote:
> Hi Thomas,
> 
> On Monday 21 July 2014 15.20.57 Thomas Schatzl wrote:
> > Hi Mikael,
> > 
> >   thanks for the review, and finding these issues!
> > 
> > On Thu, 2014-07-17 at 16:52 +0200, Mikael Gerdin wrote:
> > > Hi Thomas,
> > > 
> > > On Tuesday 15 July 2014 11.10.53 Thomas Schatzl wrote:
> > > > Hi all,
> > > > 
> > > >   could I have reviews for the following change that allows G1 to
> > > > 
> > > > eagerly/early reclaim humongous objects on every GC?
> > > > 
> > > > Problem:
> > > > 
> > > > In G1 large objects are always allocated in the old generation,
> > > > currently requiring a complete heap liveness analysis (full gc, marking)
> > > > to reclaim them.
> > > > [...]
[...]
> > Diff to last:
> > http://cr.openjdk.java.net/~tschatzl/8027959/webrev.0_to_1
> > Complete:
> > http://cr.openjdk.java.net/~tschatzl/8027959/webrev.1
> 
> I had a look through the entirety of webrev.1 again.
> 
> g1CollectedHeap.hpp:
> The comment before G1FastCSetBiasedMappedArray about failing TLAB allocations 
> is no longer relevant, since the code has been changed to explicitly handle 
> IsHumongous, right?.

Fixed.

> 
> The enum in_cset_state_t seems like it should live in G1CollectedHeap instead 
> of in the scope of the datastructure used to maintain the information.
> Not only will that reduce the length of 
> G1FastCSetBiasedMappedArray::InCSet to
> G1CollectedHeap::InCSet
> but I think that callers of G1CollectedHeap::in_cset_state should not be 
> required to know that it is in fact backed by a G1FastCSetBIasedMappedArray 
> and that the values' enum is scoped inside that class.

Fixed.

> g1CollectedHeap.cpp:
> line 5535 contains trailing white space

Fixed.

> 
> TestEagerReclaimHumongousRegions.java:
> The test looks good to me, but as you know anything may happen in the testing 
> lab :)

It's the best what I could come up with.

> 
> I think the rest of the change looks fine.

Diff to last:
http://cr.openjdk.java.net/~tschatzl/8027959/webrev.1_to_2
Complete:
http://cr.openjdk.java.net/~tschatzl/8027959/webrev.2

Thanks,
  Thomas