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

[Xiaolong Peng]

On Wed, 7 Jan 2026 19:37:13 GMT, Xiaolong Peng <xpeng at openjdk.org> wrote:

>> Would prefer not to use the atomic_allocate code here.  If you want to reuse code, maybe you can refactor allocate_in with an <ATOMIC> template argument.
>> 
>> I notice that this PR makes lots of ShenandoahHeapRegion variables volatile: _age, _youth, _top, _tlab_allocs, _gclab_allocs, _plab_allocs.  That will cause less efficient code to be generated whenever we are accessing this data from "behind" the GC heap lock, which should be rare, I guess.
>> 
>> I raised some concerns/issues about the race that happens when we move a region from the directly-allocatable set into the global-heap-lock-protected ShenandoahFreeSet partition.
>> 
>> Here's the scenario that I'm concerned about:
>> 
>> 1. A mutator obtains pointer to directly allocatable region R
>> 2. A second mutator performs a refresh, moving region R out of directly allocatable set (for whatever reason)
>> 3. Region R is now eligible to satisfy allocations from behind the global heap lock
>> 4. Some third mutator thread acquires the heap lock and fetches top for region $
>> 5. The first mutator performs its allocation within the same region R, not recognizing a CAS conflict
>> 6. This third mutator allocates from region R at top, without using CAS.  So both mutators think they own the same object
>> 
>> I think this is not a problem if the "only" reason at step 2 above that we move region R out of directly allocatable set is because R is ready to be retired.  In that case, there will be no subsequent heap-locked allocations in regions R.  However, I anticipate the day in not-too-distant future when we will want to refresh regions even when they are not ready to be retired.  Specifically, as we move rebuild-freeset out of safepoints, we will want to refresh regions before we acquire heap-lock to do rebuild, with the goal of making sure there is sufficient directly allocatable memory available that no mutator will be stalled because it needs to allocate during the time that the heap remains locked for the rebuild operation.
>> 
>> So I suppose that if we always use atomic_allocate() even for allocations that happen while holding the heap lock, we won't have this problem.  If we decide to keep this architecture, there should be comments explaining why we are doing it this way.  (I am not real happy that we have to "pay the cost" of CAS in addition to paying the cost of global heap lock, but I think these allocations should be very rare.  It seems this would only come up if, for example, a mutator wanted to allocate ...
>
> I will update the PR and not use atomic version here, and also another place in refresh_alloc_regions. 
> 
> Having volatile_top and nonvolatile_top seems necessary, it will make the code more complicated w/o much performance benefits, with CAS allocator, most of alloc request will be handled by the atomic code path, in only few 
> cases we need non-atomic allocation:
> * After reserving alloc regions from free set before storing to alloc region, it performs obj allocation if the alloc request has not been satisfied yet. 
> * After trying atomic allocation, refresh alloc regions fails, it will try to find a region in free set with enough space for the allocation request.
> 
> Yes, all the _age, _youth, _top, _tlab_allocs, _gclab_allocs, _plab_allocs are volatile now, out of these fields, I believe I can maybe remove volatile for _age and _youth(?), but the update of the rest must be atomic because mutators will increase the values in the CAS allocation code path w/o heap lock.

I have updated the method `atomic_allocate_in` with a template parameter ATOMIC, now only when allocating from shared alloc regions the ATOMIC parameter is true to use atomic operations.

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

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