-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Cleanups for condition_variable
's waiting functions
#3974
Cleanups for condition_variable
's waiting functions
#3974
Conversation
stl/inc/mutex
Outdated
if (_Abs_time <= _Now) { | ||
return false; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About this change:
- This is specified by the standard and is in the most maintainable form. The correctness is now obviously only dependent on
wait_until(lock,abs_time,pred)
. - Actually this change fixes a bug -
if (_Abs_time <= _Now)
should return_Pred()
too (this bug is not significant due to timing). - The efficiency loss is not significant IMO, and we will gain some efficiency back after fixing the clock-awareness bug.
condition_variable_any
already does like this.- (update) The following benchmark (tested on the unchanged code) shows the main "inefficiency" comes from the inaccuracy of
_Cnd_timedwait(including ceiling-cast to milliseconds) / SleepConditionVariableSRW
. I'm getting about1500 us
waiting for my call. Compared to this, aswait_until(lock,abs_time)
is practically equivalent to a single_Cnd_timedwait
call, I think any "efficiency loss" by this change should be negligible.
#include<benchmark/benchmark.h>
#include<mutex>
using namespace std;
void BM_test(benchmark::State& state) {
condition_variable cv;
mutex m;
unique_lock lock{ m };
for (auto _ : state) {
cv.wait_for(lock, 500us);
}
}
BENCHMARK(BM_test);
BENCHMARK_MAIN();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(2) is not a bug. return false
is returning the result of the most-recent evaluation of _Pred()
. (I do agree with the rest of this comment ;))
27f318a
to
aeb909b
Compare
aeb909b
to
67111c0
Compare
Co-authored-by: Casey Carter <cartec69@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good once I apply the typo fixes.
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
🧹 ✨ 🎉 |
Simplifies
, and fixes some minor problems incondition_variable
's waiting functions. The correctness can be confirmed by looking through commits in order.Works towards #718; after this change it will be much easier to fix
condition_variable
's clock-awareness problem.condition_variable_any
are not effected by this pr.