rt: implement task::Id using StaticAtomicU64
#5282
Conversation
This patch simplifies the implementation of `task::Id` by moving conditional compilation into the `AtomicU64` definition. To handle platforms that do not include `const fn Mutex::new()`, `StaticAtomicU64` is defined which always has a `const fn new()`. `StaticAtomicU64` is implemented with `OnceCell` when needed.
| @@ -1,18 +1,24 @@ | |||
| use crate::loom::sync::Mutex; | |||
| use std::sync::atomic::Ordering; | |||
|
|
|||
| cfg_has_const_mutex_new! { | |||
| #[path = "atomic_u64_static_const_new.rs"] | |||
There was a problem hiding this comment.
I'm experimenting with a way to have less code within the cfg macros. This should help rustfmt apply to more code.
| use crate::util::once_cell::OnceCell; | ||
|
|
||
| pub(crate) struct StaticAtomicU64 { | ||
| init: u64, |
There was a problem hiding this comment.
It is a bit annoying to store the initial value forever, but this is only for platforms that do not have AtomicU64 and no const fn Mutex::new() (i.e. old versions of Rust), so it isn't a big deal IMO.
|
Seems like I have to figure out how to make it work with loom... |
| // TODO: implement a loom version | ||
| pub(crate) type StaticAtomicU64 = std::sync::atomic::AtomicU64; |
There was a problem hiding this comment.
This is a bit (ok, a lot) of a hack, but it is equivalent to before the patch. Unfortunately, right now, loom does not have a way to model static variables.
| @@ -25,7 +25,7 @@ impl<T> OnceCell<T> { | |||
| /// If the `init` closure panics, then the `OnceCell` is poisoned and all | |||
| /// future calls to `get` will panic. | |||
| #[inline] | |||
| pub(crate) fn get(&self, init: fn() -> T) -> &T { | |||
| pub(crate) fn get(&self, init: impl FnOnce() -> T) -> &T { | |||
There was a problem hiding this comment.
typo on line 22: `intiailizing' -> initializing
There was a problem hiding this comment.
This may be a bad idea, but wouldn't it be reasonable to use something like atomic-polyfill or portable-atomic to polyfill out atomics on systems that don't have them? I know this probably doesn't apply to this PR, but I think that a sequence-lock system may be a more ergonomic way of accessing this value.
|
@notgull
That said, given the differences between global locking and per-object locking and tokio's policy on dependencies, it is doubtful that it is worth adding a non-optional dependency to tokio for this purpose. Footnotes
|
|
@taiki-e Thanks for explaining this to me. Would you accept a PR where the current Mutex implementation is replaced with a hand-rolled seq-lock implementation? |
|
There are not many targets where 64-bit atomic is essentially unavailable (although this fallback is used by many targets because the current detection logic is a bit rough)1, and I'm not sure if the complexity is worth it.2 Proper seqlock implementation on a 32-bit system is somewhat complex and sound seqlock implementation requires logic like per-byte atomic memcpy that loom is unclear whether it understands. What I would like to do here is to improve the detection logic. Footnotes |
This patch simplifies the implementation of
task::Idby moving conditional compilation into theAtomicU64definition. To handle platforms that do not includeconst fn Mutex::new(),StaticAtomicU64is defined, which always has aconst fn new().StaticAtomicU64is implemented withOnceCellwhen needed.Future patches will update other code locations to use
StaticAtomicU64(e.g.ThreadId).