RFR: Changes to simplify MU accounting

[William Kemper]

On Mon, 8 May 2023 23:52:30 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

> This represents a small part of the expand-old-on-demand branch.  By itself, this may result in some performance regression because we no longer use MU (mutator utilization) reports to guide generation sizing decisions.  On the other hand, the guidance provided by MU metrics does not seem to be a very accurate predictor of ideal generation sizes.

Changes requested by wkemper (Committer).

src/hotspot/share/gc/shenandoah/shenandoahControlThread.cpp line 511:

> 509:         if (heap->collection_set()->has_old_regions()) {
> 510:           bool mixed_is_done = (heap->old_heuristics()->unprocessed_old_collection_candidates() == 0);
> 511:           mmu_tracker->record_mixed(the_generation, GCId::current(), mixed_is_done);

Could use `ShenandoahControlThread::get_gc_id` or just access `_gc_id` directly. `GCId::current` has thread local access overhead.

src/hotspot/share/gc/shenandoah/shenandoahMmuTracker.cpp line 96:

> 94: }
> 95: 
> 96: void ShenandoahMmuTracker::help_record_concurrent(ShenandoahGeneration* generation, uint gcid, const char *msg) {

`help_record_concurrent` is a confusing name because this function is also used to record degenerated and full gc cycles. Maybe just `update_utilization`?

src/hotspot/share/gc/shenandoah/shenandoahMmuTracker.cpp line 106:

> 104:     _most_recent_timestamp = current;
> 105:   } else {
> 106:     double gc_cycle_duration = current - _most_recent_timestamp;

Is this cycle duration? or time between cycles?

src/hotspot/share/gc/shenandoah/shenandoahMmuTracker.cpp line 138:

> 136:                                                         bool has_old_candidates) {
> 137:   // No special processing for old marking
> 138:   double duration = os::elapsedTime() - _most_recent_timestamp;

We only seem to use `duration` here, none of the following lines produce any used values?

src/hotspot/share/gc/shenandoah/shenandoahMmuTracker.cpp line 158:

> 156:                                               uint gcid, bool is_old_bootstrap, bool is_mixed_done) {
> 157:   if ((gcid == _most_recent_gcid) && _most_recent_is_full) {
> 158:     // Do nothing.  This is a redundant recording for the full gc that just completed.

We have a redundant call here because some degenerated cycles become full cycles? Could we arrange for this to only be called once from each code path?

src/hotspot/share/gc/shenandoah/shenandoahMmuTracker.cpp line 192:

> 190:   double mu = mutator_delta / (_active_processors * time_delta);
> 191:   double gcu = gc_delta / (_active_processors * time_delta);
> 192:   log_info(gc)("Periodic Sample: Average GCU = %.3f%%, Average MU = %.3f%%", gcu * 100, mu * 100);

These aren't really averages any more right? Just the utilization over the last period?

src/hotspot/share/gc/shenandoah/shenandoahMmuTracker.hpp line 70:

> 68: 
> 69:   bool _most_recent_is_full;
> 70:   bool _doing_mixed_evacuations;

This is written, but never read. Is this used somewhere on a different branch as part of the bigger picture?

-------------

PR Review: https://git.openjdk.org/shenandoah/pull/274#pullrequestreview-1423472751
PR Review Comment: https://git.openjdk.org/shenandoah/pull/274#discussion_r1191723666
PR Review Comment: https://git.openjdk.org/shenandoah/pull/274#discussion_r1191729620
PR Review Comment: https://git.openjdk.org/shenandoah/pull/274#discussion_r1191726260
PR Review Comment: https://git.openjdk.org/shenandoah/pull/274#discussion_r1191739206
PR Review Comment: https://git.openjdk.org/shenandoah/pull/274#discussion_r1191740981
PR Review Comment: https://git.openjdk.org/shenandoah/pull/274#discussion_r1191741664
PR Review Comment: https://git.openjdk.org/shenandoah/pull/274#discussion_r1191735012