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

[stefan.johansson at oracle.com]

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.
* 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 tried running your repro from the bug-report and it seem to be 
working, but please try this out and make sure it works in your 
use-cases as well.

Our internal testing looks good so far.

Thanks,
Stefan

On 2020-08-14 17:53, Luo, Ziyi wrote:
> Hi Thomas,
> 
> Thank you for your review.
> 
> May I have a second reviewer for this patch?
> 
>> On 8/14/20, 3:56 AM, Thomas Schatzl wrote:
>>
>> On 13.08.20 23:31, Luo, Ziyi wrote:
>>> Hi Thomas,
>>>
>>> Thanks for your comments. New revision published:
>>> http://cr.openjdk.java.net/~bmathiske/8245511/webrev.03/
>>> Inc:
>>> http://cr.openjdk.java.net/~bmathiske/8245511/webrev.02_to_03/
>>>
>>> On 8/12/20, 2:15 AM, Thomas Schatzl wrote:
>>>
>> [...]
>>>> Please try to provide an incremental webrev too so that we reviewers do
>>>> not have to look through all changes all the time. It's easy to do (and
>>>> as soon as we're on github they will be created automatically):
>>>>
>>>> ...
>>>>
>>>> upload both webrev.2_to_3 and webrev.3 .
>>>
>>> I appreciate this mercurial tip, it is very helpful. Even though I may not
>>> need it anymore :)
>>>
>>>> Also, this is your second patch, isn't it? I can sponsor this one, but
>>>> please apply for authorship after that :)
>>>
>>> Thank you for your sponsorship :) I will apply for authorship after this.
>>>
>>>>>>> I will do some perf checking.
>>>>>>
>>>>>> Please let me know if you have any findings. Thank you!
>>>>>
>>>>> All good with our standard benchmarks. I forgot to do regression testing
>>>>> (tier1-5) yesterday, is running with the new patch. I'll shout if
>>>>> there's an issue.
>>>>
>>>> All good afaics.
>>>
>>> Great, thanks for checking.
>>>
>>
>> lgtm. Thanks.
> 
> Best,
> Ziyi
>