Skip to content
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

Wait condition_variable_any for steady_clock #4755

Merged
merged 5 commits into from
Aug 8, 2024

Conversation

AlexGuteniev
Copy link
Contributor

@AlexGuteniev AlexGuteniev commented Jun 30, 2024

Fixes #4723

Turned out that:

  • WinAPI SleepConditionVariableSRW times out using a steady clock that is not precisely synchronized with steady_clock implemented based on QueryPerformanceCounter
  • Fixing it with GetTickCount64() check doesn't help, as GetTickCount64() is also not precisely synchronized with QueryPerformanceCounter

The fix is to check exactly steady_clock in condition_variable_any.

This is done in a header, rather than in .cpp for these reasons:

  • The header implementation of steady_clock::now() is nontrivial, and resides in a non-core header, and we don't have a good core header to extract it into
  • Other call sites of the internal condition variable wait function check against the clock on their own already

That's why GetTickCount64() was removed from internal wait function.

To avoid destabilizing behavior for already compiled code, I kept GetTickCount64() on that code path (it might improve over raw SleepConditionVariableSRW a bit).

I've inlined _Wait_for_ms_count as to me it appeared clearer this way after these changes.

@AlexGuteniev AlexGuteniev requested a review from a team as a code owner June 30, 2024 12:42
@StephanTLavavej StephanTLavavej added the bug Something isn't working label Jun 30, 2024
@StephanTLavavej StephanTLavavej self-assigned this Jun 30, 2024
@StephanTLavavej StephanTLavavej changed the title Wait condition_variable for steady_clock Wait condition_variable_any for steady_clock Aug 2, 2024
stl/inc/condition_variable Outdated Show resolved Hide resolved
stl/src/sharedmutex.cpp Outdated Show resolved Hide resolved
stl/inc/condition_variable Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Aug 2, 2024

Thanks! I pushed a conflict-free merge with main and a simple transformation. This fix makes sense and I verified that the affected libcxx tests pass 10 times in a row. 😻

I also fixed the title, because the correctness fix is specific to condition_variable_any; the impact on condition_variable is a minor performance improvement due to not having to call GetTickCount64() anymore.

@StephanTLavavej StephanTLavavej removed their assignment Aug 2, 2024
@StephanTLavavej StephanTLavavej self-assigned this Aug 6, 2024
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 2f5b967 into microsoft:main Aug 8, 2024
39 checks passed
@StephanTLavavej
Copy link
Member

Thanks for fixing this long-standing sporadic bug! ⏳ 🐞 😻

@AlexGuteniev AlexGuteniev deleted the clocks branch August 8, 2024 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
2 participants