RFR: 8325587: Shenandoah: ShenandoahLock should allow blocking in VM

[Robbin Ehn]

On Mon, 12 Feb 2024 17:40:00 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)

I know to little about Shenandoah to say if blocking is always okay, sorry.
Other than that I don't spot any issues, had some minor suggestion.

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

> 63: }
> 64: 
> 65: void ShenandoahLock::contended_lock_or_block(JavaThread* java_thread) {

As this is a copy you could use ThreadBlockInVM or a NoOp as template parameter to get one version.

src/hotspot/share/gc/shenandoah/shenandoahLock.hpp line 35:

> 33: class ShenandoahLock  {
> 34: private:
> 35:   enum LockState { unlocked = 0, locked = 1 };

Keep ?

src/hotspot/share/gc/shenandoah/shenandoahLock.hpp line 48:

> 46: 
> 47:   void lock(bool allow_block_for_safepoint) {
> 48:     assert(_owner != Thread::current(), "reentrant locking attempt, would deadlock");

We are trying to use Atomic::load on all volatile reads (i.e. those that can be written concurrently).
As volatile semantic is on the way out.

src/hotspot/share/gc/shenandoah/shenandoahLock.hpp line 51:

> 49: 
> 50:     // Try to lock fast, or dive into contended lock handling.
> 51:     if (Atomic::cmpxchg(&_state, 0, 1) != 0) {

If we keep enum, this line reads:
`if (Atomic::cmpxchg(&_state, unlocked, locked) != unlocked) {`

Which I find more appealing.

src/hotspot/share/gc/shenandoah/shenandoahLock.hpp line 57:

> 55:     assert(_state == 1, "must be locked");
> 56:     assert(_owner == nullptr, "must not be owned");
> 57:     DEBUG_ONLY(_owner = Thread::current();)

We are trying to use Atomic::store on all volatile writes (i.e. those that can be read concurrently).
As volatile semantic is on the way out.

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

PR Review: https://git.openjdk.org/jdk/pull/17813#pullrequestreview-1881035131
PR Review Comment: https://git.openjdk.org/jdk/pull/17813#discussion_r1489939902
PR Review Comment: https://git.openjdk.org/jdk/pull/17813#discussion_r1489940587
PR Review Comment: https://git.openjdk.org/jdk/pull/17813#discussion_r1489944834
PR Review Comment: https://git.openjdk.org/jdk/pull/17813#discussion_r1489939951
PR Review Comment: https://git.openjdk.org/jdk/pull/17813#discussion_r1489940074