RFR: Changes to simplify MU accounting

[Y. Srinivas Ramakrishna]

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.

It seems like you want to rename `ShenandoahMmuTracker` to `ShenandoahMuTracker` :-)

Changes look good; just some minor comments below.

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

> 504:       msg = (generation == YOUNG) ? "At end of Concurrent Young GC" :
> 505:                                     "At end of Concurrent Bootstrap GC";
> 506:       // We only record GC results if GC was successful

Can this section on MMU tracking be moved into `SCT::service_concurrent_cycle(ShenandoahHeap*, ShenandoahGeneration*, GCCause&, bool bootstrap)` at (just after) line 728 below?

That would be consistent with doing the corresponding old gen work in `SCT::service_cocnurrent_old_cycle()` as you've done below.

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

> 294:   }
> 295: 
> 296:   // With simplified MU implementation, we no longer use _mmu_tracker->average to trigger resizing

Delete, rather than comment out. You can just leave a comment to motivate the deletion if you think that would help reviewers.

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

> 52: class ShenandoahMmuTracker {
> 53: private:
> 54:   // For reporting utilization during most recent GC cycle

May be elaborate a bit (please correct as needed):

// These hold the values of the most recent snapshots of cumulative
// observations, and are used to compute deltas for specific 
// recording/reporting epochs at each subsequent snapshot.

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

> 73:   TruncatedSeq _mmu_average;
> 74: 
> 75:   static double process_time_seconds();

It seems like `process_time_seconds()` is no longer needed either.

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

> 76:   static void fetch_cpu_times(double &gc_time, double &mutator_time);
> 77: 
> 78:   void help_record_concurrent(ShenandoahGeneration* generation, uint gcid, const char* msg);

Do you want to call this `record_concurrent_helper()` or `..._work()` in line with existing helper method convention?

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

> 85:   void initialize();
> 86: 
> 87:   // At completion of each GC cycle (not including interrupted cycles), we invoke one of the following to record the

In the case of GC's that have been canceled, is the work quantum in service of the partial/canceled cycle just dropped on the floor? Should it intentionally be? Should it intentionally not be?

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

> 88:   // GC utilization during this cycle.
> 89:   //
> 90:   // We may redundantly record degen and full, in which case the gcid will repeat.  We log these as FULL.

Won't `gc_id` also repeat for old_marking_increments too?

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

Marked as reviewed by ysr (Committer).

PR Review: https://git.openjdk.org/shenandoah/pull/274#pullrequestreview-1417731871
PR Review Comment: https://git.openjdk.org/shenandoah/pull/274#discussion_r1188041048
PR Review Comment: https://git.openjdk.org/shenandoah/pull/274#discussion_r1190537254
PR Review Comment: https://git.openjdk.org/shenandoah/pull/274#discussion_r1190535612
PR Review Comment: https://git.openjdk.org/shenandoah/pull/274#discussion_r1189236492
PR Review Comment: https://git.openjdk.org/shenandoah/pull/274#discussion_r1190532083
PR Review Comment: https://git.openjdk.org/shenandoah/pull/274#discussion_r1188057149
PR Review Comment: https://git.openjdk.org/shenandoah/pull/274#discussion_r1190532982