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

Support slices in OnceRef and OnceBox #246

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nvzqz
Copy link

@nvzqz nvzqz commented Aug 10, 2023

This is implemented through a new public-in-private OncePointee trait. If this was instead done with impl OnceRef<[T]>, the compiler fails to infer which methods we want to use.

Because of the usage of core::ptr::slice_from_raw_parts in const, this change increases the MSRV to 1.64.0.

To make correctness easier to reason about, I did not use Ordering::Relaxed. However, it's likely that this code could be improved by it.

This is implemented through a new public-in-private `OncePointee` trait.
If this was instead done with `impl OnceRef<[T]>`, the compiler fails to
infer which methods we want to use.

Because of the usage of `core::ptr::slice_from_raw_parts` in `const`,
this change increases the MSRV to `1.64.0`.
@notgull
Copy link

notgull commented Aug 10, 2023

I would discourage bumping the MSRV past 1.63, as that is the current MSRV for Debian Stable

@matklad
Copy link
Owner

matklad commented Aug 10, 2023

Compatibility with Debian stable is an explicit non-goal for once_cell:

#201 (comment)

@nvzqz
Copy link
Author

nvzqz commented Aug 10, 2023

I can rewrite this to use an associated type via OncePointee instead of UnsafeCell<*mut T>. That should remove the need to bump the MSRV.


// Spin briefly until the other thread initializes the
// pointer.
ptr = loop {
Copy link
Owner

Choose a reason for hiding this comment

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

It’s a fundamental invariant of OnceCell that it doesn’t do unbounded spinning. If mutex-like blocking behavior is needed, then std dependency is required, because correct blocking fundamentally requires talking to the OS and notifying it which other thread you are blocked on, so that the OS can correctly guard against priority inversion.

Copy link
Author

Choose a reason for hiding this comment

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

What do you recommend we do here?

Copy link
Owner

Choose a reason for hiding this comment

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

My understanding is that no good solutions are available here: either a mutex or a cas is required. Though, I think Rust has double-word cas available on at least some architectures:

https://doc.rust-lang.org/stable/core/arch/x86_64/fn.cmpxchg16b.html

So I think the path forward here is to add support for DSTs via double-word-cas, by adding a bunch of cfgs everywhere. Eg, I'd imagine some 32 processors might have AtomicU64? Though, doing that probably requires a research of what's the status quo for wider-than-word atomic operations in today's Rust. That intrinsic exists, so its non-zero, but I don't know best practicies for actually using that.

@nvzqz
Copy link
Author

nvzqz commented Aug 10, 2023

@notgull I refactored the code to only require bumping to 1.61.0.

This allows for reducing the MSRV to `1.61.0`.
@nvzqz nvzqz force-pushed the nvzqz/once_ref_slice branch 2 times, most recently from 677c49a to 8b5bab0 Compare August 10, 2023 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants