RFR: 8369048: GenShen: Defer ShenFreeSet::available() during rebuild

[Y. Srinivas Ramakrishna]

On Thu, 2 Oct 2025 17:58:48 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

> This code introduces a new rebuild-freeset lock for purposes of coordinating the freeset rebuild activities and queries as to memory available for allocation in the mutator partition.
> 
> This addresses a problem that results if available memory is probed while we are rebuilding the freeset.
> 
> Rather than using the existing global heap lock to synchronize these activities, a new more narrowly scoped lock is introduced.  This allows the available memory to be probed even when other activities hold the global heap lock for reasons other than rebuilding the freeset, such as when they are allocating memory.  It is known that the global heap lock is heavily contended for certain workloads, and using this new lock avoids adding to contention for the global heap lock.

Left a few comments. (In particular of what looks like a deadlock possibility in debug builds.)

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.hpp line 240:

> 238:   // Return available_in assuming caller does not hold the heap lock.  In production builds, available is
> 239:   // returned without acquiring the lock.  In debug builds, the global heap lock is acquired in order to
> 240:   // enforce a consistency assert.

Can the comment be simplified to:


// Return bytes `available` in the given `partition`
// while holding the `rebuild_lock`.


Don't say anything about the heap lock in the API comment. Rather, in the part that is `ifdef ASSERT` where you take the heap lock (line ~244), say:

//  Acquire the heap lock to get a consistent
// snapshot to check assert.

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.hpp line 323:

> 321:   ShenandoahRegionPartitions _partitions;
> 322: 
> 323:   // This locks the rebuild process (in combination with the global heap lock)

Explain the role of this & the global heap lock vis-a-vis the rebuild process.

Also may be call it `_rebuild_lock`, rather than just `_lock`.

src/hotspot/share/gc/shenandoah/shenandoahFreeSet.hpp line 426:

> 424: 
> 425: 
> 426:   ShenandoahRebuildLock* lock() {

`rebuild_lock()` instead?

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

> 1156:     size_t young_cset_regions, old_cset_regions, first_old, last_old, num_old;
> 1157:     ShenandoahFreeSet* free_set = heap->free_set();
> 1158:     ShenandoahRebuildLocker rebuild_locker(free_set->lock());

Should you not create a scope around lines 1158 to line 1167, since you don't want to hold the rebuild lock as soon as the rebuild is done (i.e. immediately following `finish_rebuild()`)?

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

PR Review: https://git.openjdk.org/jdk/pull/27612#pullrequestreview-3402148589
PR Review Comment: https://git.openjdk.org/jdk/pull/27612#discussion_r2479874892
PR Review Comment: https://git.openjdk.org/jdk/pull/27612#discussion_r2479853761
PR Review Comment: https://git.openjdk.org/jdk/pull/27612#discussion_r2479854317
PR Review Comment: https://git.openjdk.org/jdk/pull/27612#discussion_r2479861263