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

[Luo, Ziyi]

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:

> So the code passes a reference to an uninitialized variable to the
> _ihop_control constructor, which is exactly the problem :). It is
> cleaner if _old_gen_alloc_tracker were located somewhere before
> _ihop_control in the initializer list. This means you need to shuffle
> the class members a bit too.

Got it. Done

> I would prefer a method in G1OldGenAllocationTracker that returned the
> recent survived old gen bytes (saturated at the bottom to 0) instead of
> the friend declaration.
> 
> ...
> 
> something like
> 
> double G1AdaptiveIHOPControl::last_mutator_period_old_allocation_rate()
> const {
>    return _old_gen_alloc_tracker->last_period_net_survived_old_bytes() /
> _last_allocation_time_s;
> }
> 
> and move all that calculation into the old gen allocation tracker. THis
> would afaics avoid the friend declaration and be cleaner.

Good point. Moved the net survived old byte calculation to 
G1OldGenAllocationTracker.

> Some pre-existing issue, please fix while there: there is a missing
> whitespace between the closing parenthesis and the curly brace.
> 
> G1IHOPControl* G1Policy::create_ihop_control(const
> G1OldGenAllocationTracker* old_gen_alloc_tracker,
>                                               const G1Predictions*
> predictor){
>           ^^^

Done

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

Best,
Ziyi