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

struct Unique: Copy more from std and use for Rav1dPictureDataComponentInner::ptr to make it Send + Sync #1327

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

kkysen
Copy link
Collaborator

@kkysen kkysen commented Jul 18, 2024

We may be able to replace SendSyncNonNull with Unique, but we can do that later. They have basically the same APIs, though slightly different safety invariants (uniqueness vs. thread-safe).

@kkysen kkysen requested a review from rinon July 18, 2024 02:51
@@ -189,7 +192,7 @@ impl Rav1dPictureDataComponentInner {
/// so it is sound to further subdivide it into disjoint `&mut`s.
pub fn wrap_buf<BD: BitDepth>(buf: &mut [BD::Pixel], stride: usize) -> Self {
let buf = AsBytes::as_bytes_mut(buf);
let ptr = NonNull::new(buf.as_mut_ptr()).unwrap();
let ptr = Unique::from_ref_mut(buf).cast();
Copy link
Collaborator

Choose a reason for hiding this comment

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

How is this unique in the wrap_buf case? We're borrowing a reference to this data, right? So should we really use unique here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A &mut reference is always unique. So it can be &mut or owned. core::ptr::Unique impls From<&mut T> even.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But once that constructor returns the borrow is over, unless there's a lifetime. So it's no longer unique? How do we ensure no where else has a pointer to the same thing?

NonNull::new(self.as_strided_byte_mut_ptr().cast())
let ptr = NonNull::new(self.as_strided_byte_mut_ptr()).unwrap();
// SAFETY: The `ptr` originally comes from a `Unique` in `Rav1dPictureDataComponentInner::ptr`.
let ptr = unsafe { Unique::new(ptr) };
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, how is this unique? Can this method return a NonNull instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because self.as_strided_byte_mut_ptr() is derived from Rav1dPictureDataComponentInner::ptr, which is a Unique. I can say that more explicitly if that's clearer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does that mean there's two Uniques pointing to the same data? The original and the one into the middle?

Copy link
Collaborator

Choose a reason for hiding this comment

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

e.g. can I just call this twice and get two Uniques? Doesn't that violate the invariants?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ugh, yeah, you're right.

Why does core::ptr::Unique: Copy + Clone then?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No idea.

@@ -0,0 +1,190 @@
//! Largely copied from [`rust-lang/rust/library/core/src/ptr/unique.rs`](https://github.com/rust-lang/rust/blob/e35364a521372ce682e4bd4a5850d97ea33b0eab/library/core/src/ptr/unique.rs#L58).
Copy link
Collaborator

Choose a reason for hiding this comment

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

License? Copyright?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Umm I don't know how to do that. What do I write?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
//! Largely copied from [`rust-lang/rust/library/core/src/ptr/unique.rs`](https://github.com/rust-lang/rust/blob/e35364a521372ce682e4bd4a5850d97ea33b0eab/library/core/src/ptr/unique.rs#L58).
//! Largely copied from [`rust-lang/rust/library/core/src/ptr/unique.rs`](https://github.com/rust-lang/rust/blob/e35364a521372ce682e4bd4a5850d97ea33b0eab/library/core/src/ptr/unique.rs#L58).
//!
//! Used under the MIT license <LICENSE-MIT> or <http://opensource.org/licenses/MIT>.
//! Copyright is retained by the original Rust contributors.
//! For full authorship information, see the Rust compiler version control history or
//! https://thanks.rust-lang.org

@kkysen kkysen requested a review from rinon July 18, 2024 08:07
@rinon
Copy link
Collaborator

rinon commented Jul 18, 2024

To follow up on the discussion of "should DisjointMut require T: Sync," DisjointMut is effectively a ranged RwLock, which does require T: Sync to allow concurrent reads from multiple threads (which is what Sync means). I think we should impl Sync for this data pointer but I don't think it can be Unique. Could we just have a new PictureDataPtr type that wraps NonNull and documents the lifetime invariants?

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.

2 participants