RFR: 8365880: Shenandoah: Unify memory usage accounting in ShenandoahFreeSet [v3]

[William Kemper]

On Tue, 9 Sep 2025 22:07:11 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

>> This PR eliminates redundant bookkeeping that had been carried out by both ShenandoahGeneration and ShenandoahFreeSet.  In the new code, we keep a single tally of relevant information within ShenandoahFreeSet.
>> Queries serviced by ShenandoahGeneration are now delegated to ShenandoahFreeSet.
>> 
>> This change eliminates rare and troublesome assertion failures that were often raised when the ShenandoahFreeSet tallies did not match the ShenandoahGeneration tallies.  These assertion failures resulted because the two sets of books are updated at different times, using different synchronization mechanisms.
>> 
>> The other benefit of this change is that we have less synchronization overhead because we only have to maintain a single set of books.
>
> Kelvin Nilsen has updated the pull request incrementally with one additional commit since the last revision:
> 
>   another tweak to make GHA gtest happy

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp line 55:

> 53: class ShenandoahLeftRightIterator {
> 54: private:
> 55:   index_type _idx;

Changing this to a different type introduces a lot of noise in the review. It was compiling before this change, can we figure out if it is really necessary to change this type?

src/hotspot/share/gc/shenandoah/shenandoahGeneration.hpp line 134:

> 132:   virtual size_t used_regions_size() const;
> 133:   virtual size_t free_unaffiliated_regions() const;
> 134:   size_t used() const override {

This is already virtual (since it is marked as `override`) so it makes sense to put these different behaviors in the different generation subclasses.

src/hotspot/share/gc/shenandoah/shenandoahGeneration.hpp line 154:

> 152:   size_t available() const override;
> 153:   size_t available_with_reserve() const;
> 154:   size_t used_including_humongous_waste() const {

Can we get rid of this method? Just have callers call `used` now?

src/hotspot/share/gc/shenandoah/shenandoahGenerationalEvacuationTask.cpp line 253:

> 251:     if (available_in_region < plab_min_size_in_bytes) {
> 252:       // The available memory in young had been retired.  Retire it in old also.
> 253:       region_to_be_used_in_old += available_in_region;

This looks like a dead store.

src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp line 104:

> 102: 
> 103:   // Use this only for unit testing.  Do not use for production.
> 104:   inline void set_capacity(size_t bytes) {

We should be able to inline this code in `test_shenandoahOldHeuristic.cpp` in the constructor of the test fixture that calls this method.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26867#discussion_r2344980222
PR Review Comment: https://git.openjdk.org/jdk/pull/26867#discussion_r2344937348
PR Review Comment: https://git.openjdk.org/jdk/pull/26867#discussion_r2344940289
PR Review Comment: https://git.openjdk.org/jdk/pull/26867#discussion_r2345035769
PR Review Comment: https://git.openjdk.org/jdk/pull/26867#discussion_r2345072351