Add an efficient implementation based on critical-section #148
Conversation
The current no-std implementation is somewhat ineffecient, potentially broken and relies on some slow data structures. The main reason why we use this implementation is because we don't have a way to "lock" the linked list, since there's no way to lock things in no-std. However, many embedded platforms have a concept of a "critical section" that can be used to exclusively lock something. This PR adds an option to the "std" implementation that replaces the existing Mutex with a usage of the critical-section crate. This is enabled with the "critical-section" feature. This allows us to have the advantages of the std-based implementation without needing to rely on std for platforms that don't have it. Signed-off-by: John Nunley <dev@notgull.net>
Since `std.rs` and `no_std.rs` no longer correspond to std and no_std, I've renamed them based on the underlying algorithm used. `std.rs` is now `intrusive.rs`, since it uses an intrusive linked list. `no_std.rs` is now `slab.rs`, since it relies on a structure similar to the one used in the `slab` crate. Signed-off-by: John Nunley <dev@notgull.net>
src/intrusive.rs
Outdated
| @@ -274,11 +337,13 @@ impl<T> Inner<T> { | |||
| } | |||
| } | |||
|
|
|||
| #[cfg(all(feature = "std", not(feature = "critical-section")))] | |||
There was a problem hiding this comment.
imo it would be a good idea to put ListLock into a separate file and mark the whole file with #[cfg(...)], this would make this easier to read.
There was a problem hiding this comment.
I've inlined the entire construct into with_inner.
| @@ -1359,7 +1365,7 @@ mod sync { | |||
| #[cfg(feature = "portable-atomic")] | |||
| pub(super) use portable_atomic_util::Arc; | |||
|
|
|||
| #[cfg(all(feature = "std", not(loom)))] | |||
| #[cfg(all(feature = "std", not(feature = "critical-section"), not(loom)))] | |||
There was a problem hiding this comment.
why wasn't the critical_section::Mutex import put here, too?
There was a problem hiding this comment.
Because critical-section doesn't rely on loom intrinsically.
There was a problem hiding this comment.
hm std also doesn't rely on loom intrinsically; or how is that meant?/ what does that mean?
I just thought it should belong here because that part does accumulate definitions which are basically "alternatives" between different features used throughout the code, and the underlying Mutex impl imo is one of those "switchable" things (unfortunately not with a 1-to-1 common/shared sufficient API)
There was a problem hiding this comment.
With std you need to replace std's primitives with loom, with critical-section you can just use a CS based on Loom.
notgull
force-pushed
the
notgull/cs
branch
2 times, most recently
from
a6bd06d
to
a90f8ff
Compare
- Untangle imports. - Inline the entirely of ListLock into with_inner(). - Add documentation for feature. Signed-off-by: John Nunley <dev@notgull.net>
| /// libstd-based implementation uses a normal Muetx to secure the data. | ||
| #[cfg(all(feature = "std", not(feature = "critical-section")))] | ||
| crate::sync::Mutex<Inner<T>>, | ||
| /// CS-based implementation uses a CS cell that wraps a RefCell. |
There was a problem hiding this comment.
I think this should spell out critical section instead of CS at least once, otherwise it might lead to confusion.
| }; | ||
|
|
||
| #[cfg(feature = "critical-section")] | ||
| { |
The current no-std implementation is somewhat ineffecient, potentially
broken and relies on some slow data structures. The main reason why we
use this implementation is because we don't have a way to "lock" the
linked list, since there's no way to lock things in no-std. However,
many embedded platforms have a concept of a "critical section" that can
be used to exclusively lock something.
This PR adds an option to the "std" implementation that replaces the
existing Mutex with a usage of the critical-section crate. This is
enabled with the "critical-section" feature. This allows us to have the
advantages of the std-based implementation without needing to rely on
std for platforms that don't have it.