RFR: 8377048: Shenandoah: shenandoahLock related improvments [v6]

[Xiaolong Peng]

On Wed, 4 Feb 2026 07:40:36 GMT, Xiaolong Peng <xpeng at openjdk.org> wrote:

>> 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?
>
> I didn't run test for it yet, but for non-debug build, the capacity/used/available are not frequently read, the overhead should have very minor impact(it may still compete with thread doing rebuild). 
> In debug build, because of the asserts need to read the accountings much more often, it should have a small performance impact.
> 
> The change is really about correctness, these filed may be be updated with rebuild lock being hold, but can be read w/o acquiring the rebuild lock, so a thread reading them w/o the rebuild lock may see stale value.
> 
> As a part of the change of JDK-8373371, I am updating how the heap usage accountings are updated:
> 1. All changes to heap usage accountings must be done with holding heap lock and heap usage accounting lock(rebuild lock will be renamed to usage accounting lock),
> 2. All reads must be done under heap usage accounting lock, no need to hold heap lock. 
> 3. Computation of total/young/old accountings will be done in the read path under heap usage accounting lock.
> 
> Here are the examples of young_used(), old_used(), global_used() with deferred re-computation:
> 
>   // Return bytes used by young
>   inline size_t young_used() {
>     ShenandoahHeapUsageAccountingLocker locker(usage_accounting_lock());
>     return _partitions.used_by(ShenandoahFreeSetPartitionId::Mutator) +
>       _partitions.used_by(ShenandoahFreeSetPartitionId::Collector);
>   }
> 
>   // Return bytes used by old
>   inline size_t old_used() {
>     ShenandoahHeapUsageAccountingLocker locker(usage_accounting_lock());
>     return _partitions.used_by(ShenandoahFreeSetPartitionId::OldCollector);
>   }
> 
>   // Return bytes used by global
>   inline size_t global_used() {
>     ShenandoahHeapUsageAccountingLocker locker(usage_accounting_lock());
>     return young_used() + old_used();
>   }
> 
> 
> ShenandoahHeapUsageAccountingLock is a reentrant lock, global_used acquires the lock first, so it will have stable snapshot of young_used and old_used when it calculate the global_used. 
> 
> Reentrance of the ShenandoahHeapUsageAccountingLock is very cheap since the current thread already holds it, it just checks the owner of the lock and increase the counter.
> 
> I have pushed this change to gitfarm pipeline, I'm not really expecting any performance regression form this, I'll check the result tomorrow.

I have reverted this change, I'll include it in the following PR deferring accounting re-computation.

>> 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
>
> Yes, it is related to initialization, and only happens in the  "pre-initialized state", I'll add some comments for this.

I have updated the comments and added #ifdef ASSERT to enclose the asserts 

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

PR Review Comment: https://git.openjdk.org/jdk/pull/29543#discussion_r2848719927
PR Review Comment: https://git.openjdk.org/jdk/pull/29543#discussion_r2848722200