RFR: 8291714: Implement a Multi-Reader Single-Writer mutex for Hotspot [v4]

[Kim Barrett]

On Thu, 25 Aug 2022 13:06:01 GMT, Johan Sjölén <duke at openjdk.org> wrote:

>> May I please have a review for this PR which implements a `MRWMutex` class for Hotspot?
>> 
>> This PR does 3 things:
>> 
>> * Adds a port of ZGC's MRSW mutex (see [0]) to Hotspot
>> * Adds some new utilities for writing multi-threaded tests.
>> * Adds some tests for MRSW Mutex using these new utilities
>> 
>> The ticket has some comments which might be worth checking out: https://bugs.openjdk.org/browse/JDK-8291714
>> 
>> [0] Original source code here: https://github.com/openjdk/zgc/blob/zgc_generational/src/hotspot/share/gc/z/zJNICritical.cpp
>
> Johan Sjölén has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains six commits:
> 
>  - Fix tests for updated threadHelper
>  - Remove outdated threadHelper
>  - Update documentation
>  - Fix outdated headers and remove dead code
>  - Review comments
>  - Implement MRSWMutex

Changes requested by kbarrett (Reviewer).

src/hotspot/share/utilities/readWriteLock.cpp line 32:

> 30: #include "utilities/readWriteLock.hpp"
> 31: 
> 32: void ReadWriteLock::read_lock(Thread *current) {

Consider asserting `current` really is the current thread, both here and in `write_lock`.

src/hotspot/share/utilities/readWriteLock.cpp line 32:

> 30: #include "utilities/readWriteLock.hpp"
> 31: 
> 32: void ReadWriteLock::read_lock(Thread *current) {

The more common (though far from universal) style in HotSpot code cuddles the `*` punctuator with the type rather than the variable, i.e. `Thread* current`.

src/hotspot/share/utilities/readWriteLock.cpp line 38:

> 36:       // Wait until unlocked by writer
> 37:       auto await = [&]() {
> 38:         Locker locker(&_mon);

In Mutex lock/unlock/wait/&etc there are various validity checks on the state of the current thread.  This is going directly to a PlatformMonitor, so not doing any of those checks even though they seem like they are relevant for safe usage checking.  This also doesn't support the InFlightMutexRelease stuff - not sure whether that matters here or not.

src/hotspot/share/utilities/readWriteLock.cpp line 42:

> 40:           _mon.wait(0);
> 41:         }
> 42:       };

Why is this a lambda rather than a (private) ordinary member function?  There's a bunch of these.  Use of lambda reduces the separation of the code from it's use, but that doesn't seem to me worth the additional clutter.  I'd rather a well-named helper function here, esp. since in this case would be the same helper as used by write_lock.  Call it await_write_unlock.  Consider putting the thread-type handling in the helper too, keeping this function small.

src/hotspot/share/utilities/readWriteLock.cpp line 64:

> 62: void ReadWriteLock::read_unlock() {
> 63:   for (;;) {
> 64:     const int32_t count = Atomic::load_acquire(&_count);

I think an invariant here is that count is never 0 or -1.  Consider asserting that.

src/hotspot/share/utilities/readWriteLock.cpp line 67:

> 65: 
> 66:     if (count > 0) {
> 67:       // No writer in progress, try to decrement reader count.

s/in progress/pending/

src/hotspot/share/utilities/readWriteLock.cpp line 72:

> 70:       }
> 71:     } else {
> 72:       // Writer in progress, try to increment reader count.

s/in progress/pending/

src/hotspot/share/utilities/readWriteLock.cpp line 81:

> 79:       if (count == -2) {
> 80:         Locker locker(&_mon);
> 81:         _mon.notify_all();

Unfortunately, this also wakes up all pending readers.  I wonder if there's a way to avoid that by having two monitors (only having monitors and not bare condvars makes that harder).

src/hotspot/share/utilities/readWriteLock.cpp line 87:

> 85:     return;
> 86:   }
> 87: }

I found the control structure of read_unlock, with the use of continue and
eventual "fall through" to the exit, a bit confusing and requiring lining up
or counting braces. I think something like this (totally untested, but
intended to be equivalent except it should have comments) is easier to
follow:


void ReadWriteLock::read_unlock() {
  for (;;) {
    const int32_t count = Atomic::load_acquire(&_count);
    if (count > 0) {
      if (Atomic::cmpxchg(&_count, count, count - 1) == count) {
        break;
      }
    } else if (Atomic::cmpxchg(&_count, count, count + 1) == count) {
      if (count == -2) {
        Locker locker(&_mon);
        _mon.notify_all();
      }
      break;
    }
  }
}


I had a similar reaction to read_lock and write_lock.

src/hotspot/share/utilities/readWriteLock.cpp line 139:

> 137:     return;
> 138:   }
> 139: }

I was wondering why a lock for write doesn't hold the underlying
PlatformMonitor across the scope of the mutex region. Something like this
(completely untested)


void ReadWriteLock::write_lock(Thread* current) {
  _mon.lock();
  while (true) {
    const int32_t count = Atomic::load_acquire(&_count);
    if (count < 0) {
      // Some other writer is waiting for readers to complete.
      // Approx:
      // while (Atomic::load_acquire(&_count)) _mon.wait();
      await_write_unlock(current);
    } else if (Atomic::cmpxchg(&_count, count, -(count + 1)) == count) {
      // Claimed the write slot, but there might be active readers.
      if (count != 0) {
        // Approx:
        // while (Atomic::load_acquire(&_count) != -1) _mon.wait();
        await_no_active_readers(current);
      }
      return; // return still holding lock
    } // else failed to claim write ownership, so try again.
  }
}

void ReadWriteLock::write_unlock() {
  assert(Atomic::load(&_count) == -1, "invariant");
  Atomic::release_store(&_count, (int32_t)0);
  _mon.notify_all();
  _mon.unlock();
}


This seems simpler to me.  Am I missing something?

And looking at this also makes me wonder if `_mon` can just be a normal
HotSpot `Monitor`, which would resolve many of the questions and comments I've
made that are mostly driven by the direct use of `PlatformMonitor`.

src/hotspot/share/utilities/readWriteLock.hpp line 82:

> 80:   //
> 81:   volatile int32_t _count;
> 82: 

This class should be noncopyable.  Maybe it is implicitly so via the `PlatformMonitor` member, but would be better to be explicit.

src/hotspot/share/utilities/readWriteLock.hpp line 89:

> 87:   }
> 88: 
> 89:   ~ReadWriteLock() {

Use default destructor (either implicit or explicit) rather than empty body destructor.

src/hotspot/share/utilities/readWriteLock.hpp line 97:

> 95:   void read_lock(Thread* current = Thread::current());
> 96:   void read_unlock();
> 97: };

There should be RAII classes associated with locking for read and locking for write.

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

PR: https://git.openjdk.org/jdk/pull/9838