RFR: 8361099: Shenandoah: Improve heap lock contention by using CAS for memory allocation [v20]

[Xiaolong Peng]

On Tue, 6 Jan 2026 01:28:42 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

>> Xiaolong Peng has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 265 commits:
>> 
>>  - Merge branch 'openjdk:master' into cas-alloc-1
>>  - Fix build error after merging from tip
>>  - Merge branch 'master' into cas-alloc-1
>>  - Merge branch 'master' into cas-alloc-1
>>  - Some comments updates as suggested in PR review
>>  - Fix build failure after merge
>>  - Expend promoted from ShenandoahOldCollectorAllocator
>>  - Merge branch 'master' into cas-alloc-1
>>  - Address PR comments
>>  - Merge branch 'openjdk:master' into cas-alloc-1
>>  - ... and 255 more: https://git.openjdk.org/jdk/compare/de81d389...cf13b7b5
>
> src/hotspot/share/gc/shenandoah/shenandoahAllocator.cpp line 376:
> 
>> 374: }
>> 375: 
>> 376: THREAD_LOCAL uint ShenandoahMutatorAllocator::_alloc_start_index = UINT_MAX;
> 
> I raised questions about this in a previous review.  Have I overlooked your response?  What is the tradeoff between declaring this THREAD_LOCAL vs. creating a new field in ShenandoahThreadLocal?  I believe we need to use fields of ShenandoahThreadLocal so that we do not incur an overhead on all threads when JVM is not configured for Shenandoah GC.

I don't have concern to move it to ShenandoahThreadLocalData, the benefit of using THREAD_LOCAL is just better cohesive code because  _alloc_start_index is defined in the same namespace where it is used. Performance wise, I don't think there is much benefits.

I do see ZGC also use THREAD_LOCAL directly, I guess the overhead on all threads is not a huge concern. 
But given Shenandoah has ShenandoahThreadLocalData to manage all the thread locals, it make sense to not use THREAD_LOCAL directly, I'll update the PR to move _alloc_start_index to ShenandoahThreadLocalData.

> src/hotspot/share/gc/shenandoah/shenandoahAllocator.cpp line 436:
> 
>> 434:   // Make sure the old generation has room for either evacuations or promotions before trying to allocate.
>> 435:   auto old_gen = ShenandoahHeap::heap()->old_generation();
>> 436:   if (req.is_old() && !old_gen->can_allocate(req)) {
> 
> This test for req.is_old() appears to be unnecessary.  The verify(req) assert above requires that req.is_old().
> 
> Perhaps the verify() method is too abstract.  Add a comment there that says: "Confirm that req.is_old()"

Thanks for pointing out this, it is not necessary, I have removed it.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26171#discussion_r2677369816
PR Review Comment: https://git.openjdk.org/jdk/pull/26171#discussion_r2677373200