RFR: 8245511: G1 adaptive IHOP does not account for reclamation of humongous objects by young GC

[Luo, Ziyi]

Hi Stefan,

On 8/19/20, 11:47 AM, Stefan Johansson wrote:

> Hi Ziyi,
>
>On 2020-08-19 19:04, Luo, Ziyi wrote:
>> Hi Stefan,
>>
>> Thank you for looking into this patch.
>>
>> On 8/19/20, 4:10 AM, Stefan Johansson wrote:
>>
>>> Hi Ziyi,
>>>
>>> Thanks for finding this issue and fixing it. The idea for the fix is
>>> very easy, take eager reclaim into consideration when deciding the
>>> allocation rate for old. Reading the patch and fully understand the
>>> calculations was a bit harder. Trying to explain what I would like to
>>> see to make it easier to follow is hard. So I've made a proposal that
>>> can be applied upon your changes.
>>>
>>> Inc: http://cr.openjdk.java.net/~sjohanss/8245511/rev1.inc/
>>> Full: http://cr.openjdk.java.net/~sjohanss/8245511/rev1.full/
>>>
>>> Hopefully I did not miss anything that your patch covers that is missing
>>> from this proposal. I'll summarize what I did:
>>> * Keep track of humongous and non-humongous allocations independently to
>>> simplify calculations in reset_after_gc().
>>> * Calculate the old generation growth in reset_for_gc(), this avoids
>>> saving an additional value and if I'm not missing anything we have all
>>> information we need at this point.
>>
>> I really don't like the name humongous_bytes_after_penultimate_gc. Thank you
>> for helping me get rid of it :)
>
> That name was the trigger for me to look deeper into an alternative
> approach :)
>
>>
>>> * The growth is calculated by adding the non-humongous allocations
>>> during the last mutator period with a potential increase in humongous size.
>>> * Added _gen to two members to make it clear this is the whole old
>>> generation not only old regions.
>>>
>>> Does these changes all sound good? Let me know what you think.
>>
>> I like how you reshaped the code structure especially how you deconstructed
>> the net_survived_old_bytes in a clearer fashion. This saves a lot of comments
>> to explain the net survived old bytes calculation (now
>> last_period_old_gen_growth). The change looks good to me.
> 
> Thanks, do you need a sponsor or will you handle that internally at
> Amazon? You can consider this reviewed by me, but we should wait for
> Thomas to take a final look as well.

Thomas agreed to sponsor this patch for me. Let's leave this for his final 
review.

Thanks,
Ziyi