RFR: 8244505: G1 pause time ratio calculation does not consider Remark/Cleanup

[Thomas Schatzl]

Hi Ivan,

On 15.09.20 15:57, Ivan Walulya wrote:
> The computation for long and short term pause time ratios does not include Remark/Cleanup pause times. Isolate updates
> to _prev_collection_pause_end_ms from update_recent_gc_times(), then have all gc pauses (including Remark/Cleanup)
> recorded by update_recent_gc_times(). Patch also includes trivial clean ups to g1Policy.* files.

Initial comments:

  - please remove the trivial cleanups from this change and do them in a 
separate "trivial cleanups" change either before or after: they really 
detract from the fix for the actual problem as they are too many, and 
they, while they are located in the g1Policy file, are completely 
unrelated to this patch.

This includes at least the hunks in
  - G1Policy::revise_young_list_target_length_if_necessary (why is 
110/100 better than 1100/1000?)
  - G1Policy::record_concurrent_mark_init_end
  - G1Policy::calc_min_old_cset_length
  - the first hunk in G1Policy::record_collection_pause_end

None of these locations have anything to do with actual pause time ratio 
calculation. Not that we do not do bundle cleanups elsewhere, but the 
cleanup to actual related change ratio is quite high in this change.

  - I would also almost prefer if the pre-existing bug in 
G1Analytics::compute_pause_time_ratios() were also moved out of this 
change to highlight it. It does not seem to be connected to that pause 
time calculation did not consider Remark/Cleanup pause before. (Not 
completely sure why this has not been noticed in the recent young gen 
sizing changes).

  - Please rename G1Policy::update_pause_time_stats() to something more 
related to gc time ratio, i.e. update_actual_gc_time_ratio() or so to 
make it a bit more specific. (This is probably not the perfect name for it).

Will keep looking.

> PR: https://git.openjdk.java.net/jdk/pull/183
> 

Thanks,
   Thomas