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

[Stefan Johansson]

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

Thanks,
Stefan

> Testing:
> hs-tier1-5; local tests showed significant decreases in time spent in 
> mixed gcs; dev-submit testing: no significant score changes either with 
> this change in particular or compared to all recent changes
> 
> Thanks,
>    Thomas