Eliminate shutdown lag from the signal monitor #2766
Conversation
The simple `sleep_for` used to introduce a shutdown delay of up to 750ms to every client command invocation. This replaces the naive sleep with wakeup on event logic, which is reused to signal the monitoring thread to stop.
tobim
force-pushed
the
story/sc-39891/eliminate-signal-monitor-shutdown-lag
branch
from
c52f6b9
to
85d32d4
Compare
| @@ -41,24 +41,30 @@ extern "C" void signal_monitor_handler(int sig) { | |||
| } | |||
| signals[0] = true; | |||
| signals[sig] = true; | |||
| cvp->notify_one(); | |||
There was a problem hiding this comment.
The standard says:
The behavior of any function other than a [plain old function] used as a signal handler in a C++ program is implementation-defined.
The function notify_one is not from a "common subset", so the code is not well defined.
There was a problem hiding this comment.
TIL! I dismissed my approval.
I found this great blog post on an approach that is not UB from what I can tell. This works by blocking signals in all threads except for the signal monitor one, and then having the signal monitor thread use sigwait rather than a condition variable.
There was a problem hiding this comment.
I half expected that there was a catch. Well at least @dominiklohmann already found a blueprint for a more sound approach.
There was a problem hiding this comment.
This makes the integration tests run around 90 seconds faster on my machine in a release build. Wow. Crazy to think we had not noticed this since Feb 2019 where this was introduced in 92fe590.
| @@ -30,6 +29,7 @@ namespace { | |||
| // Keeps track of all signals by their value from 1 to 31. The flag at index 0 | |||
| // is used to tell whether a signal has been raised or not. | |||
| std::atomic<bool> signals[32]; | |||
| std::condition_variable* cvp; | |||
There was a problem hiding this comment.
If I understand the comment above correctly then this means that signals[0] is no longer required, because this condition variable serves the same purpose.
| static std::condition_variable cv; | ||
| static std::mutex m; |
There was a problem hiding this comment.
As far as I can tell these two only need to be in the header because run_guarded is defined inline, which it doesn't have to be.
| @@ -30,6 +29,7 @@ namespace { | |||
| // Keeps track of all signals by their value from 1 to 31. The flag at index 0 | |||
| // is used to tell whether a signal has been raised or not. | |||
| std::atomic<bool> signals[32]; | |||
| std::condition_variable* cvp; | |||
There was a problem hiding this comment.
Why do we need the cvp in addition to cv? Can't we always access cv directly?
| std::atomic<bool> signal_monitor::stop; | ||
| std::condition_variable signal_monitor::cv; | ||
| std::mutex signal_monitor::m; |
There was a problem hiding this comment.
You can get rid of these three lines if you mark the variables as inline static in the header.
dominiklohmann
dismissed
their stale review
Dismissing based on @mavam's review–did not know this was UB.
|
As explained in #2766 (comment) this is technically not allowed. |
tobim
deleted the
story/sc-39891/eliminate-signal-monitor-shutdown-lag
branch
The simple
sleep_forused to introduce a shutdown delay of up to 750ms to every client command invocation. This replaces the naive sleep with wakeup on event logic, which is reused to signal the monitoring thread to stop.Instructions for the reviewer:
count/export/etc. on an empty database and see for yourself that the command returns in less than 750ms.