RFR: Generation resizing [v4]

[Y. Srinivas Ramakrishna]

On Thu, 8 Dec 2022 21:46:54 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> These changes have the generational mode track the minimum mutator utilization (percentage of process time used by mutators). When it falls below a configuration percentage (GCTimeRatio), a heuristic will transfer memory capacity to whatever generation has been using more CPU time. The assumption here is that by increasing capacity, we will decrease the collection frequency and improve the MMU.
>
> William Kemper has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 17 commits:
> 
>  - Merge branch 'shenandoah-master' into mmu-instrumentation
>  - Remove vestigial lock, do not enroll periodic task while holding threads_lock
>  - Remove unnecessary logging, clean up imports
>  - Merge from shenandoah/master
>  - Document the class responsible for adjusting generation sizes
>  - Revert unnecessary change
>  - Remove unused time between cycle tracking
>  - Remove vestigial mmu tracker instance
>  - Clamp adjustments to min/max when increment is too large
>  - Adjust generation sizes from safepoint
>  - ... and 7 more: https://git.openjdk.org/shenandoah/compare/25469283...50896e31

Sorry again for not getting this review back to you in time.

It looks good overall, but some comments here for you to use to perhaps improve a few things.

Reviewed & approved, modulo the above comments.

src/hotspot/share/gc/shenandoah/mode/shenandoahGenerationalMode.cpp line 39:

> 37:   }
> 38: 
> 39:   SHENANDOAH_ERGO_OVERRIDE_DEFAULT(GCTimeRatio, 70);

Does this translate to a GC overhead of 1/71*100% = 1.4%?

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

> 57:   ShenandoahHeap::heap()->gc_threads_do(&cl);
> 58:   // Include VM thread? Compiler threads? or no - because there
> 59:   // is nothing the collector can do about those threads.

Correct, we should not measure in the control signal that which we do not affect. I'd either delete this comment, or just state smething like:
// We do not include non-GC vm threads, such as compiler threads, etc. in our measurement
// since we are using the tracker only to control (affect) the time spent in GC.

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

> 86:   // This is only called by the control thread.
> 87:   double collector_time_s = gc_thread_time_seconds();
> 88:   double elapsed_gc_time_s = collector_time_s - _initial_collector_time_s;

Since "elapsed" has a different connotation, it would be less confusing for this variable to be called something like `delta_gc_time_s` being the delta between what was previously recorded and what has now been recorded.

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

> 94:   // This is only called by the periodic thread.
> 95:   double process_time_s = process_time_seconds();
> 96:   double elapsed_process_time_s = process_time_s - _initial_process_time_s;

elapsed -> delta

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

> 97:   _initial_process_time_s = process_time_s;
> 98:   double verify_time_s = gc_thread_time_seconds();
> 99:   double verify_elapsed = verify_time_s - _initial_verify_collector_time_s;

elapsed -> delta

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

> 122:   if (old_time_s > young_time_s) {
> 123:     return transfer_capacity(young, old);
> 124:   } else {

In another place I had asked if this method was idempotent. It would be nice if it were.

This is almost idempotent, but not quite.

You can make it idempotent by changing the `else` to `else if (young_time_s > old_time_s)` thus sidestepping the case where the two have just been reset and will be 0 (at least until the next gc).

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

> 144:   size_t regions_to_transfer = MAX2(1UL, size_t(double(available_regions) * _resize_increment));
> 145:   size_t bytes_to_transfer = regions_to_transfer * ShenandoahHeapRegion::region_size_bytes();
> 146:   if (from->generation_mode() == YOUNG) {

I'd consider extracting the work in the `if` and `else` arms into a suitable smaller work method (or two, if one won't suffice for both arms) instead of doing it in line here. It might improve readability and maintainability of the code.

If you tried that and it didn't help, you can ignore this comment. The similarity in shape of the two arms and the "duplication" just seemed to be worth refactoring into a worker method.

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

> 52:   double _initial_collector_time_s;
> 53:   double _initial_process_time_s;
> 54:   double _initial_verify_collector_time_s;

What does this field with `verify` in its name track?

For each of the data fields, I'd suggest adding a short comment; e.g.:


double _initial_collector_time_s;    // tracks cumulative collector threads virtual cpu-time at last recording


etc.

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

PR: https://git.openjdk.org/shenandoah/pull/177