-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Add lazy initialization primitives to std #68198
Conversation
r? @cramertj (rust_highfive has picked a reviewer for you, use r? to override) |
Ah this is actually not totally true. I replaced the |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Also the RFC seems to have Lazy and Once in the sync module, which seems to make a lot more sense to me. Having not read the RFC before seeing this PR, I've been really confused by what the difference between the core and std versions of the Lazy and Once types are. |
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.
😍
There's a bunch of tests here, and most of them could be ported to std
.
I am personally not too thrilled about the naming scheme proposed in the PR. At a glance, I can't tell whats the difference between lazy::OnceCell
and cell::OnceCell
, as nothing says that the first one is sync
.
I've also thought about this, seemingly minimally disagreeable, scheme:
std::{
lazy::{
OnceCell, Lazy,
SyncOnceCell, SyncLazy,
}
}
Also, a specific unresolved question which maybe makes sense to attack first is poisoning. Once we figure out poisoning, we will be able to share the implementaion betwen Once
and OnceCell
(in particular, we can share the impl even if Once is release/acquire, and OnceCell is release/consume).
Not putting the sync variants in the
Ah, the scheme used at the moment in this PR is Personally, I prefer the
Great! I'll pull them in 😄 |
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.
Thank you for doing the work, and for notifying me 😄 !
2 small comments.
state_and_queue: AtomicUsize, | ||
_marker: PhantomData<*mut Waiter>, | ||
// Whether or not the value is initialized is tracked by `state_and_queue`. | ||
value: UnsafeCell<MaybeUninit<T>>, |
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.
How do you implement Drop
in combination with MaybeUninit
? Can you compare it with matklad/once_cell#72?
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.
Ah great point! When you made this comment I wasn't handling Drop
at all. I've since pulled in your implementation from once_cell
👍
That does not seem well motivated. Instead you are just growing the base module of std more with that, which already is getting quite large, while the sync module is much smaller. If the unsync variant is in the cell module, the sync variant should be in the sync module. I'd accept an alternative where all are in the lazy module, but then the distinction between the sync and unsync variants needs to be much clearer and can't compare with other Cells as much as it currently does (cause why is it just another Cell type but not in the cell module?) |
I think it’s less the size of the To keep things moving I’ll refactor back to the original layout: crate core {
pub mod lazy {
pub struct OnceCell;
pub struct Lazy;
}
}
crate std {
pub mod lazy {
pub use core::lazy::*;
pub struct SyncOnceCell;
pub struct SyncLazy;
}
} and we can focus on other implementation questions. I think there are still open questions surfaced by this API we should answer eventually, like what a cell actually is, and whether Does that seem reasonable? |
See also: rust-lang/rfcs#2788 (comment). I personally do think that |
Something which occurred to me just now: we can implement impl<T, F: FnOnce() -> T> DerefMut for Lazy<T, F> as well as impl OnceCell<T> {
pub fn get_or_init_mut<F>(&mut self, f: F) -> &mut T
} ! I think this would be sound, and I have a use case for EDIT: yup, |
Agreed on I am not sure what your thoughts are with |
@KodrAus yeah, I think it might be in general useful to open a tracking issue with checkboxes for all of the unresolved questions |
r? @sfackler |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@matklad Ah I think our lines might’ve crossed, but maybe to try keep the number of open channels lower we could try work through some of those implementation questions here and then open a tracking issue with a link to the RFC and any remaining open questions or non-blocking ones that might come up from a FCP before merging? |
Ah, I see that the PR description has open questions section, missed it at first. I've added a couple |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@@ -18,15 +18,16 @@ | |||
//! [`RwLock`](../../std/sync/struct.RwLock.html) or | |||
//! [`atomic`](../../core/sync/atomic/index.html) types. | |||
//! | |||
//! Values of the `Cell<T>` and `RefCell<T>` types may be mutated through shared references (i.e. | |||
//! Values of the cell types may be mutated through shared references (i.e. |
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.
Should just revert these now
Ok, I've still got the tests to pull in, but also noticed we have a little |
That one sets a destructor that is run during |
Thought about
I was wondering if |
Ok(unsafe { self.get_unchecked() }) | ||
} | ||
|
||
/// Consumes the `Once`, returning the wrapped value. Returns |
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.
The documentation and some comments do not match the type names. Something to check when the bikeshed is concluded...
☔ The latest upstream changes (presumably #68066) made this pull request unmergeable. Please resolve the merge conflicts. |
@pitdicker pointed out in the RFC that we could reduce the size of the |
Co-Authored-By: Paul Dicker <pitdicker@users.noreply.github.com>
☔ The latest upstream changes (presumably #68803) made this pull request unmergeable. Please resolve the merge conflicts. |
@KodrAus any updates? |
closing this due to inactivity |
…crum Add lazy initialization primitives to std Follow-up to rust-lang#68198 Current RFC: rust-lang/rfcs#2788 Rebased and fixed up a few of the dangling comments. Some notes carried over from the previous PR: - [ ] Naming. I'm ok to just roll with the `Sync` prefix like `SyncLazy` for now, but [have a personal preference for `Atomic`](rust-lang/rfcs#2788 (comment)) like `AtomicLazy`. - [x] [Poisoning](rust-lang/rfcs#2788 (comment)). It seems like there's [some regret around poisoning in other `std::sync` types that we might want to just avoid upfront for `std::lazy`, especially if that would align with a future `std::mutex` that doesn't poison](https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/parking_lot.3A.3AMutex.20in.20std/near/190331199). Personally, if we're adding these types to `std::lazy` instead of `std::sync`, I'd be on-board with not worrying about poisoning in `std::lazy`, and potentially deprecating `std::sync::Once` and `lazy_static` in favour of `std::lazy` down the track if it's possible, rather than attempting to replicate their behavior. cc @Amanieu @sfackler. - [ ] [Consider making`SyncOnceCell::get` blocking](matklad/once_cell#92). There doesn't seem to be consensus in the linked PR on whether or not that's strictly better than the non-blocking variant. In general, none of these seem to be really blocking an initial unstable merge, so we could possibly kick off a FCP if y'all are happy? cc @matklad @pitdicker have I missed anything, or were there any other considerations that have come up since we last looked at this?
…crum Add lazy initialization primitives to std Follow-up to rust-lang#68198 Current RFC: rust-lang/rfcs#2788 Rebased and fixed up a few of the dangling comments. Some notes carried over from the previous PR: - [ ] Naming. I'm ok to just roll with the `Sync` prefix like `SyncLazy` for now, but [have a personal preference for `Atomic`](rust-lang/rfcs#2788 (comment)) like `AtomicLazy`. - [x] [Poisoning](rust-lang/rfcs#2788 (comment)). It seems like there's [some regret around poisoning in other `std::sync` types that we might want to just avoid upfront for `std::lazy`, especially if that would align with a future `std::mutex` that doesn't poison](https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/parking_lot.3A.3AMutex.20in.20std/near/190331199). Personally, if we're adding these types to `std::lazy` instead of `std::sync`, I'd be on-board with not worrying about poisoning in `std::lazy`, and potentially deprecating `std::sync::Once` and `lazy_static` in favour of `std::lazy` down the track if it's possible, rather than attempting to replicate their behavior. cc @Amanieu @sfackler. - [ ] [Consider making`SyncOnceCell::get` blocking](matklad/once_cell#92). There doesn't seem to be consensus in the linked PR on whether or not that's strictly better than the non-blocking variant. In general, none of these seem to be really blocking an initial unstable merge, so we could possibly kick off a FCP if y'all are happy? cc @matklad @pitdicker have I missed anything, or were there any other considerations that have come up since we last looked at this?
…crum Add lazy initialization primitives to std Follow-up to rust-lang#68198 Current RFC: rust-lang/rfcs#2788 Rebased and fixed up a few of the dangling comments. Some notes carried over from the previous PR: - [ ] Naming. I'm ok to just roll with the `Sync` prefix like `SyncLazy` for now, but [have a personal preference for `Atomic`](rust-lang/rfcs#2788 (comment)) like `AtomicLazy`. - [x] [Poisoning](rust-lang/rfcs#2788 (comment)). It seems like there's [some regret around poisoning in other `std::sync` types that we might want to just avoid upfront for `std::lazy`, especially if that would align with a future `std::mutex` that doesn't poison](https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/parking_lot.3A.3AMutex.20in.20std/near/190331199). Personally, if we're adding these types to `std::lazy` instead of `std::sync`, I'd be on-board with not worrying about poisoning in `std::lazy`, and potentially deprecating `std::sync::Once` and `lazy_static` in favour of `std::lazy` down the track if it's possible, rather than attempting to replicate their behavior. cc @Amanieu @sfackler. - [ ] [Consider making`SyncOnceCell::get` blocking](matklad/once_cell#92). There doesn't seem to be consensus in the linked PR on whether or not that's strictly better than the non-blocking variant. In general, none of these seem to be really blocking an initial unstable merge, so we could possibly kick off a FCP if y'all are happy? cc @matklad @pitdicker have I missed anything, or were there any other considerations that have come up since we last looked at this?
cc @matklad @pitdicker
Current RFC: rust-lang/rfcs#2788
This PR picks up @matklad's initial
std
implementation, based on theironce_cell
crate, and shifts a few things around, adds unstability attributes, and some docs.It adds the following new unstable APIs:
The implementation is based on
once_cell
.Open questions
Merge blockers
SyncOnceCell
.sync
flavor (acquire or consume?).Others
OnceCell
implCoerceUnsized
?get_or_init
orget_or_set_with
?).std::io::lazy::Lazy
.SyncLazy
.Any of these questions that aren't resolved in this PR will get copied into a tracking issue