From 99c17fbf338acc43e692a49151d0f485de475356 Mon Sep 17 00:00:00 2001 From: David Holmes Date: Tue, 3 Sep 2019 23:42:06 -0400 Subject: [PATCH] 6313903: Thread.sleep(3) might wake up immediately on windows Reviewed-by: rehn, dcubed, rriggs --- src/hotspot/os/posix/os_posix.cpp | 72 -------------- src/hotspot/os/windows/os_windows.cpp | 134 ++++++++++---------------- src/hotspot/share/runtime/os.cpp | 83 ++++++++++++++++ 3 files changed, 134 insertions(+), 155 deletions(-) diff --git a/src/hotspot/os/posix/os_posix.cpp b/src/hotspot/os/posix/os_posix.cpp index cd73248bc9d..17db3a93627 100644 --- a/src/hotspot/os/posix/os_posix.cpp +++ b/src/hotspot/os/posix/os_posix.cpp @@ -624,78 +624,6 @@ char* os::build_agent_function_name(const char *sym_name, const char *lib_name, return agent_entry_name; } -int os::sleep(Thread* thread, jlong millis, bool interruptible) { - assert(thread == Thread::current(), "thread consistency check"); - - ParkEvent * const slp = thread->_SleepEvent ; - slp->reset() ; - OrderAccess::fence() ; - - if (interruptible) { - jlong prevtime = javaTimeNanos(); - - for (;;) { - if (os::is_interrupted(thread, true)) { - return OS_INTRPT; - } - - jlong newtime = javaTimeNanos(); - - if (newtime - prevtime < 0) { - // time moving backwards, should only happen if no monotonic clock - // not a guarantee() because JVM should not abort on kernel/glibc bugs - assert(!os::supports_monotonic_clock(), "unexpected time moving backwards detected in os::sleep(interruptible)"); - } else { - millis -= (newtime - prevtime) / NANOSECS_PER_MILLISEC; - } - - if (millis <= 0) { - return OS_OK; - } - - prevtime = newtime; - - { - assert(thread->is_Java_thread(), "sanity check"); - JavaThread *jt = (JavaThread *) thread; - ThreadBlockInVM tbivm(jt); - OSThreadWaitState osts(jt->osthread(), false /* not Object.wait() */); - - jt->set_suspend_equivalent(); - // cleared by handle_special_suspend_equivalent_condition() or - // java_suspend_self() via check_and_wait_while_suspended() - - slp->park(millis); - - // were we externally suspended while we were waiting? - jt->check_and_wait_while_suspended(); - } - } - } else { - OSThreadWaitState osts(thread->osthread(), false /* not Object.wait() */); - jlong prevtime = javaTimeNanos(); - - for (;;) { - // It'd be nice to avoid the back-to-back javaTimeNanos() calls on - // the 1st iteration ... - jlong newtime = javaTimeNanos(); - - if (newtime - prevtime < 0) { - // time moving backwards, should only happen if no monotonic clock - // not a guarantee() because JVM should not abort on kernel/glibc bugs - assert(!os::supports_monotonic_clock(), "unexpected time moving backwards detected on os::sleep(!interruptible)"); - } else { - millis -= (newtime - prevtime) / NANOSECS_PER_MILLISEC; - } - - if (millis <= 0) break ; - - prevtime = newtime; - slp->park(millis); - } - return OS_OK ; - } -} void os::naked_short_nanosleep(jlong ns) { struct timespec req; diff --git a/src/hotspot/os/windows/os_windows.cpp b/src/hotspot/os/windows/os_windows.cpp index f27359ec176..5d7b5c702d9 100644 --- a/src/hotspot/os/windows/os_windows.cpp +++ b/src/hotspot/os/windows/os_windows.cpp @@ -497,7 +497,10 @@ static OSThread* create_os_thread(Thread* thread, HANDLE thread_handle, OSThread* osthread = new OSThread(NULL, NULL); if (osthread == NULL) return NULL; - // Initialize support for Java interrupts + // Initialize the JDK library's interrupt event. + // This should really be done when OSThread is constructed, + // but there is no way for a constructor to report failure to + // allocate the event. HANDLE interrupt_event = CreateEvent(NULL, true, false, NULL); if (interrupt_event == NULL) { delete osthread; @@ -599,7 +602,10 @@ bool os::create_thread(Thread* thread, ThreadType thr_type, return false; } - // Initialize support for Java interrupts + // Initialize the JDK library's interrupt event. + // This should really be done when OSThread is constructed, + // but there is no way for a constructor to report failure to + // allocate the event. HANDLE interrupt_event = CreateEvent(NULL, true, false, NULL); if (interrupt_event == NULL) { delete osthread; @@ -3480,87 +3486,7 @@ void os::pd_start_thread(Thread* thread) { assert(ret != SYS_THREAD_ERROR, "StartThread failed"); // should propagate back } -class HighResolutionInterval : public CHeapObj { - // The default timer resolution seems to be 10 milliseconds. - // (Where is this written down?) - // If someone wants to sleep for only a fraction of the default, - // then we set the timer resolution down to 1 millisecond for - // the duration of their interval. - // We carefully set the resolution back, since otherwise we - // seem to incur an overhead (3%?) that we don't need. - // CONSIDER: if ms is small, say 3, then we should run with a high resolution time. - // Buf if ms is large, say 500, or 503, we should avoid the call to timeBeginPeriod(). - // Alternatively, we could compute the relative error (503/500 = .6%) and only use - // timeBeginPeriod() if the relative error exceeded some threshold. - // timeBeginPeriod() has been linked to problems with clock drift on win32 systems and - // to decreased efficiency related to increased timer "tick" rates. We want to minimize - // (a) calls to timeBeginPeriod() and timeEndPeriod() and (b) time spent with high - // resolution timers running. - private: - jlong resolution; - public: - HighResolutionInterval(jlong ms) { - resolution = ms % 10L; - if (resolution != 0) { - MMRESULT result = timeBeginPeriod(1L); - } - } - ~HighResolutionInterval() { - if (resolution != 0) { - MMRESULT result = timeEndPeriod(1L); - } - resolution = 0L; - } -}; - -int os::sleep(Thread* thread, jlong ms, bool interruptable) { - jlong limit = (jlong) MAXDWORD; - - while (ms > limit) { - int res; - if ((res = sleep(thread, limit, interruptable)) != OS_TIMEOUT) { - return res; - } - ms -= limit; - } - - assert(thread == Thread::current(), "thread consistency check"); - OSThread* osthread = thread->osthread(); - OSThreadWaitState osts(osthread, false /* not Object.wait() */); - int result; - if (interruptable) { - assert(thread->is_Java_thread(), "must be java thread"); - JavaThread *jt = (JavaThread *) thread; - ThreadBlockInVM tbivm(jt); - - jt->set_suspend_equivalent(); - // cleared by handle_special_suspend_equivalent_condition() or - // java_suspend_self() via check_and_wait_while_suspended() - - HANDLE events[1]; - events[0] = osthread->interrupt_event(); - HighResolutionInterval *phri=NULL; - if (!ForceTimeHighResolution) { - phri = new HighResolutionInterval(ms); - } - if (WaitForMultipleObjects(1, events, FALSE, (DWORD)ms) == WAIT_TIMEOUT) { - result = OS_TIMEOUT; - } else { - ResetEvent(osthread->interrupt_event()); - osthread->set_interrupted(false); - result = OS_INTRPT; - } - delete phri; //if it is NULL, harmless - // were we externally suspended while we were waiting? - jt->check_and_wait_while_suspended(); - } else { - assert(!thread->is_Java_thread(), "must not be java thread"); - Sleep((long) ms); - result = OS_TIMEOUT; - } - return result; -} // Short sleep, direct OS call. // @@ -3687,6 +3613,9 @@ void os::interrupt(Thread* thread) { ParkEvent * ev = thread->_ParkEvent; if (ev != NULL) ev->unpark(); + + ev = thread->_SleepEvent; + if (ev != NULL) ev->unpark(); } @@ -5170,6 +5099,40 @@ bool os::ThreadCrashProtection::call(os::CrashProtectionCallback& cb) { return success; } + +class HighResolutionInterval : public CHeapObj { + // The default timer resolution seems to be 10 milliseconds. + // (Where is this written down?) + // If someone wants to sleep for only a fraction of the default, + // then we set the timer resolution down to 1 millisecond for + // the duration of their interval. + // We carefully set the resolution back, since otherwise we + // seem to incur an overhead (3%?) that we don't need. + // CONSIDER: if ms is small, say 3, then we should run with a high resolution time. + // Buf if ms is large, say 500, or 503, we should avoid the call to timeBeginPeriod(). + // Alternatively, we could compute the relative error (503/500 = .6%) and only use + // timeBeginPeriod() if the relative error exceeded some threshold. + // timeBeginPeriod() has been linked to problems with clock drift on win32 systems and + // to decreased efficiency related to increased timer "tick" rates. We want to minimize + // (a) calls to timeBeginPeriod() and timeEndPeriod() and (b) time spent with high + // resolution timers running. + private: + jlong resolution; + public: + HighResolutionInterval(jlong ms) { + resolution = ms % 10L; + if (resolution != 0) { + MMRESULT result = timeBeginPeriod(1L); + } + } + ~HighResolutionInterval() { + if (resolution != 0) { + MMRESULT result = timeEndPeriod(1L); + } + resolution = 0L; + } +}; + // An Event wraps a win32 "CreateEvent" kernel handle. // // We have a number of choices regarding "CreateEvent" win32 handle leakage: @@ -5205,7 +5168,7 @@ bool os::ThreadCrashProtection::call(os::CrashProtectionCallback& cb) { // 1. Reconcile Doug's JSR166 j.u.c park-unpark with the objectmonitor implementation. // 2. Consider wrapping the WaitForSingleObject(Ex) calls in SEH try/finally blocks // to recover from (or at least detect) the dreaded Windows 841176 bug. -// 3. Collapse the interrupt_event, the JSR166 parker event, and the objectmonitor ParkEvent +// 3. Collapse the JSR166 parker event, and the objectmonitor ParkEvent // into a single win32 CreateEvent() handle. // // Assumption: @@ -5278,11 +5241,16 @@ int os::PlatformEvent::park(jlong Millis) { if (Millis > MAXTIMEOUT) { prd = MAXTIMEOUT; } + HighResolutionInterval *phri = NULL; + if (!ForceTimeHighResolution) { + phri = new HighResolutionInterval(prd); + } rv = ::WaitForSingleObject(_ParkHandle, prd); assert(rv == WAIT_OBJECT_0 || rv == WAIT_TIMEOUT, "WaitForSingleObject failed"); if (rv == WAIT_TIMEOUT) { Millis -= prd; } + delete phri; // if it is NULL, harmless } v = _Event; _Event = 0; diff --git a/src/hotspot/share/runtime/os.cpp b/src/hotspot/share/runtime/os.cpp index 4a93eb97b14..aba22596226 100644 --- a/src/hotspot/share/runtime/os.cpp +++ b/src/hotspot/share/runtime/os.cpp @@ -1836,3 +1836,86 @@ os::SuspendResume::State os::SuspendResume::switch_state(os::SuspendResume::Stat return result; } #endif + +int os::sleep(Thread* thread, jlong millis, bool interruptible) { + assert(thread == Thread::current(), "thread consistency check"); + + ParkEvent * const slp = thread->_SleepEvent; + // Because there can be races with thread interruption sending an unpark() + // to the event, we explicitly reset it here to avoid an immediate return. + // The actual interrupt state will be checked before we park(). + slp->reset(); + // Thread interruption establishes a happens-before ordering in the + // Java Memory Model, so we need to ensure we synchronize with the + // interrupt state. + OrderAccess::fence(); + + if (interruptible) { + jlong prevtime = javaTimeNanos(); + + assert(thread->is_Java_thread(), "sanity check"); + JavaThread *jt = (JavaThread *) thread; + + for (;;) { + // interruption has precedence over timing out + if (os::is_interrupted(thread, true)) { + return OS_INTRPT; + } + + jlong newtime = javaTimeNanos(); + + if (newtime - prevtime < 0) { + // time moving backwards, should only happen if no monotonic clock + // not a guarantee() because JVM should not abort on kernel/glibc bugs + assert(!os::supports_monotonic_clock(), + "unexpected time moving backwards detected in os::sleep(interruptible)"); + } else { + millis -= (newtime - prevtime) / NANOSECS_PER_MILLISEC; + } + + if (millis <= 0) { + return OS_OK; + } + + prevtime = newtime; + + { + ThreadBlockInVM tbivm(jt); + OSThreadWaitState osts(jt->osthread(), false /* not Object.wait() */); + + jt->set_suspend_equivalent(); + // cleared by handle_special_suspend_equivalent_condition() or + // java_suspend_self() via check_and_wait_while_suspended() + + slp->park(millis); + + // were we externally suspended while we were waiting? + jt->check_and_wait_while_suspended(); + } + } + } else { + OSThreadWaitState osts(thread->osthread(), false /* not Object.wait() */); + jlong prevtime = javaTimeNanos(); + + for (;;) { + // It'd be nice to avoid the back-to-back javaTimeNanos() calls on + // the 1st iteration ... + jlong newtime = javaTimeNanos(); + + if (newtime - prevtime < 0) { + // time moving backwards, should only happen if no monotonic clock + // not a guarantee() because JVM should not abort on kernel/glibc bugs + assert(!os::supports_monotonic_clock(), + "unexpected time moving backwards detected on os::sleep(!interruptible)"); + } else { + millis -= (newtime - prevtime) / NANOSECS_PER_MILLISEC; + } + + if (millis <= 0) break ; + + prevtime = newtime; + slp->park(millis); + } + return OS_OK ; + } +}