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

[Mikael Gerdin]

Hi Thomas,

On Tuesday 22 July 2014 16.26.46 Thomas Schatzl wrote:
> 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

Looks good.
/Mikael

> 
> Thanks,
>   Thomas