RFR (S): 8245087: Use ratios instead of percentages in G1HeapSizingPolicy::expansion_amount

[Thomas Schatzl]

Hi Stefan,

   thanks for your review.

On 18.05.20 15:34, stefan.johansson at oracle.com wrote:
> Hi Thomas,
> 
> On 2020-05-18 10:15, Thomas Schatzl wrote:
>> Hi all,
>>
>>    can I have reviews for this change that changes internal 
>> calculations to use ratios (i.e. values between 0-1) instead of 
>> percentages (0-100)?
>>
>> While in this case use of percent or ratios cancels itself out I 
>> prefer to use ratios for calculations and if needed percent for 
>> displaying.
>>
>> There is just no need to scale the ratios we have with 100, apparently 
>> done to make the printing code simpler, but I really prefer it the 
>> other way.
>>
>> In the log message I also unified the code to print the "%" sign 
>> without whitespace before (there has been a mix of that).
>>
>> Based on 8245086 also out for review.
>>
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8245087
>> Webrev:
>> http://cr.openjdk.java.net/~tschatzl/8245087/webrev
> I like the way you've split up these cleanup, very nice to review :)
> 
> This looks good in general, a few small suggestions:
> src/hotspot/share/gc/g1/g1HeapSizingPolicy.cpp
> ---
> 53 double G1HeapSizingPolicy::scale_with_heap(double pause_time_ratio) {
> ...
> 82   const double pause_time_ratio = 1.0 / (1.0 + GCTimeRatio);
> 83
> 84   double threshold = scale_with_heap(pause_time_ratio);
> 
> Not sure I have better suggestions, but the naming here was a bit 
> misleading at first. I though about changing the function name, but I 
> think naming the variable/parameter pause_time_threshold would make it 
> clearer. Do you agree?

Done.

> ---
> 
> In the scale-function, we could add some debug/trace logging that the 
> threshold was scaled.

The logging added in 8245088 adds the current scaled threshold.

> ---
> 
> No need for a re-reivew and if you don't agree, leave as is.

New webrevs:
http://cr.openjdk.java.net/~tschatzl/8245087/webrev.0_to_1/ (diff)
http://cr.openjdk.java.net/~tschatzl/8245087/webrev.1 (full)

Thanks,
   Thomas