RFR: 8328626: GenShen: Combine old generation surplus/deficit fields into a single balance field [v2]

[Y. Srinivas Ramakrishna]

On Fri, 22 Mar 2024 17:44:02 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> This is a follow up to https://github.com/openjdk/shenandoah/pull/406.
>
> William Kemper has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Fix zero build
>  - Fix comments

A few suggestions that might lead to further simplifications, it would seem. Let me know if that is not the case for some reason.

The rest of the changes look good to me, so I am approving the changes, with the suggested changes as something to look into.

If those suggested changes should work out, I'll be happy to do a rereview.

Thanks for the cleanup & refactoring of the code; it feels more compact, and easier to understand and to maintain.

src/hotspot/share/gc/shenandoah/shenandoahGenerationalHeap.cpp line 136:

> 134: 
> 135:   if (old_region_balance < 0) {
> 136:     const auto old_region_deficit = checked_cast<size_t>(-old_region_balance);

Like I remarked elsewhere, the code in the if > 0 and if < 0 branches (or if you will, the if and else branches) are largely isomorphic, and can probably be folded together by calling a generic `transfer_from_old(ssize_t num_regions)` on ShenandoahGenerationSizer. The latter new method would be a folding together of the old methods `transfer_to_young()` and `transfer_to_old()`, and would return the `TransferResult` that this method `balance_generations` returns to its caller.

src/hotspot/share/gc/shenandoah/shenandoahGenerationalHeap.cpp line 242:

> 240:     // curtailed if the budget is restricted.
> 241:     old_region_deficit = MIN2(old_region_deficit, max_old_region_xfer);
> 242:     old_generation()->set_region_balance(0 - checked_cast<ssize_t>(old_region_deficit));

The code in the if and the else branches is highly isomorphic. I have the feeling you may be able to fold them together, but I am not sure how clean it will be. Also if you start with `ssize_t` for some of the operands that feed into the surplus calculation (or balance if you will, but we can call it surplus wit the understanding that a negative surplus is a deficit), then you may be able to avoid some of the later casts and the signing of unsigned quantities, including the `(cast)x`, `-(cast)x` or `0-(cast)x`constructs above.

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

Marked as reviewed by ysr (Committer).

PR Review: https://git.openjdk.org/shenandoah/pull/410#pullrequestreview-1956280101
PR Review Comment: https://git.openjdk.org/shenandoah/pull/410#discussion_r1536542143
PR Review Comment: https://git.openjdk.org/shenandoah/pull/410#discussion_r1536540579