RFR (M): 8231579: G1's incremental calculation of region elapsed time always uses the same age group for prediction
stefan.johansson at oracle.com
Wed Nov 20 11:24:27 UTC 2019
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.
Looks good, just some variable naming that I think should be updated:
248 double old_elapsed_time_ms = hr->predicted_non_copy_time_ms();
249 double new_region_elapsed_time_ms =
250 double non_copy_time_ms_diff = 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".
> 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
More information about the hotspot-gc-dev