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

[sangheon.kim at oracle.com]

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.

> ------------------------------------------------------------------------------
> 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.
Done.

> ------------------------------------------------------------------------------
>
> [discussed this offline with Sangheon and Thomas; mentioning it here
> in case anyone else is following email.]
I discussed with Kim and Thomas. And we decided not to pursue this nTAMS 
idea.
nTAMS is used in many locations and currently it is updated mostly at 
the start/end of marking.
And changing the value conditionally seems riskier. So decided to use 
current approach which is more localized.

> "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() ?
Done.

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

> 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?
N/A.

http://cr.openjdk.java.net/~sangheki/8220089/webrev.3
http://cr.openjdk.java.net/~sangheki/8220089/webrev.3_inc/
Testing: hs-tier1 ~ 5, tier7-gc

Thanks,
Sangheon

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20190515/c771c715/attachment.htm>