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

[Kim Barrett]

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

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1AllocRegion.hpp

 270   SurvivorGCAllocRegion(G1EvacStats* stats)
 271   : G1GCAllocRegion("Survivor GC Alloc Region", false /* bot_updates */, stats, InCSetState::Young) { }
 272   static bool should_retain(HeapRegion* region);

Maybe add a blank line before should_retain declaration.

------------------------------------------------------------------------------

[discussed this offline with Sangheon and Thomas; mentioning it here
in case anyone else is following email.]

"root regions" representation was changed from an array of HeapRegion*
to an array of HeapWord*, where the values are the captured "top"
values from the corresponding regions.  This was done because later
allocation in survivor regions would change top, and we don't want
that changed value.

It seems like it might be easier and simpler to retain the existing
representation, but capture the desired "top" value in NTAMS. Then in
scan_root_region, for survivor regions scan from bottom to NTAMS. That
means scan_root_region is different for old/survivor regions. But
that's kind of already true; it's a bit of a hack that the current
scan from NTAMS to top works for both.

This also requires looking at and possibly adjusting other uses of NTAMS.

------------------------------------------------------------------------------

Consider renaming
G1CollectedHeap::survivor_convert_to_eden() => convert_survivor_to_eden() ?

------------------------------------------------------------------------------

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.

I also dislike some of the accounting kludges around retained regions,
such as for used_bytes.  Maybe the above discussed use of NTAMS might
provide information that could simplify the accounting?

------------------------------------------------------------------------------