Skip to content
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 #72414

Merged
merged 7 commits into from
Jul 18, 2020
Merged

Add lazy initialization primitives to std #72414

merged 7 commits into from
Jul 18, 2020

Conversation

KodrAus
Copy link
Contributor

@KodrAus KodrAus commented May 21, 2020

Follow-up to #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:

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
Copy link
Collaborator

r? @sfackler

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 21, 2020
@KodrAus KodrAus changed the title Feat/stdlazy Add lazy initialization primitives to std May 21, 2020
@KodrAus
Copy link
Contributor Author

KodrAus commented May 21, 2020

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! 🙂

@KodrAus 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
@bors
Copy link
Contributor

bors commented May 29, 2020

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

@JohnCSimon 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
Copy link
Contributor Author

KodrAus commented Jun 7, 2020

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.

@KodrAus 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
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @matklad

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

src/libstd/lazy.rs Outdated Show resolved Hide resolved
src/libstd/lazy.rs Outdated Show resolved Hide resolved
src/libstd/lazy.rs Outdated Show resolved Hide resolved
thread: Cell::new(Some(thread::current())),
signaled: AtomicBool::new(false),
next: (current_state & !STATE_MASK) as *const Waiter,
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@KodrAus KodrAus Jun 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@KodrAus 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
@bors
Copy link
Contributor

bors commented Jun 28, 2020

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

@KodrAus
Copy link
Contributor Author

KodrAus commented Jun 30, 2020

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.

@KodrAus 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
@bors
Copy link
Contributor

bors commented Jul 2, 2020

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

@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

matklad and others added 5 commits July 17, 2020 07:23
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 <pitdicker@users.noreply.github.com>
@KodrAus
Copy link
Contributor Author

KodrAus commented Jul 16, 2020

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

@KodrAus
Copy link
Contributor Author

KodrAus commented Jul 18, 2020

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
Copy link
Contributor

bors commented Jul 18, 2020

📌 Commit fe63905 has been approved by Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Jul 18, 2020

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

@bors 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 added a commit to Manishearth/rust that referenced this pull request Jul 18, 2020
…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?
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 18, 2020
…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?
Comment on lines +30 to +33
pub struct OnceCell<T> {
// Invariant: written to at most once.
inner: UnsafeCell<Option<T>>,
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 18, 2020
…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
@bors bors merged commit 01418bd into rust-lang:master Jul 18, 2020
@jethrogb
Copy link
Contributor

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

Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 20, 2020
…be_uninit_extra, r=kennytm

Fix duplicate maybe_uninit_extra attribute

Introduced in rust-lang#72414
Comment on lines +553 to +554
// miri doesn't support threads
#[cfg(not(miri))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filed #75696

yvt added a commit to r3-os/r3 that referenced this pull request Jun 18, 2022
`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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.