RFR(M): 8220089: G1 wastes a significant amount of space in survivor regions

[Thomas Schatzl]

Hi Sangheon,

On Wed, 2019-05-15 at 15:08 -0700, sangheon.kim at oracle.com wrote:
> Hi Kim,
> 
> Thanks for reviewing this.
> 
> On 5/8/19 9:42 PM, Kim Barrett wrote:
> > > On Apr 30, 2019, at 2:06 AM, sangheon.kim at oracle.com wrote:
> > > 
> > > Hi all,
> > > 
> > > I got some comments from Thomas Schatzl, mostly about comment and
> > > assert changes. He also suggested to investigate combining
> > > current 2 steps of handling retaining survivor region into 1
> > > step, so I filed JDK-8223106.
> > > 
> > > Webrev: 
> > > http://cr.openjdk.java.net/~sangheki/8220089/webrev.2/
> > > http://cr.openjdk.java.net/~sangheki/8220089/webrev.2_inc/
> > > Testing: hs-tier1
> > 
> > -----------------------------------------------------------------
> > -------------
> > src/hotspot/share/gc/g1/g1AllocRegion.cpp
> > 367 bool SurvivorGCAllocRegion::should_retain(HeapRegion* region) {
> >  368   if (region == NULL || region->free() < MinTLABSize) {
> >  369     return false;
> >  370   }
> >  371   return true;
> >  372 }
> > 
> > Why does should_retain accept a NULL region?
>  Removed NULL check at should_retain() and added NULL check before
> calling should_retain.
> G1AllocRegion::release() which used as an input of should_retain()
> may return NULL.

Could you change this to "return region != NULL && region->free() >=
MinTLABSize;" ?

> > 
> > I dislike the "retained" region nomenclature for survivors. I
> > didn't notice an explanation of it, and it's not obvious why that
> > name was chosen or how it relates to how these regions are used.
> > But I don't have a good suggestion for an alternative yet. It seems
> > like that name was copied from MutatorAllocRegion, but I'm not sure
> > the survivor case is really that similar.
>  Yeah, I agree that it is not that similar. But as I don't have
> better name, I copied from mutator case. Initially I named 'reuse'
> but seems not better than 'retain'. Unless there is a better naming
> suggestion, I would like to proceed as is.

That may be revisited, and may go away with the CR filed by Sangheon to
convert all survivors to eden at the end of gc.

- there is an extra newline at the end of g1SurvivorRegions.cpp.

Looks good otherwise.

Thanks,
  Thomas