-
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
Implementation of std::atomic::wait #593
Conversation
What happened to README.md? |
Sorry, I'm new to Github, it was deleted by mistake. Reverted. |
Isn't the lack of a license (AFAICT) on https://github.com/ogiroux/atomic_wait problematic? |
I think that's fine. Every file there is under MIT license, see top of https://github.com/ogiroux/atomic_wait/blob/master/include/atomic_wait for example |
Forgot to check source files 🤦♂️. And MIT isn't a compatible license #113 (review) |
I see... Wouldn't it help that:
|
Not a lawyer or maintainer but for the second bullet point, no. STL pointed out in this comment #29 (comment) for another C++20 feature that the reference implementation couldn't be used due to its MIT license. |
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.
- I asked Olivier to comment on the licensing issue over Twitter
- This needs tests
- The similar wait-on-address fallback in
parallel_algorithms.cpp
should probably be changed to call this, it's OK to file that as an issue if you don't want to do it here (after this merges)
Thanks for your contribution! |
https://github.com/ogiroux/atomic_wait is an earlier version of the code; the newer version is https://github.com/ogiroux/freestanding, and the version incorporated in the CUDA toolkit as of CUDA 10.2. I'm the team lead at NVIDIA for that codebase, and can affirm that it is licensed under the Apache 2.0 with LLVM exception license. |
@BillyONeal , regarding tests. I see there are several folders with test cases. Apparently this one should go into a new folder named "p1135r6_atomic_wait" under /tests/std/tests. I have not figured out how to build and run those tests. |
Since @AlexGutenev has confirmed that this PR's code was written from scratch (thank you for carefully disclosing the origins!), and the "inspiration" code's license is identical to ours, I don't believe that we'll need to update our
Thanks, that's extremely helpful. @brycelelbach, can we ask you or @ogiroux to check in a (Aside: I observe that that repo currently bears license banners of a different nature: It seems like these banners should be updated if the license is Apache 2.0 + LLVM.) |
I like more the approach in @BillyONeal , could |
Didn't get it from the beginning. You mean sharing fallback, not just address obtaining.
My intention was to target Windows XP, that's why I did not consider using SRW Lock. But if there isn't such intention for |
Correct me if I am wrong but the Windows 7 the successor of the successor of XP reached end of Life earlier this year. So why would anybody ever again target XP with a modern library? |
@miscco ,
Yes, you are right. I thought VS 2019 still comes with XP support, but it is because it can install VS 2017 XP toolset, which obviously doesn't come with VS 2019 STL @BillyONeal , any motivation in preferring my implementation based on Windows Semaphore over the one in I've created a branch with an implementation that does not care about XP https://github.com/AlexGutenev/STL/tree/vista |
That touches preprocessor metaprogramming I don't want to touch but I have no objection to you touching it :). Though in that case you probably want that in a different PR. |
XP support is unnecessary for completely new features; I think for them Win7 is fine. We only need to keep existing programs that don't touch new features working on XP since they share the same redist, as you mentioned. Does that mean you want to switch from the Semaphore fallback to something else? |
stl/src/winapisupp.cpp
Outdated
@@ -548,6 +564,7 @@ extern "C" PVOID __KERNEL32Functions[eMaxKernel32Function] = {0}; | |||
|
|||
static int __cdecl initialize_pointers() { | |||
HINSTANCE hKernel32 = GetModuleHandleW(L"kernel32.dll"); | |||
HINSTANCE hSynch = GetModuleHandleW(L"api-ms-win-core-synch-l1-2-0.dll"); |
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.
GetModuleHandleEx with GET_MODULE_HANDLE_EX_FLAG_PIN
That doesn't fix the problem that the module might not be loaded in the first place. We also can't _PIN
as that prevents the standard library from being cleanly unloaded.
Like I said I would refrain from touching this until we have someone who actually understands apisets here to comment.
Well, yes. To something that is very similar to what is already there for (https://github.com/ogiroux/atomic_wait is a bit more complex, as it tries not to wake CV unnecessarily, but Windows CV takes care about that on its own) |
Changed to use SRW Lock |
I've created #598 for that. |
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.
This does the loop on an absolute timeout so on spurious wake the timeout explodes. I'm working on fixing that now.
Fun ICE I ran into as an FYI https://developercommunity.visualstudio.com/content/problem/1133284/c2-internal-compiler-error.html |
Correct. Spurious wakes were handled by arriving to header, and now they are not. I guess just making indirect function take absolute timeout is fine |
…n spurious wake by having the header compare again. * Remove double indirect call trampoline for the non-lock free case.
… of needed spurious wakes.
Instead of passing absolute timeouts over the boundary which has caused us ABI problems before I added a special case for no timeout where the separately compiled bits can eat some of the spurious wakes. |
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.
One suggestion.
stl/inc/atomic
Outdated
} | ||
|
||
struct _Spinlock_guard { | ||
long& _Spinlock; | ||
_Spinlock_guard(long& _Spinlock_) noexcept : _Spinlock(_Spinlock_) { |
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.
I believe that explicit
is most important for construction from common types like integers and pointers:
_Spinlock_guard(long& _Spinlock_) noexcept : _Spinlock(_Spinlock_) { | |
explicit _Spinlock_guard(long& _Spinlock_) noexcept : _Spinlock(_Spinlock_) { |
This has passed DevDiv tests too. I'll apply STL and Casey's nitpicks and merge :) @AlexGuteniev Thanks for putting up with us! |
Yay! I can't wait for this to ship in VS 2019 16.8 Preview 3! 😹 |
Will we ship it all at once, or piecemeal so some people might observe the feature partially shipping? |
@CaseyCarter Gotta watch out for those spurious wakes. |
Description
Implementation of std::atomic::wait , part of #52
Partly based on https://github.com/ogiroux/atomic_wait (no code copying, but being familiar with that implementation)
(initially; currently it substantially diverges from that idea)
Design decisions:
WaitOnAddress
and fallback. Still when STL targets Windows 8+ (ARM), the decision to useWaitOnAddress
is made at compile timeWaitOnAddress
, sinceWaitOnAddress
spins by itself (andSleepConditionVariableSRW
is expecteed to spin too, though did not check that).Known issues:
atomic_shared_ptr
. Another change maybe.atomic_ref
is merged, as it is available only foratomic_ref
Checklist
Be sure you've read README.md and understand the scope of this repo.
If you're unsure about a box, leave it unchecked. A maintainer will help you.
_Ugly
as perhttps://eel.is/c++draft/lex.name#3.1 or there are no product code changes.
verified by an STL maintainer before automated testing is enabled on GitHub,
leave this unchecked for initial submission).
members, adding virtual functions, changing whether a type is an aggregate
or trivially copyable, etc.).
the C++ Working Draft (including any cited standards), other WG21 papers
(excluding reference implementations outside of proposed standard wording),
and LWG issues as reference material. If they were derived from a project
that's already listed in NOTICE.txt, that's fine, but please mention it.
If they were derived from any other project (including Boost and libc++,
which are not yet listed in NOTICE.txt), you must mention it here,
so we can determine whether the license is compatible and what else needs
to be done.