RFR: Generation resizing [v4]

[William Kemper]

On Fri, 9 Dec 2022 00:59:24 GMT, Y. Srinivas Ramakrishna <ysr at openjdk.org> wrote:

>> 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
>
> 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).

Good point, I'll make that change.

> 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.

Yes, this method is a bit long. The similarity breaks a bit because it needs to enforce the min or max constraint depending on the direction of the transfer. I'll split it up in a subsequent PR.

> 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.

It's vestigial - I was using it originally to "verify" the result of the per-generation based MMU. I'll rename it.

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

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