RFR: 8345423: Shenandoah: Parallelize concurrent cleanup [v8]

[Y. Srinivas Ramakrishna]

On Tue, 10 Dec 2024 23:22:02 GMT, Xiaolong Peng <xpeng at openjdk.org> wrote:

>>> There could be race condition that other caller immoderately set the flag, hence the assert may fail, 
>> 
>> Which other caller? We are asserting here in the constructor of the SHR object. Is this object visible to anyone other than the constructing thread at this point? I am not sure I understand the reason for a race here.
>> 
>> It's possible I am missing something in the lifecycle of the SHR object here.
>> 
>> If so, it would be good to add a brief comment on why this needs to occur here despite the constructor for the `_recycling` flag which should have been called by this point, so it should already be unset by now.
>> 
>>> notice similar race condition in test, that is why the double check for is_trash() was added.
>> 
>> Yes, I understood that race, which is between multiple threads potentially racing to recycle a trashed region, and resolves such a race in favor of the thread that manages to CAS  true into `_recycling` with interlocking checks for its `trash`ness.
>
>> Which other caller? 
> 
> Sorry for the confusion, not the caller of this specific method.
> 
> I meant to say the the mutator thread, mutator may call try_recycle_trashed here https://github.com/openjdk/jdk/blob/master/src/hotspot/share/gc/shenandoah/shenandoahFreeSet.cpp#L1005 under heap-lock, we have removed the heap-lock from GC concurrent cleanup, therefore it becomes race condition between mutator and GC threads.

OK, that's what I meant. I didn't think the region would be visible to any other thread in the time that the constructor is being executed (which, my guess was, would be when the ShenandoahHeap is first initialized -- before any mutators exist that can access the heap), but I may be missing something here in the lifecycle of a region. Thanks for pointing out the possibility of a race (but that makes me wonder about other things that may go wrong if there were such a race.) I'll think more about this later.

In any case, what I was pointing out (based on my mental model) was not a correctness issue, so I'll go away and try and understand the race you mention.

No change is needed here. It's good as is, and my review approval stands. Thanks Xiaolong!

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22538#discussion_r1880574386