RFR(XS): 8240591: G1HeapSizingPolicy attempts to compute expansion_amount even when at full capacity

[Ivan Walulya]

Thanks Stefan

> On 12 Mar 2020, at 18:58, Stefan Johansson <stefan.johansson at oracle.com> wrote:
> 
> Hi Ivan,
> 
>> 12 mars 2020 kl. 14:12 skrev Ivan Walulya <ivan.walulya at oracle.com>:
>> 
>> Please find updated webrev: http://cr.openjdk.java.net/~iwalulya/8240591/01/
> Looks good, and I agree on trace-level being good, just one minor thing you could fix before pushing:
> 
> 65                               recent_gc_overhead,_g1h->capacity());
> Please add a space after the comma. 

> 
> I can do the push iif you don’t already have a sponsor, given you get the second review.

Will make the changes and send to you for pushing after getting the second review.

> 
> Thanks, 
> Stefan
> 
> 
>> 
>>> On 10 Mar 2020, at 10:16, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
>>> 
>>> Hi,
>>> 
>>> On 05.03.20 11:33, Ivan Walulya wrote:
>>>> Hi all,
>>>> Please review a small modification for G1HeapSizingPolicy to return without  computing expansion_amount when heap is already at full capacity.
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8240591
>>>> Webrev: http://cr.openjdk.java.net/~iwalulya/8240591/00/
>>>> //Ivan
>>> 
>>> some minor (imo) comments to start a discussion:
>>> 
>>> - I (weakly) suggest to remove the unnecessary assert(GCTimeRatio > 0) or put it at the beginning of the method. The initialization already guarantees that it is > 0. Probably best to move to the very top of the method.
>>> 
>>> - I think I understand why the code clears the ratio check data, presumably to avoid the "windup" of the ratio check data being stuck into the maximum, and taking some time to wind down. I think this is a good idea, but I would prefer to just unconditionally clear the data - it does not seem time consuming, and makes the code a bit smaller.
>>> 
>>> - undecided on the log message: while it is informative, now you get a log message even if nothing changed. Maybe make it trace level? Others might chime in here with their opinions. Also given that often people set -Xms==-Xmx, this one seems to be a bit chatty at this level. I can see the point of it though.
>>> 
>>> So overall, I am good with the change but asking for opinions :)
>>> 
>>> Thanks,
>>> Thomas
>> 
>