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

[William Kemper]

On Tue, 30 Sep 2025 21:55:46 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 five additional commits since the last revision:
> 
>  - small refactoring
>  - OO refinements to used_regions_size()
>  - fix broken assert from previous commit
>  - OO refinements to free_unaffiliated_regions()
>  - OO refinements to max_capacity()

Mostly nits and questions.

src/hotspot/share/gc/shared/gc_globals.hpp line 472:

> 470:   product(size_t, SoftMaxHeapSize, 0, MANAGEABLE,                           \
> 471:           "Soft limit for maximum heap size (in bytes)")                    \
> 472:           constraint(SoftMaxHeapSizeConstraintFunc,AfterMemoryInit)         \

Changing a common file might raise eyebrows. Why do we need this?

src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp line 1153:

> 1151: void ShenandoahConcurrentGC::op_final_update_refs() {
> 1152:   ShenandoahHeap* const heap = ShenandoahHeap::heap();
> 1153:   bool is_generational = heap->mode()->is_generational();

Are the changes here necessary? This is a big PR, it would be helpful to minimize unnecessary changes.

src/hotspot/share/gc/shenandoah/shenandoahFullGC.cpp line 1145:

> 1143:   _preserved_marks->reclaim();
> 1144: 
> 1145:   // We defer generation resizing actions until after cset regions have been recycled.  We do this even following an

Stale comment?

src/hotspot/share/gc/shenandoah/shenandoahGenerationSizer.hpp line 1:

> 1: /*

Is there any point in keeping this file now? Not sure what this class is doing now.

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 745:

> 743: //  2. Region's free is less than minimum TLAB size and is retired
> 744: //  3. The unused portion of memory in the last region of a humongous object
> 745: void ShenandoahHeap::increase_used(const ShenandoahAllocRequest& req) {

I don't see the point of this method now. Can we delete it?

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 2719:

> 2717:   assert(used() <= ShenandoahHeap::heap()->max_capacity(), "sanity");
> 2718:   assert(committed() <= ShenandoahHeap::heap()->max_capacity(), "sanity");
> 2719:   assert(max_capacity() <= ShenandoahHeap::heap()->max_capacity(), "sanity");

This last one is just comparing the results of the same method call. Are we worried about races here?

src/hotspot/share/gc/shenandoah/shenandoahMemoryPool.cpp line 65:

> 63:   // to make sense under the race. See JDK-8207200.
> 64:   committed = MAX2(used, committed);
> 65:   assert(used <= committed, "used: %zu, committed: %zu", used, committed);

Shouldn't it always be the case that `initial <= used <= committed <= max`?

src/hotspot/share/gc/shenandoah/shenandoahSimpleBitMap.cpp line 42:

> 40: }
> 41: 
> 42: size_t ShenandoahSimpleBitMap::count_leading_ones(index_type start_idx) const {

Did we figure out why we want to rename this type? It adds a lot of noise to this PR and I'd hate to do something like this because of a C++ technicality.

src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp line 498:

> 496: 
> 497: class ShenandoahVerifyHeapRegionClosure : public ShenandoahHeapRegionClosure {
> 498:   private:

Formatting the access modifiers like this looks idiosyncratic:

% grep -ri -P '^  private:' src/hotspot | wc -l                  
115
% grep -ri -P '^private:' src/hotspot | wc -l  
1444

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

Changes requested by wkemper (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/26867#pullrequestreview-3296845640
PR Review Comment: https://git.openjdk.org/jdk/pull/26867#discussion_r2400422999
PR Review Comment: https://git.openjdk.org/jdk/pull/26867#discussion_r2400380773
PR Review Comment: https://git.openjdk.org/jdk/pull/26867#discussion_r2400382789
PR Review Comment: https://git.openjdk.org/jdk/pull/26867#discussion_r2400397766
PR Review Comment: https://git.openjdk.org/jdk/pull/26867#discussion_r2400401051
PR Review Comment: https://git.openjdk.org/jdk/pull/26867#discussion_r2400404948
PR Review Comment: https://git.openjdk.org/jdk/pull/26867#discussion_r2400410359
PR Review Comment: https://git.openjdk.org/jdk/pull/26867#discussion_r2400411950
PR Review Comment: https://git.openjdk.org/jdk/pull/26867#discussion_r2400418474