Added spin loop pause function #40537
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
src/libstd/sync/mod.rs
Outdated
| #[stable(feature = "rust1", since = "1.1.6")] | ||
| pub fn spin_loop_pause() | ||
| { | ||
| #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] |
There was a problem hiding this comment.
We should probably support more than just x86 if this were to merge - is there some LLVM intrinsic we could leverage here?
There was a problem hiding this comment.
_mm_pause is part of SSE2, so we should probably use that. But that is exactly as portable as what's provided here I think. (If _mm_pause is the answer, then we should probably punt on this until SIMD.)
|
cc @rust-lang/libs thoughts? |
sfackler
added
the
T-libs-api
src/libstd/sync/mod.rs
Outdated
| /// system's scheduler. In some cases it might be useful to use a | ||
| /// combination of both functions. | ||
| #[inline] | ||
| #[stable(feature = "rust1", since = "1.1.6")] |
There was a problem hiding this comment.
I'm fine if this isn't stable.
There was a problem hiding this comment.
@sfackler Actually can you help me out with the stability attribute syntax? I can't find any reference of it online.
There was a problem hiding this comment.
Here's an example: https://github.com/rust-lang/rust/blob/master/src/libcore/convert.rs#L291
Pick some arbitrary (unique) feature name, and before we merge this we'll create a tracking issue to refer to.
|
If this was explicitly left out of a c11 memory model or something like that then I'd personally not be in favor of such a function. @sstewartgallus do you have more motivation for adding this beyond "I feel would be really convenient in Rust" to add this? |
|
The motivating case is that every lock-free data-structure in crossbeam should use this function. IMO, almost every time you use atomics you should use this function. Secondly, this function causes no incompatibility problems with platforms it doesn't work for and is just a nop on those. |
|
See http://openjdk.java.net/jeps/285 for previous work. |
|
This doesn't really have to be in the stdlib, it could just be a library on crates.io. However we do need stable inline asm for this to be widely used. |
|
I think it probably makes sense to land this unstable for now at least. @rfcbot fcp merge |
|
Team member @sfackler has proposed to merge this. The next step is review by the rest of the tagged teams: No concerns currently listed. Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
|
The documentation here seems particularly lacking to me I think? There's no dicussion of cross-platform concerns, portability, etc. I'd personally prefer to see at least a warning that this function isn't guaranteed to do anything, it's just a small optimization on some architectures in some situations. I also feel like this'd best be inside of libcore, not inside of libstd. Finally, I'm not personally sold on the Looks like tidy is also failing? |
Spin loop pause function redux GitHub's interface is screwy. This is the same PR as #40537
This is the one function I wish was added to the C11 atomics support and I feel would be really convenient in Rust.