[GR-34100] ParkEvent improvements.

PullRequest: graal/13155

Author: peter-hofer

substratevm/mx.substratevm/suite.py

@@ -428,6 +428,9 @@
                 "com.oracle.svm.core.graal.aarch64",
             ],
             "requiresConcealed" : {
+                "java.base" : [
+                    "jdk.internal.misc",
+                ],
                 "jdk.internal.vm.ci" : [
                     "jdk.vm.ci.code",
                 ],

substratevm/src/com.oracle.svm.core.posix/src/com/oracle/svm/core/posix/headers/Pthread.java

@@ -147,6 +147,9 @@ public interface pthread_key_tPointer extends PointerBase {
     @CFunction(transition = Transition.NO_TRANSITION)
     public static native int pthread_mutex_init(pthread_mutex_t mutex, pthread_mutexattr_t mutexattr);
 
+    @CFunction(value = "pthread_mutex_trylock", transition = Transition.NO_TRANSITION)
+    public static native int pthread_mutex_trylock_no_transition(pthread_mutex_t mutex);
+
     @CFunction(transition = Transition.TO_NATIVE)
     public static native int pthread_mutex_lock(pthread_mutex_t mutex);
 

substratevm/src/com.oracle.svm.core.posix/src/com/oracle/svm/core/posix/pthread/PthreadConditionUtils.java

@@ -63,7 +63,7 @@ public static int initCondition(Pthread.pthread_cond_t cond) {
         return Pthread.pthread_cond_init(cond, attr);
     }
 
-    @Uninterruptible(reason = "Called from uninterruptible code.")
+    @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
     private static void getAbsoluteTimeNanos(timespec result) {
         /*
          * We need the real-time clock to compute absolute deadlines when a conditional wait should
@@ -88,15 +88,15 @@ private static void getAbsoluteTimeNanos(timespec result) {
         }
     }
 
-    /** Turn a delay in nanoseconds into a deadline in a Time.timespec. */
-    @Uninterruptible(reason = "Called from uninterruptible code.")
-    public static void delayNanosToDeadlineTimespec(long delayNanos, Time.timespec result) {
+    /** Turn a duration in nanoseconds into a deadline in a Time.timespec. */
+    @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
+    public static void durationNanosToDeadlineTimespec(long durationNanos, Time.timespec result) {
         timespec currentTimespec = StackValue.get(timespec.class);
         getAbsoluteTimeNanos(currentTimespec);
 
-        assert delayNanos >= 0;
-        long sec = TimeUtils.addOrMaxValue(currentTimespec.tv_sec(), TimeUtils.divideNanosToSeconds(delayNanos));
-        long nsec = currentTimespec.tv_nsec() + TimeUtils.remainderNanosToSeconds(delayNanos);
+        assert durationNanos >= 0;
+        long sec = TimeUtils.addOrMaxValue(currentTimespec.tv_sec(), TimeUtils.divideNanosToSeconds(durationNanos));
+        long nsec = currentTimespec.tv_nsec() + TimeUtils.remainderNanosToSeconds(durationNanos);
         if (nsec >= TimeUtils.nanosPerSecond) {
             sec = TimeUtils.addOrMaxValue(sec, 1);
             nsec -= TimeUtils.nanosPerSecond;
@@ -108,7 +108,7 @@ public static void delayNanosToDeadlineTimespec(long delayNanos, Time.timespec r
     }
 
     @Uninterruptible(reason = "Called from uninterruptible code.")
-    public static long deadlineTimespecToDelayNanos(Time.timespec deadlineTimespec) {
+    public static long deadlineTimespecToDurationNanos(Time.timespec deadlineTimespec) {
         timespec currentTimespec = StackValue.get(timespec.class);
         getAbsoluteTimeNanos(currentTimespec);
 

substratevm/src/com.oracle.svm.core.posix/src/com/oracle/svm/core/posix/pthread/PthreadVMLockSupport.java

@@ -315,7 +315,7 @@ public void blockNoTransitionUnspecifiedOwner() {
     @Override
     public long block(long waitNanos) {
         Time.timespec deadlineTimespec = UnsafeStackValue.get(Time.timespec.class);
-        PthreadConditionUtils.delayNanosToDeadlineTimespec(waitNanos, deadlineTimespec);
+        PthreadConditionUtils.durationNanosToDeadlineTimespec(waitNanos, deadlineTimespec);
 
         mutex.clearCurrentThreadOwner();
         final int timedWaitResult = Pthread.pthread_cond_timedwait(getStructPointer(), ((PthreadVMMutex) getMutex()).getStructPointer(), deadlineTimespec);
@@ -326,14 +326,14 @@ public long block(long waitNanos) {
         }
         /* Check for other errors from the timed wait. */
         PthreadVMLockSupport.checkResult(timedWaitResult, "pthread_cond_timedwait");
-        return PthreadConditionUtils.deadlineTimespecToDelayNanos(deadlineTimespec);
+        return PthreadConditionUtils.deadlineTimespecToDurationNanos(deadlineTimespec);
     }
 
     @Override
     @Uninterruptible(reason = "Called from uninterruptible code.", callerMustBe = true)
     public long blockNoTransition(long waitNanos) {
         Time.timespec deadlineTimespec = StackValue.get(Time.timespec.class);
-        PthreadConditionUtils.delayNanosToDeadlineTimespec(waitNanos, deadlineTimespec);
+        PthreadConditionUtils.durationNanosToDeadlineTimespec(waitNanos, deadlineTimespec);
 
         mutex.clearCurrentThreadOwner();
         final int timedwaitResult = Pthread.pthread_cond_timedwait_no_transition(getStructPointer(), ((PthreadVMMutex) getMutex()).getStructPointer(), deadlineTimespec);
@@ -344,7 +344,7 @@ public long blockNoTransition(long waitNanos) {
         }
         /* Check for other errors from the timed wait. */
         PthreadVMLockSupport.checkResult(timedwaitResult, "pthread_cond_timedwait");
-        return PthreadConditionUtils.deadlineTimespecToDelayNanos(deadlineTimespec);
+        return PthreadConditionUtils.deadlineTimespecToDurationNanos(deadlineTimespec);
     }
 
     @Override

substratevm/src/com.oracle.svm.core.posix/src/com/oracle/svm/core/posix/thread/PosixPlatformThreads.java

@@ -60,6 +60,7 @@
 import com.oracle.svm.core.feature.AutomaticallyRegisteredImageSingleton;
 import com.oracle.svm.core.graal.stackvalue.UnsafeStackValue;
 import com.oracle.svm.core.log.Log;
+import com.oracle.svm.core.monitor.JavaMonitor;
 import com.oracle.svm.core.os.IsDefined;
 import com.oracle.svm.core.posix.PosixUtils;
 import com.oracle.svm.core.posix.headers.Errno;
@@ -75,9 +76,12 @@
 import com.oracle.svm.core.thread.ParkEvent;
 import com.oracle.svm.core.thread.ParkEvent.ParkEventFactory;
 import com.oracle.svm.core.thread.PlatformThreads;
+import com.oracle.svm.core.util.TimeUtils;
 import com.oracle.svm.core.util.UnsignedUtils;
 import com.oracle.svm.core.util.VMError;
 
+import jdk.internal.misc.Unsafe;
+
 @AutomaticallyRegisteredImageSingleton(PlatformThreads.class)
 public final class PosixPlatformThreads extends PlatformThreads {
 
@@ -287,16 +291,32 @@ final class Target_java_lang_Thread {
     Pthread.pthread_t pthreadIdentifier;
 }
 
-class PosixParkEvent extends ParkEvent {
+/**
+ * {@link PosixParkEvent} is based on HotSpot class {@code Parker} in {@code os_posix.cpp}, as of
+ * JDK 19 (git commit hash: f640fc5a1eb876a657d0de011dcd9b9a42b88eec, JDK tag: jdk-19+30).
+ * <p>
+ * HotSpot has two constructs with a similar purpose: {@code ParkEvent} and {@code Parker}. The
+ * latter implements JSR 166 synchronization primitives {@link Unsafe#park} and
+ * {@link Unsafe#unpark}, just like we do here, therefore we base this implementation on
+ * {@code Parker}. Our implementation of Java object monitors, {@link JavaMonitor}, uses the JSR 166
+ * primitives, so it can potentially experience interference from unrelated calls to
+ * {@link Unsafe#unpark}. This is a difference to HotSpot's {@code ObjectMonitor}, which uses a
+ * separate HotSpot {@code ParkEvent} instance. Another difference is that {@code Parker} and the
+ * code below return control to the caller on spurious wakeups, unlike HotSpot's {@code ParkEvent}.
+ * This does not affect correctness.
+ */
+final class PosixParkEvent extends ParkEvent {
+    private static final Unsafe U = Unsafe.getUnsafe();
+    private static final long EVENT_OFFSET = U.objectFieldOffset(PosixParkEvent.class, "event");
 
     private Pthread.pthread_mutex_t mutex;
     private Pthread.pthread_cond_t cond;
 
-    /**
-     * The ticket: false implies unavailable, true implies available. Volatile so it can be safely
-     * updated in {@link #reset()} without holding the lock.
-     */
-    protected volatile boolean event;
+    /** Permit: 1 if an unpark is pending, otherwise 0. */
+    private volatile int event = 0;
+
+    /** Whether the owner is currently parked. Guarded by {@link #mutex}. */
+    private boolean parked = false;
 
     PosixParkEvent() {
         // Allocate mutex and condition in a single step so that they are adjacent in memory.
@@ -308,61 +328,79 @@ class PosixParkEvent extends ParkEvent {
         final Pthread.pthread_mutexattr_t mutexAttr = WordFactory.nullPointer();
         PosixUtils.checkStatusIs0(Pthread.pthread_mutex_init(mutex, mutexAttr), "mutex initialization");
         PosixUtils.checkStatusIs0(PthreadConditionUtils.initCondition(cond), "condition variable initialization");
+        // Note: HotSpot has another pthread_cond_t without CLOCK_MONOTONIC for absolute timed waits
     }
 
     @Override
     protected void reset() {
-        event = false;
+        event = 0;
     }
 
     @Override
     protected void condWait() {
-        StackOverflowCheck.singleton().makeYellowZoneAvailable();
-        try {
-            PosixUtils.checkStatusIs0(Pthread.pthread_mutex_lock(mutex), "park(): mutex lock");
-            try {
-                while (!event) {
-                    int status = Pthread.pthread_cond_wait(cond, mutex);
-                    PosixUtils.checkStatusIs0(status, "park(): condition variable wait");
-                }
-                event = false;
-            } finally {
-                PosixUtils.checkStatusIs0(Pthread.pthread_mutex_unlock(mutex), "park(): mutex unlock");
-            }
-        } finally {
-            StackOverflowCheck.singleton().protectYellowZone();
+        park(false, 0);
+    }
+
+    @Override
+    protected void condTimedWait(long durationNanos) {
+        if (durationNanos > 0) {
+            park(false, durationNanos);
         }
     }
 
     @Override
-    protected void condTimedWait(long delayNanos) {
+    protected boolean tryFastPark() {
+        // We depend on getAndSet having full barrier semantics since we are not locking
+        return U.getAndSetInt(this, EVENT_OFFSET, 0) != 0;
+    }
+
+    @Override
+    protected void park(boolean isAbsolute, long time) {
+        if (time < 0 || (isAbsolute && time == 0)) {
+            return; // don't wait at all
+        }
         StackOverflowCheck.singleton().makeYellowZoneAvailable();
         try {
-            /* Encode the delay as a deadline in a Time.timespec. */
-            Time.timespec deadlineTimespec = UnsafeStackValue.get(Time.timespec.class);
-            PthreadConditionUtils.delayNanosToDeadlineTimespec(delayNanos, deadlineTimespec);
-
-            PosixUtils.checkStatusIs0(Pthread.pthread_mutex_lock(mutex), "park(long): mutex lock");
+            int status = Pthread.pthread_mutex_trylock_no_transition(mutex);
+            if (status == Errno.EBUSY()) {
+                return; // can only mean another thread is unparking us: don't wait
+            }
+            PosixUtils.checkStatusIs0(status, "park: mutex trylock");
             try {
-                while (!event) {
-                    int status = Pthread.pthread_cond_timedwait(cond, mutex, deadlineTimespec);
-                    if (status == Errno.ETIMEDOUT()) {
-                        break;
-                    } else if (status != 0) {
-                        Log.log().newline()
-                                        .string("[PosixParkEvent.condTimedWait(delayNanos: ").signed(delayNanos).string("): Should not reach here.")
-                                        .string("  mutex: ").hex(mutex)
-                                        .string("  cond: ").hex(cond)
-                                        .string("  deadlineTimeSpec.tv_sec: ").signed(deadlineTimespec.tv_sec())
-                                        .string("  deadlineTimespec.tv_nsec: ").signed(deadlineTimespec.tv_nsec())
-                                        .string("  status: ").signed(status).string(" ").string(Errno.strerror(status))
-                                        .string("]").newline();
-                        PosixUtils.checkStatusIs0(status, "park(long): condition variable timed wait");
+                if (event == 0) {
+                    assert !parked;
+                    parked = true;
+                    if (!isAbsolute && time == 0) {
+                        status = Pthread.pthread_cond_wait(cond, mutex);
+                        PosixUtils.checkStatusIs0(status, "park(): condition variable wait");
+                    } else {
+                        long durationNanos = TimeUtils.durationNanos(isAbsolute, time);
+                        Time.timespec deadlineTimespec = UnsafeStackValue.get(Time.timespec.class);
+                        PthreadConditionUtils.durationNanosToDeadlineTimespec(durationNanos, deadlineTimespec);
+
+                        status = Pthread.pthread_cond_timedwait(cond, mutex, deadlineTimespec);
+                        if (status != 0 && status != Errno.ETIMEDOUT()) {
+                            Log.log().newline()
+                                            .string("[PosixParkEvent.park(durationNanos: ").signed(durationNanos).string("): Should not reach here.")
+                                            .string("  mutex: ").hex(mutex)
+                                            .string("  cond: ").hex(cond)
+                                            .string("  deadlineTimeSpec.tv_sec: ").signed(deadlineTimespec.tv_sec())
+                                            .string("  deadlineTimespec.tv_nsec: ").signed(deadlineTimespec.tv_nsec())
+                                            .string("  status: ").signed(status).string(" ").string(Errno.strerror(status))
+                                            .string("]").newline();
+                            PosixUtils.checkStatusIs0(status, "park(boolean, long): condition variable timed wait");
+                        }
                     }
+                    parked = false;
                 }
-                event = false;
+                event = 0;
+
             } finally {
-                PosixUtils.checkStatusIs0(Pthread.pthread_mutex_unlock(mutex), "park(long): mutex unlock");
+                PosixUtils.checkStatusIs0(Pthread.pthread_mutex_unlock(mutex), "park: mutex unlock");
+
+                // Paranoia to ensure our locked and lock-free paths interact
+                // correctly with each other and Java-level accesses.
+                U.fullFence();
             }
         } finally {
             StackOverflowCheck.singleton().protectYellowZone();
@@ -373,13 +411,23 @@ protected void condTimedWait(long delayNanos) {
     protected void unpark() {
         StackOverflowCheck.singleton().makeYellowZoneAvailable();
         try {
+            int s;
+            boolean p;
             PosixUtils.checkStatusIs0(Pthread.pthread_mutex_lock(mutex), "PosixParkEvent.unpark(): mutex lock");
             try {
-                event = true;
-                PosixUtils.checkStatusIs0(Pthread.pthread_cond_broadcast(cond), "PosixParkEvent.unpark(): condition variable broadcast");
+                s = event;
+                event = 1;
+                p = parked;
             } finally {
                 PosixUtils.checkStatusIs0(Pthread.pthread_mutex_unlock(mutex), "PosixParkEvent.unpark(): mutex unlock");
             }
+            if (s == 0 && p) {
+                /*
+                 * Signal without holding the mutex, which is safe and avoids futile wakeups if the
+                 * platform does not implement wait morphing.
+                 */
+                PosixUtils.checkStatusIs0(Pthread.pthread_cond_signal(cond), "PosixParkEvent.unpark(): condition variable signal");
+            }
         } finally {
             StackOverflowCheck.singleton().protectYellowZone();
         }
@@ -400,9 +448,4 @@ class PosixParkEventFactory implements ParkEventFactory {
     public ParkEvent acquire() {
         return new PosixParkEvent();
     }
-
-    @Override
-    public boolean usesParkEventList() {
-        return false;
-    }
 }

substratevm/src/com.oracle.svm.core.windows/src/com/oracle/svm/core/windows/WindowsPlatformThreads.java

@@ -231,13 +231,13 @@ protected void condWait() {
     }
 
     @Override
-    protected void condTimedWait(long delayNanos) {
+    protected void condTimedWait(long durationNanos) {
         StackOverflowCheck.singleton().makeYellowZoneAvailable();
         try {
             final int maxTimeout = 0x10_000_000;
-            long delayMillis = Math.max(0, TimeUtils.roundUpNanosToMillis(delayNanos));
+            long durationMillis = Math.max(0, TimeUtils.roundUpNanosToMillis(durationNanos));
             do { // at least once to consume potential unpark
-                int timeout = (delayMillis < maxTimeout) ? (int) delayMillis : maxTimeout;
+                int timeout = (durationMillis < maxTimeout) ? (int) durationMillis : maxTimeout;
                 int status = SynchAPI.WaitForSingleObject(eventHandle, timeout);
                 if (status == SynchAPI.WAIT_OBJECT_0()) {
                     break; // unparked
@@ -246,8 +246,8 @@ protected void condTimedWait(long delayNanos) {
                     Log.log().newline().string("GetLastError returned:  ").hex(WinBase.GetLastError()).newline();
                     throw VMError.shouldNotReachHere("WaitForSingleObject failed");
                 }
-                delayMillis -= timeout;
-            } while (delayMillis > 0);
+                durationMillis -= timeout;
+            } while (durationMillis > 0);
         } finally {
             StackOverflowCheck.singleton().protectYellowZone();
         }
@@ -279,9 +279,4 @@ class WindowsParkEventFactory implements ParkEventFactory {
     public ParkEvent acquire() {
         return new WindowsParkEvent();
     }
-
-    @Override
-    public boolean usesParkEventList() {
-        return false;
-    }
 }