Add lazy initialization primitives to std #72414

KodrAus · 2020-05-21T05:55:30Z

Follow-up to #68198

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 like AtomicLazy.
Poisoning. 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. 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 makingSyncOnceCell::get blocking. 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?

rust-highfive · 2020-05-21T05:55:33Z

r? @sfackler

(rust_highfive has picked a reviewer for you, use r? to override)

KodrAus · 2020-05-21T06:19:21Z

My bandwidth is still very patchy, so if I'm not able to get this across the line this time if somebody else would like to pick up and continue they'd be most welcome to! 🙂

bors · 2020-05-29T23:57:02Z

☔ The latest upstream changes (presumably #72747) made this pull request unmergeable. Please resolve the merge conflicts.

KodrAus · 2020-06-07T04:02:28Z

Ah I think the way I pushed the branch when rebasing made it unclear what state this PR is in. It’s mergeable and ready for a review.

Mark-Simulacrum

Mostly some minor comments. I don't think we have to resolve the unresolved questions immediately, so maybe good to just land this?

Mark-Simulacrum · 2020-06-16T13:53:25Z

src/libstd/lazy.rs

+impl<T> From<T> for SyncOnceCell<T> {
+    fn from(value: T) -> Self {
+        let cell = Self::new();
+        cell.get_or_init(|| value);


Hm, comparing this to Clone, I am somewhat surprised this doesn't use set() -- any reason for that discrepancy?

cc @matklad

I can’t see a reason not to use the same match res.set() {} approach here in From.

src/libstd/lazy.rs

Mark-Simulacrum · 2020-06-16T14:14:49Z

src/libstd/lazy.rs

+            thread: Cell::new(Some(thread::current())),
+            signaled: AtomicBool::new(false),
+            next: (current_state & !STATE_MASK) as *const Waiter,
+        };


I think this is fine -- we'll block on the signal coming in before running the destructor -- but currently this is setup such that the Waiter is deconstructed and the Thread is dropped by the Drop for WaiterQueue; it seems like it would be beneficial to either:

drop naturally here, avoiding the Cell entirely

put the Waiter inside something like ManuallyDrop

I believe this is lifted directly from sync::Once. I’ll check it out properly and see if there’s a reason it was originally written this way.

Left a comment down below so it's more discoverable: #72414 (comment)

bors · 2020-06-28T20:53:25Z

☔ The latest upstream changes (presumably #73838) made this pull request unmergeable. Please resolve the merge conflicts.

KodrAus · 2020-06-30T08:31:03Z

I've spiked out removing the inlined Waiter and WaiterQueue here in favour of the one in sync::Once in d57decb. It's meant a few small adjustments to sync::Once that should be benchmarked, but I haven't done that yet because we may not want to use that approach and let the two synchronisation methods diverge naturally.

bors · 2020-07-02T16:38:08Z

☔ The latest upstream changes (presumably #73954) made this pull request unmergeable. Please resolve the merge conflicts.

Keruspe · 2020-07-09T13:56:29Z

src/libcore/lazy.rs

+    /// assert!(cell.get().is_some());
+    /// ```
+    #[unstable(feature = "once_cell", issue = "68198")]
+    pub fn set(&self, value: T) -> Result<(), T> {


Shouldn't this take a &mut self given the comment below?

No, although the first commit is admittedly tautological. We can't get overlapping mutable borrows because we have a &self, which is guarantees their absence.

Mark-Simulacrum · 2020-07-16T13:33:56Z

@KodrAus I would love to get this landed so we can start experimenting on nightly -- can you rebase this?

I feel like we have sufficient desire/consensus from amongst the library team that this is something we want (in some form) and the specific remaining questions can be hashed out separately I imagine, before stabilization. So I'm going to go ahead and r? @Mark-Simulacrum and personally I'm prepared to r=me once this is rebased and we have a tracking issue open etc

This commit refactors the initial implementation to fit into std and makes some other changes: - use MaybeUninit internally in SyncOnceCell - correctly impl Drop for lazy::Once - port Lazy::take from once_cell from: matklad/once_cell#100 Co-Authored-By: Paul Dicker <[email protected]>

KodrAus · 2020-07-16T21:30:11Z

@Mark-Simulacrum No problem! This is all rebased, so I'll open a tracking issue and update the attributes 👍

KodrAus · 2020-07-18T00:14:41Z

I've gone and filed #74465 as a tracking issue for the once_cell feature and linked it up to the unstable attribute. I'll get this one on the go and post some notes in the RFC about trying out the new API 🎉 Exciting times!

As per: #72414 (comment)

@bors r=Mark-Simulacrum

bors · 2020-07-18T00:14:42Z

📌 Commit fe63905 has been approved by Mark-Simulacrum

bors · 2020-07-18T00:14:43Z

🌲 The tree is currently closed for pull requests below priority 5, this pull request will be tested once the tree is reopened

@Amanieu

…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?

@Amanieu

…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?

eddyb · 2020-07-18T10:13:07Z

src/libcore/lazy.rs

+pub struct OnceCell<T> {
+    // Invariant: written to at most once.
+    inner: UnsafeCell<Option<T>>,
+}


Do we rely on implicit !Sync from UnsafeCell, or do we try to be explicit? cc @RalfJung

(Sorry if this was asked already, I just saw this PR and thought it was neat, and wasn't sure if this OnceCell was meant to be thread-safe or not)

Ah the OnceCell here in core isn’t meant to be thread-safe, so we’re just relying on UnsafeCell. I’m actually not sure whether we’ve got a preferred approach for being explicit about clobbering auto-traits... I know it’s bitten us sometimes.

It didn't help that GitHub hides the std::sync version by default, it was clearer once I found it.

I'm personally a big fan of making things explicit.

@ghost

…arth Rollup of 11 pull requests Successful merges: - rust-lang#72414 ( Add lazy initialization primitives to std) - rust-lang#74069 (Compare tagged/niche-filling layout and pick the best one) - rust-lang#74418 (ci: Set `shell: bash` as a default, remove duplicates) - rust-lang#74441 (bootstrap.py: patch RPATH on NixOS to handle the new zlib dependency.) - rust-lang#74444 (Add regression test for rust-lang#69414) - rust-lang#74448 (improper_ctypes_definitions: allow `Box`) - rust-lang#74449 (Test codegen of compare_exchange operations) - rust-lang#74450 (Fix `Safety` docs for `from_raw_parts_mut`) - rust-lang#74453 (Use intra-doc links in `str` and `BTreeSet`) - rust-lang#74457 (rustbuild: drop tool::should_install) - rust-lang#74464 (Use pathdiff crate) Failed merges: r? @ghost

jethrogb · 2020-07-20T10:24:47Z

This broke tests for a tier 2 target, fixed in #74546

…be_uninit_extra, r=kennytm Fix duplicate maybe_uninit_extra attribute Introduced in rust-lang#72414

RalfJung · 2020-08-19T08:05:07Z

src/libstd/lazy.rs

+    // miri doesn't support threads
+    #[cfg(not(miri))]


Was this code copied from somewhere else? Miri supports threads these days, and also the libstd tests are not currently run in Miri (but maybe I should look into that).

filed #75696

`std::lazy::SyncOnceCell` was unstably added by [rust-lang/rust#72414] [1] and is being tracked by [rust-lang/rust#74465][2]. This replaces `once_cell::sync::OnceCell`. [1]: rust-lang/rust#72414 [2]: rust-lang/rust#74465

rust-highfive assigned sfackler May 21, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 21, 2020

KodrAus changed the title ~~Feat/stdlazy~~ Add lazy initialization primitives to std May 21, 2020

KodrAus added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label May 23, 2020

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 6, 2020

KodrAus added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 7, 2020

Mark-Simulacrum reviewed Jun 16, 2020

View reviewed changes

KodrAus added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 18, 2020

KodrAus added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 30, 2020

Keruspe reviewed Jul 9, 2020

View reviewed changes

rust-highfive assigned Mark-Simulacrum and unassigned sfackler Jul 16, 2020

matklad and others added 5 commits July 17, 2020 07:23

First cut of std::lazy module

cac1768

use set() in SyncOnceCell::from

d1263f5

remove inlined lazy::Waiter in favor of sync::Once

d101794

appease tidy

1f1cda6

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 18, 2020

Manishearth mentioned this pull request Jul 18, 2020

Rollup of 12 pull requests #74466

Closed

Manishearth mentioned this pull request Jul 18, 2020

Rollup of 11 pull requests #74467

Closed

Manishearth mentioned this pull request Jul 18, 2020

Rollup of 11 pull requests #74468

Merged

eddyb reviewed Jul 18, 2020

View reviewed changes

bors merged commit 01418bd into rust-lang:master Jul 18, 2020

jethrogb mentioned this pull request Jul 20, 2020

Fix duplicate maybe_uninit_extra attribute #74546

Merged

Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 20, 2020

Rollup merge of rust-lang#74546 - jethrogb:jb/duplicate-attribute-may…

b7357c4

…be_uninit_extra, r=kennytm Fix duplicate maybe_uninit_extra attribute Introduced in rust-lang#72414

nbdd0121 mentioned this pull request Jul 22, 2020

Implement reentrance detection for std::sync::Once #72311

Closed

Mark-Simulacrum mentioned this pull request Jul 24, 2020

Revert lazy init #74716

Closed

RalfJung reviewed Aug 19, 2020

View reviewed changes

This was referenced Mar 29, 2023

Tracking Issue for once_cell_try #109737

Open

Tracking Issue for lazy_cell #109736

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lazy initialization primitives to std #72414

Add lazy initialization primitives to std #72414

KodrAus commented May 21, 2020 •

edited

Loading

rust-highfive commented May 21, 2020

KodrAus commented May 21, 2020

bors commented May 29, 2020

KodrAus commented Jun 7, 2020

Mark-Simulacrum left a comment

Mark-Simulacrum Jun 16, 2020

KodrAus Jun 30, 2020

Mark-Simulacrum Jun 16, 2020

KodrAus Jun 30, 2020

KodrAus Jun 30, 2020 •

edited

Loading

bors commented Jun 28, 2020

KodrAus commented Jun 30, 2020

bors commented Jul 2, 2020

Keruspe Jul 9, 2020

matklad Jul 9, 2020

Mark-Simulacrum commented Jul 16, 2020

KodrAus commented Jul 16, 2020

KodrAus commented Jul 18, 2020

bors commented Jul 18, 2020

bors commented Jul 18, 2020

eddyb Jul 18, 2020

KodrAus Jul 18, 2020

eddyb Jul 18, 2020

RalfJung Jul 19, 2020

jethrogb commented Jul 20, 2020

RalfJung Aug 19, 2020

matklad Aug 19, 2020

Add lazy initialization primitives to std #72414

Add lazy initialization primitives to std #72414

Conversation

KodrAus commented May 21, 2020 • edited Loading

rust-highfive commented May 21, 2020

KodrAus commented May 21, 2020

bors commented May 29, 2020

KodrAus commented Jun 7, 2020

Mark-Simulacrum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KodrAus Jun 30, 2020 • edited Loading

Choose a reason for hiding this comment

bors commented Jun 28, 2020

KodrAus commented Jun 30, 2020

bors commented Jul 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mark-Simulacrum commented Jul 16, 2020

KodrAus commented Jul 16, 2020

KodrAus commented Jul 18, 2020

bors commented Jul 18, 2020

bors commented Jul 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jethrogb commented Jul 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KodrAus commented May 21, 2020 •

edited

Loading

KodrAus Jun 30, 2020 •

edited

Loading