Simplify SyncOnceCell's take and drop.
#76640
Conversation
This comment has been minimized.
This comment has been minimized.
jyn514
added
T-libs
Without this, it was not inlined in SyncOnceCell::into_inner(), causing unecessary checks and dead code.
This comment has been minimized.
This comment has been minimized.
|
The struct type has a comment but there's no Also Cc @Mark-Simulacrum who was involved in previous reviews here. |
| // SAFETY: `self.value` is initialized and contains a valid `T`. | ||
| // `self.once` is reset, so `is_initialized()` will be false again | ||
| // which prevents the value from being read twice. | ||
| unsafe { Some((&mut *self.value.get()).assume_init_read()) } |
There was a problem hiding this comment.
So we leave some garbage bytes inside, but they are guarded by reseted Once? Nifty!
There was a problem hiding this comment.
FWIW, previously this wrote MaybeUninit::uninit() which is equivalent to not writing anything, so we already left behind garbage before. (Just the compiler might find it easier this way as there are no uninit writes to eliminate.)
There was a problem hiding this comment.
The disassembly of this new version is slightly shorter. It seems to make one less copy. (Specifically for SyncOnceCell<String> at least.) But that's probably just the copy out of self.value into value.
There was a problem hiding this comment.
It's also possible that the compiler just did not realize before that it was needlessly copying uninitialized memory.
| /// | ||
| /// Safety: The cell must now be free'd WITHOUT dropping. No other usages of the cell | ||
| /// are valid. Only used by `into_inner` and `drop`. | ||
| unsafe fn take_inner(&mut self) -> Option<T> { |
There was a problem hiding this comment.
👍 I was thinking about removing take_inner as well (its contract is just weird), but I didn't know about those useful MaybeUninit helpers!
There was a problem hiding this comment.
That's because they didn't exist yet. :) I added assume_init_drop in #76484 just a few days ago.
|
📌 Commit aa68aaa has been approved by |
Yeah, that comment is outdated, it's basically a transposon -- it was originally copy-pasted from std's Once impl into once_cell crate, and now it is copy-pasted back This comment also seems suspicious: Lines 379 to 381 in aa68aaa It would be good to clean those up, but probably in a separate PR. |
bors
added
S-waiting-on-bors
…ll-drop, r=matklad Simplify SyncOnceCell's `take` and `drop`. Prevents copies by using `assume_init_read` and `assume_init_drop`.
…ll-drop, r=matklad Simplify SyncOnceCell's `take` and `drop`. Prevents copies by using `assume_init_read` and `assume_init_drop`.
…as-schievink Rollup of 12 pull requests Successful merges: - rust-lang#75559 (unions: test move behavior of non-Copy fields) - rust-lang#76441 (Note that parallel-compiler = true causes tests to fail) - rust-lang#76527 (Remove internal and unstable MaybeUninit::UNINIT.) - rust-lang#76629 (Simplify iter zip struct doc) - rust-lang#76640 (Simplify SyncOnceCell's `take` and `drop`.) - rust-lang#76646 (Add mailmap entry) - rust-lang#76651 (Remove Windows details from Unix and VmWorks symlink() docstrings) - rust-lang#76663 (Simplify iter chain struct doc) - rust-lang#76665 (slice::from_raw_parts: explicitly mention that data must be initialized) - rust-lang#76667 (Fix CI LLVM to work on NixOS out of the box) - rust-lang#76668 (Add visualization of rustc span in doc) - rust-lang#76677 (note that test_stable_pointers does not reflect a stable guarantee) Failed merges: r? `@ghost`
Prevents copies by using
assume_init_readandassume_init_drop.