Skip to content

[Threading] Always round up when converting std::chrono::durations. #61352

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Sep 30, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions include/swift/Threading/Impl/C11.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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(
Expand Down
10 changes: 5 additions & 5 deletions include/swift/Threading/Impl/Darwin.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
#include <os/lock.h>
#include <pthread.h>

#include <chrono>
#include "chrono_utils.h"

#include "llvm/ADT/Optional.h"

Expand Down Expand Up @@ -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(
Expand Down
11 changes: 6 additions & 5 deletions include/swift/Threading/Impl/Linux.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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(
Expand Down
2 changes: 1 addition & 1 deletion include/swift/Threading/Impl/Nothreads.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

#include "llvm/ADT/Optional.h"

#include <chrono>
#include "chrono_utils.h"

namespace swift {
namespace threading_impl {
Expand Down
11 changes: 6 additions & 5 deletions include/swift/Threading/Impl/Pthreads.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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(
Expand Down
74 changes: 71 additions & 3 deletions include/swift/Threading/Impl/Win32.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@

#include "Win32/Win32Defs.h"

#include <chrono>
#include "chrono_utils.h"

#include <atomic>

#include "llvm/ADT/Optional.h"
Expand Down Expand Up @@ -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,
Expand Down
71 changes: 71 additions & 0 deletions include/swift/Threading/Impl/chrono_utils.h
Original file line number Diff line number Diff line change
@@ -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