RFR (M): 8231579: G1's incremental calculation of region elapsed time always uses the same age group for prediction

[Stefan Johansson]

On 2019-11-20 13:49, Thomas Schatzl wrote:
> Hi Stefan,
> 
> On 20.11.19 12:24, Stefan Johansson wrote:
>> Hi Thomas,
>>
>> On 2019-11-19 10:25, Thomas Schatzl wrote:
>>> Hi all,
>>>
>>>    can I have reviews for this change that fixes use of the wrong 
>>> predictors when adding a new mutator region to the collection set as 
>>> it is retired?
>>>
>>> G1, through the young remset sampling thread, and the mutator threads 
>>> when they retire, keep track of a prediction for the time it takes to 
>>> evacuate the entire young gen.
>>>
>>> The mutator thread, when it retires a region, updates that value by 
>>> the current prediction of that retired region: that is where the 
>>> error occurs: currently, it only ever uses the prediction for the 
>>> region that has been retired just before GC.
>>> Typically a lot of data in this region survives (e.g. 80%+), so the 
>>> prediction for the overall young gen is heavily inflated.
>>> Which means that during mixed gc, a smaller than necessary amount of 
>>> time is seen as left for evacuating old gen regions, which in turn 
>>> means that G1 typically does more, shorter than expected mixed gcs.
>>>
>>> This wastes some throughput, as typically normal young collections 
>>> can take a much larger eden.
>>>
>>> This change fixes that by the mutator and the young remset sampling 
>>> thread not updating the time it takes to copy the contents of eden 
>>> regions - the predictions for that are fixed anyway after a GC as the 
>>> predictors for how many objects are expected to be live in a region 
>>> are not updated while the mutator is running. Only other components 
>>> of the prediction are.
>>>
>>> The time to copy the eden regions is later, when G1 finalizes the 
>>> prediction for the time the young gen will take, added back.
>>>
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8231579
>>> Webrev:
>>> http://cr.openjdk.java.net/~tschatzl/8231579/webrev/
>> Looks good, just some variable naming that I think should be updated:
>> src/hotspot/share/gc/g1/g1CollectionSet.cpp
>> ---
>>   248   double old_elapsed_time_ms = hr->predicted_non_copy_time_ms();
>>   249   double new_region_elapsed_time_ms = 
>> predict_region_non_copy_time_ms(hr);
>>   250   double non_copy_time_ms_diff = new_region_elapsed_time_ms - 
>> old_elapsed_time_ms;
>>   251   hr->set_predicted_non_copy_time_ms(new_region_elapsed_time_ms);
>>   252   _inc_predicted_non_copy_time_ms_diff += non_copy_time_ms_diff;
>>
>> I think the local variables should change to reflect the new naming, 
>> something like "old_non_copy_time" and "new_non_copy_time".
> 
>   you are right. Fixed in
> 
> http://cr.openjdk.java.net/~tschatzl/8231579/webrev.0_to_1/ (diff)
> http://cr.openjdk.java.net/~tschatzl/8231579/webrev.1/ (full)
Looks good.

Thanks,
Stefan
> 
> Thanks,
>    Thomas