feat: Add Slab.get_mut #106
Conversation
gretchenfrage
force-pushed
the
get-mut
branch
3 times, most recently
from
4c9cacb to
045c8f5
Compare
There was a problem hiding this comment.
Thanks for the change!
I definitely agree that a get_mut() API taking a mutable borrow makes sense and is something we ought to have.
I'll be happy to merge this once the CI build passes. Right now, it looks like there are a few issues:
-
It seems
rustfmtneeds to be run -
There's a compilation error on the current (admittedly, ancient) MSRV of Rust 1.42.0 due to some of the new code requiring non-lexical lifetimes. I commented on this inline.
-
It looks like the new code doesn't compile with
cfg(loom), due to Loom's use of aMutPtr<T>type for tracking mutable accesses toUnsafeCell<T>s rather than returning*mut T, and becauseloom'sAtomicPtrdoesn't have aget_mut(). I commented on some of them inline, and it should be possible to fix.With that said, I don't really care about including the
get_mutAPI in loom tests, since it doesn't actually exercise any concurrency. There's no way there could be the kind of race condition thatloomis used to detect inget_mut(), becauseget_mut()is inherently single-threaded. So, if it's too much work to get this API to compile underloom, I'm fine with adding#[cfg(not(loom)]to the various functions that are only used to implementget_mut. As long as we can make the loom tests compile again, I'm fine either way.
src/lib.rs
Outdated
| @@ -623,6 +623,33 @@ impl<T, C: cfg::Config> Slab<T, C> { | |||
| }) | |||
| } | |||
|
|
|||
| /// Return a mutable reference to the value associated with the given key. | |||
| /// | |||
| /// This call borrows self mutably (at compile-time) which guarantees that we | |||
There was a problem hiding this comment.
Teensy nit: I might format self in monospace here:
| /// This call borrows self mutably (at compile-time) which guarantees that we | |
| /// This call borrows `self` mutably (at compile-time) which guarantees that we |
src/shard.rs
Outdated
| @@ -398,6 +409,11 @@ impl<T, C: cfg::Config> Ptr<T, C> { | |||
| Some(track.get_ref()) | |||
| } | |||
|
|
|||
| #[inline] | |||
| fn get_mut(&mut self) -> Option<&mut Shard<T, C>> { | |||
| NonNull::new(*self.0.get_mut()).map(|mut track| unsafe { track.as_mut() }.get_mut()) | |||
There was a problem hiding this comment.
This doesn't seem to compile on the current MSRV (Rust 1.42.0): https://github.com/hawkw/sharded-slab/actions/runs/12508005473/job/34895461998?pr=106
I believe that's because this code requires non-lexical lifetimes (NLL), which were added to the compiler in Rust 1.62.0.
I think it might be possible to rework this to return a *mut Shard from the map so that we don't run into a lifetime issue here. But, if that's not practical, I'll freely admit that Rust 1.42.0 is now ancient, and it would be fine to bump the MSRV to Rust 1.62.0 for NLL, which should hopefully fix this.
There was a problem hiding this comment.
This code also does not compile under cfg(loom), as loom's simulated version of AtomicPtr doesn't have a get_mut. I think it would be fine for a loom version of this code to use the unsafe AtomicPtr::unsync_load instead, with a comment explaining that this is okay because the &mut self ensures we don't actually do a racy unsynchronized access.
gretchenfrage
force-pushed
the
get-mut
branch
4 times, most recently
from
6938484 to
c2d11db
Compare
There was a problem hiding this comment.
Looks good to me, although I noticed one last typo. Thanks for adding this!
As an aside, you don't need to force push; I typically squash merge branches unless the author wants to go out of their way to rebase-merge multiple commits. Whatever workflow works for you is fine, though.
src/shard.rs
Outdated
| #[inline] | ||
| #[cfg(not(loom))] | ||
| fn get_mut(&mut self) -> Option<&mut Shard<T, C>> { | ||
| // MSRV: We could do `track.is_mut()`, but the current MSRV doesn't have NLL |
There was a problem hiding this comment.
typo: i think you meant "track.as_mut()" here?
This commit adds a Slab::get_mut method, which takes a `&mut self`, and thus is able to get a mutable reference to an element without actually using any synchronization structures. This is a similar concept to `UnsafeCell::get_mut`, and may be useful for cases such as: - A sharded `Slab` wrapped in a `RwLock`, such that hot path operations are done through read guards, and rarer and complex operations are done through write guards. - Any other situation where code has both parallel and serial section, such as in fork-join systems. - Gradually converting single-threaded systems to parallelizable systems. The doc comment adds a test that helps ensure basic functionality works. `test_println!` comments are not added since, as this doesn't actually manipulate any internal structures except the user-provided types, they don't seem that useful in this case.
gretchenfrage
force-pushed
the
get-mut
branch
from
c2d11db to
2ccd5fa
Compare
|
:) |
|
All conversations should be resolved, CI passes, and there are no merge conflicts. I think this should be able to be merged |
This commit adds a Slab::get_mut method, which takes a
&mut self, and thus is able to get a mutable reference to an element without actually using any synchronization structures. This is a similar concept toUnsafeCell::get_mut, and may be useful for cases such as:Slabwrapped in aRwLock, such that hot path operations are done through read guards, and rarer and complex operations are done through write guards.The doc comment adds a test that helps ensure basic functionality works.
test_println!comments are not added since, as this doesn't actually manipulate any internal structures except the user-provided types, they don't seem that useful in this case.Closes #97