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

[Thomas Schatzl]

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)

Thanks,
   Thomas