RFR: 8325587: Shenandoah: ShenandoahLock should allow blocking in VM [v2]

[Roman Kennke]

On Thu, 15 Feb 2024 12:02:29 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> `ShenandoahLock` is a spinlock that is supposed to guard heap state. That lock is normally only lightly contended, as threads normally only allocate large TLABs/GCLABs. So we just summarily delegated to `Thread::{SpinAcquire,SpinRelease}`, which spins a bit, and then starts going to sleep/yield dance on contention.
>> 
>> This does not work well when there are lots of threads near the OOM conditions. Then, most of these threads would fail to allocate the TLAB, go for out-of-TLAB alloc, start lock acquisition, and spend _a lot_ of time trying to acquire the lock. The handshake/safepoint would think those threads are running, even when they are actually yielding or sleeping. As seen in bug report, a handshake operation over many such threads could then take hundreds of seconds.
>> 
>> The solution is to notify VM that we are blocking before going for `sleep` or `yield`. This is similar to what other VM code does near such yields. This involves state transitions, so it is only cheap to do near the actual blocking. Protecting the whole lock with VM transition would be very slow.
>> 
>> I also de-uglified bits of adjacent code.
>> 
>> Additional testing:
>>  - [x] Original Extremem reproducer does not have outliers anymore
>>  - [x] Linux x86_64 server fastdebug, `hotspot_gc_shenandoah`
>>  - [x] Linux x86_64 server fastdebug, `all` passes with `-XX:+UseShenandoahGC` (some old failures, but quite a few "timeouts" also disappear)
>
> Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Review comments

Looks good. I have one question (and leave it up to you to make the change).

Thanks,
Roman

src/hotspot/share/gc/shenandoah/shenandoahLock.cpp line 39:

> 37: class ShenandoahNoBlockOp : public StackObj {
> 38: public:
> 39:   ShenandoahNoBlockOp(JavaThread* java_thread) {

Should this be inline? Or would compiler be clever enough to not emit anything there?
Edit: or doesn't it matter, because we are going to yield anyway?

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

Marked as reviewed by rkennke (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17813#pullrequestreview-1892683211
PR Review Comment: https://git.openjdk.org/jdk/pull/17813#discussion_r1497259792