RFR: 8377048: Shenandoah: shenandoahLock related improvments

[Kelvin Nilsen]

On Tue, 3 Feb 2026 09:54:44 GMT, Xiaolong Peng <xpeng at openjdk.org> wrote:

> While I was working on https://bugs.openjdk.org/browse/JDK-8373371, I made these changes to the shenandoahLock, but I feel it make sense peel off these changes into a separate PR as they are not really related to JDK-8373371. 
> 
> Major thing in this PR is to use template for ShenandoahReentrantLock and ShenandoahLocker so we could get rid of duplicate code, along with some other changes where the locks are used e.g. use type alias for ShenandoahNMethodLock and ShenandoahNMethodLocker, use ShenandoahReentrantLock for ShenandoahRebuildLock.
> 
> Another improvement is to strengthen assert in SheandnoahHeapLocker to avoid potential dead lock(this was also discussed in previous PR [here](https://github.com/openjdk/jdk/pull/27612#discussion_r2479898169) and our internal channel
> 
> ### Test
> - [x] MacOS aach64, server fastdebug: hotspot_gc_shenandoah
> - [x] GHA

Thanks for submitting this PR.  Please help motivate the changes in the intro to this PR.

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

> 910: void ShenandoahRegionPartitions::assert_bounds() {
> 911:   shenandoah_assert_heaplocked();
> 912:   ShenandoahRebuildLocker locker(_free_set->rebuild_lock());

assert_bounds() often happens when we are not rebuilding the free set.  Not clear why you need to add this lock here.

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

> 645:   ShenandoahFreeSet(ShenandoahHeap* heap, size_t max_regions);
> 646: 
> 647:     ShenandoahRebuildLock* rebuild_lock() {

unintentional spaces?

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

> 788:   // inconsistent data: available may not equal capacity - used because the intermediate states of any "atomic"
> 789:   // locked action can be seen by these unlocked functions.
> 790:   inline size_t capacity_holding_lock() const {

It looks like you're adding overhead to these calls here.  Previously, we were able to ask for capacity() and used() and available() without needing to pay the cost of acquiring a lock.  Now, we always have to acquire the lock().  Have you measured impact?

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

> 2844: }
> 2845: 
> 2846: ShenandoahHeapLocker::ShenandoahHeapLocker(ShenandoahHeapLock* lock, bool allow_block_for_safepoint) : _lock(lock) {

Can we have a comment to explain when lock will equal nullptr?

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

> 2846: ShenandoahHeapLocker::ShenandoahHeapLocker(ShenandoahHeapLock* lock, bool allow_block_for_safepoint) : _lock(lock) {
> 2847:   ShenandoahFreeSet* free_set = ShenandoahHeap::heap()->free_set();
> 2848:   assert(free_set == nullptr || !free_set->rebuild_lock()->owned_by_self(), "Dead lock, can't acquire heap lock while holding free-set rebuild lock");

Can we add a comment to explain when free_set will equal nullptr?  Does this correspond to "pre-initialized state" or something else?

Would probably be more clear that we don't need to consult free_set unless asserts are enabled, so enclose this code in #ifdef ASSERT

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 121:

> 119: 
> 120: typedef ShenandoahLock                       ShenandoahHeapLock;
> 121: // ShenandoahHeapLocker checks potential deadlock and asserts

Years from now, the reader of this comment will not "appreciate" the context in which it was written.  The comment only makes sense in the context of this "diff".

Would be better to say: ShenandoahHeapLocker implements a lock to assure mutually exclusive access to the global heap data structures.  Asserts in the implementation detect potential deadlock usage with regards the rebuild lock that is present in ShenandoahFreeSet.  Whenever both locks are acquired, this lock should be acquired before the ShenandoahFreeSet rebuild lock.

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

Changes requested by kdnilsen (Committer).

PR Review: https://git.openjdk.org/jdk/pull/29543#pullrequestreview-3746585690
PR Review Comment: https://git.openjdk.org/jdk/pull/29543#discussion_r2760187909
PR Review Comment: https://git.openjdk.org/jdk/pull/29543#discussion_r2760194774
PR Review Comment: https://git.openjdk.org/jdk/pull/29543#discussion_r2760206836
PR Review Comment: https://git.openjdk.org/jdk/pull/29543#discussion_r2760231347
PR Review Comment: https://git.openjdk.org/jdk/pull/29543#discussion_r2760229796
PR Review Comment: https://git.openjdk.org/jdk/pull/29543#discussion_r2760247618