Merge pull request #61352 from al45tair/eng/PR-100236038-2

[Threading] Improve ConditionVariable wait behaviour

Author: al45tair

include/swift/Threading/Impl/C11.h

@@ -18,11 +18,12 @@
 #define SWIFT_THREADING_IMPL_C11_H
 
 #include <atomic>
-#include <chrono>
 #include <cstdint>
 
 #include <threads.h>
 
+#include "chrono_utils.h"
+
 #include "llvm/ADT/Optional.h"
 
 #include "swift/Threading/Errors.h"
@@ -180,12 +181,14 @@ inline void cond_wait(cond_handle &handle) {
 template <class Rep, class Period>
 inline bool cond_wait(cond_handle &handle,
                       std::chrono::duration<Rep, Period> duration) {
-  auto deadline = std::chrono::system_clock::now() + duration;
+  auto to_wait = chrono_utils::ceil<
+    std::chrono::system_clock::duration>(duration);
+  auto deadline = std::chrono::system_clock::now() + to_wait;
   return cond_wait(handle, deadline);
 }
 inline bool cond_wait(cond_handle &handle,
                       std::chrono::system_clock::time_point deadline) {
-  auto ns = std::chrono::duration_cast<std::chrono::nanoseconds>(
+  auto ns = chrono_utils::ceil<std::chrono::nanoseconds>(
     deadline.time_since_epoch()).count();
   struct ::timespec ts = { ::time_t(ns / 1000000000), long(ns % 1000000000) };
   SWIFT_C11THREADS_RETURN_TRUE_OR_FALSE(

include/swift/Threading/Impl/Darwin.h

@@ -21,7 +21,7 @@
 #include <os/lock.h>
 #include <pthread.h>
 
-#include <chrono>
+#include "chrono_utils.h"
 
 #include "llvm/ADT/Optional.h"
 
@@ -157,14 +157,14 @@ inline void cond_wait(cond_handle &handle) {
 template <class Rep, class Period>
 inline bool cond_wait(cond_handle &handle,
                       std::chrono::duration<Rep, Period> duration) {
-  auto deadline = std::chrono::time_point_cast<
-    std::chrono::system_clock::duration>(std::chrono::system_clock::now()
-                                         + duration);
+  auto to_wait = chrono_utils::ceil<
+    std::chrono::system_clock::duration>(duration);
+  auto deadline = std::chrono::system_clock::now() + to_wait;
   return cond_wait(handle, deadline);
 }
 inline bool cond_wait(cond_handle &handle,
                       std::chrono::system_clock::time_point deadline) {
-  auto ns = std::chrono::duration_cast<std::chrono::nanoseconds>(
+  auto ns = chrono_utils::ceil<std::chrono::nanoseconds>(
     deadline.time_since_epoch()).count();
   struct ::timespec ts = { ::time_t(ns / 1000000000), long(ns % 1000000000) };
   SWIFT_PTHREADS_RETURN_TRUE_OR_FALSE(

include/swift/Threading/Impl/Linux.h

@@ -21,9 +21,10 @@
 #include <pthread.h>
 
 #include <atomic>
-#include <chrono>
 #include <optional>
 
+#include "chrono_utils.h"
+
 #include "llvm/ADT/Optional.h"
 
 #include "swift/Threading/Errors.h"
@@ -167,14 +168,14 @@ inline void cond_wait(cond_handle &handle) {
 template <class Rep, class Period>
 inline bool cond_wait(cond_handle &handle,
                       std::chrono::duration<Rep, Period> duration) {
-  auto deadline = std::chrono::time_point_cast<
-    std::chrono::system_clock::duration>(std::chrono::system_clock::now()
-                                         + duration);
+  auto to_wait = chrono_utils::ceil<
+    std::chrono::system_clock::duration>(duration);
+  auto deadline = std::chrono::system_clock::now() + to_wait;
   return cond_wait(handle, deadline);
 }
 inline bool cond_wait(cond_handle &handle,
                       std::chrono::system_clock::time_point deadline) {
-  auto ns = std::chrono::duration_cast<std::chrono::nanoseconds>(
+  auto ns = chrono_utils::ceil<std::chrono::nanoseconds>(
     deadline.time_since_epoch()).count();
   struct ::timespec ts = { ::time_t(ns / 1000000000), long(ns % 1000000000) };
   SWIFT_LINUXTHREADS_RETURN_TRUE_OR_FALSE(

include/swift/Threading/Impl/Nothreads.h

@@ -19,7 +19,7 @@
 
 #include "llvm/ADT/Optional.h"
 
-#include <chrono>
+#include "chrono_utils.h"
 
 namespace swift {
 namespace threading_impl {

include/swift/Threading/Impl/Pthreads.h

@@ -21,9 +21,10 @@
 #include <pthread.h>
 
 #include <atomic>
-#include <chrono>
 #include <cstdint>
 
+#include "chrono_utils.h"
+
 #include "llvm/ADT/Optional.h"
 
 #include "swift/Threading/Errors.h"
@@ -163,14 +164,14 @@ inline void cond_wait(cond_handle &handle) {
 template <class Rep, class Period>
 inline bool cond_wait(cond_handle &handle,
                       std::chrono::duration<Rep, Period> duration) {
-  auto deadline = std::chrono::time_point_cast<
-    std::chrono::system_clock::duration>(std::chrono::system_clock::now()
-                                         + duration);
+  auto to_wait = chrono_utils::ceil<
+    std::chrono::system_clock::duration>(duration);
+  auto deadline = std::chrono::system_clock::now() + to_wait;
   return cond_wait(handle, deadline);
 }
 inline bool cond_wait(cond_handle &handle,
                       std::chrono::system_clock::time_point deadline) {
-  auto ns = std::chrono::duration_cast<std::chrono::nanoseconds>(
+  auto ns = chrono_utils::ceil<std::chrono::nanoseconds>(
     deadline.time_since_epoch()).count();
   struct ::timespec ts = { ::time_t(ns / 1000000000), long(ns % 1000000000) };
   SWIFT_PTHREADS_RETURN_TRUE_OR_FALSE(

include/swift/Threading/Impl/Win32.h

@@ -19,7 +19,8 @@
 
 #include "Win32/Win32Defs.h"
 
-#include <chrono>
+#include "chrono_utils.h"
+
 #include <atomic>
 
 #include "llvm/ADT/Optional.h"
@@ -120,10 +121,77 @@ inline void cond_wait(cond_handle &handle) {
 template <class Rep, class Period>
 inline bool cond_wait(cond_handle &handle,
                       std::chrono::duration<Rep, Period> duration) {
-  auto ms = std::chrono::duration_cast<std::chrono::milliseconds>(duration);
+  auto ms = chrono_utils::ceil<std::chrono::milliseconds>(duration);
+
+  /* If you are paying attention to the next line, you are now asking
+
+       "Why are you adding 16 - that seems mad?"
+
+     To explain, we need to understand how the Sleep...() APIs in
+     Windows work.
+
+     The Windows kernel runs a timer, the system tick, which is
+     used for scheduling; the timer in question, for historical
+     reasons, by default runs at 64Hz.  Every time the timer fires,
+     Windows updates the tick count, schedules processes and so on.
+
+     When you ask to sleep, there are two cases:
+
+     1. You've asked to sleep for less than a tick, or
+
+     2. You've asked to sleep for at least a tick.
+
+     In case 1, the sleep functions appear to run a delay loop; this
+     is by its very nature inaccurate and you may or may not wait less
+     than the time you requested.  You might also get rescheduled,
+     in which case you'll wait at least a whole tick.
+
+     In case 2, Windows appears to be adding the requested wait to
+     its current tick count to work out when to wake your thread.
+     That *sounds* sensible, until you realise that if it does this
+     towards the end of a tick period, you will wait for that much
+     less time.  i.e. the sleep functions will return *early* by
+     up to one tick period.
+
+     This is especially unfortunate if you ask to wait for exactly
+     one tick period, because you will end up waiting for anything
+     between no time at all and a full tick period.
+
+     To complicate matters, the system tick rate might not be 64Hz;
+     the multimedia timer API can cause it to change to as high as
+     1kHz, and on *some* machines this happens globally for all
+     processes, while on *other* machines the kernel is actually using
+     a separate system tick rate and merely pretending to Win32
+     processes that things still work this way.
+
+     On other platforms, these kinds of functions are guaranteed to
+     wait for at least the time you requested, and we'd like for that
+     to be true for the Threading package's APIs here.  One way to
+     achieve that is to add a whole tick's worth of milliseconds
+     to the time we're requesting to wait for.  In that case, we'll
+     avoid the delay loop from case (1), and we'll also guarantee that
+     we wait for at least the amount of time the caller of this
+     function expected.
+
+     It would be nice if there were a Windows API we could call to
+     obtain the current system tick period.  The Internet suggests
+     that the undocumented call NtQueryTimerResolution() might be of
+     some use for this, but it turns out that that just gives you
+     the kernel's idea of the system tick period, which isn't the
+     same as Win32's idea necessarily.  e.g. on my test system, I
+     can see that the tick period is 15.625ms, while that call tells
+     me 1ms.
+
+     We don't want to change the system tick rate using the multimedia
+     timer API mentioned earlier, because on some machines that is
+     global and will use extra power and CPU cycles.
+
+     The upshot is that the best choice here appears to be to add
+     15.625ms (1/64s) to the time we're asking to wait.  That rounds
+     up to 16ms, which is why there's a +16. */
   return SleepConditionVariableSRW(&handle.condition,
                                    &handle.lock,
-                                   DWORD(ms.count()),
+                                   DWORD(ms.count()) + 16,
                                    0);
 }
 inline bool cond_wait(cond_handle &handle,

include/swift/Threading/Impl/chrono_utils.h

@@ -0,0 +1,71 @@
+//===--- chrono_utils.h - Utility functions for duration ------ -*- C++ -*-===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2022 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+//
+// Specifically, we want ceil() for these types, but that's only available in
+// C++17, and we need to build with C++14, so... include a version of the
+// necesary code here.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef SWIFT_THREADING_IMPL_CHRONO_UTILS_H
+#define SWIFT_THREADING_IMPL_CHRONO_UTILS_H
+
+#include <chrono>
+#include <type_traits>
+
+namespace swift {
+namespace threading_impl {
+namespace chrono_utils {
+
+#if __cplusplus >= 201703L
+using std::chrono::ceil;
+#else
+
+namespace detail {
+template <class _Tp>
+struct is_duration : std::false_type {};
+
+template <class _Rep, class _Period>
+struct is_duration<std::chrono::duration<_Rep, _Period> >
+  : std::true_type  {};
+
+template <class _Rep, class _Period>
+struct is_duration<const std::chrono::duration<_Rep, _Period> >
+  : std::true_type  {};
+
+template <class _Rep, class _Period>
+struct is_duration<volatile std::chrono::duration<_Rep, _Period> >
+  : std::true_type  {};
+
+template <class _Rep, class _Period>
+struct is_duration<const volatile std::chrono::duration<_Rep, _Period> >
+  : std::true_type  {};
+}
+
+template <class To, class Rep, class Period,
+          class = std::enable_if_t<detail::is_duration<To>::value>>
+constexpr To
+ceil(const std::chrono::duration<Rep, Period>& d)
+{
+  To t = std::chrono::duration_cast<To>(d);
+  if (t < d)
+    t = t + To{1};
+  return t;
+}
+
+#endif
+
+} // namespace chrono_utils
+} // namespace threading_impl
+} // namespace swift
+
+#endif // SWIFT_THREADING_IMPL_CHRONO_UTILS_H