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 Bytes::is_unique #643

Merged
merged 5 commits into from Jan 19, 2024
Merged

add Bytes::is_unique #643

merged 5 commits into from Jan 19, 2024

Conversation

Cyborus04
Copy link
Contributor

Closes #533

This adds the is_unique method to Bytes. It checks reference count when backed by (effectively) Arc<Vec<u8>>, returns true when backed by a vec, and returns false for static slices

src/bytes.rs Outdated
let kind = shared as usize & KIND_MASK;

if kind == KIND_ARC {
let ref_cnt = unsafe { (*shared.cast::<Shared>()).ref_cnt.load(Ordering::Relaxed) };
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 am unsure if Relaxed is correct here

@xonatius
Copy link

xonatius commented Jan 8, 2024

I think there might be an issue with this api: if is_unique returns true, there is no guarantee that it is still unique immediately after this call, as some other thread can hold the reference to this Bytes object and clone it while is_unique check is performed.

@Cyborus04
Copy link
Contributor Author

Ah, like if it was an Arc<Bytes>. Strange case, but still an issue. I think changing it to take &mut self should solve that?

@xonatius
Copy link

xonatius commented Jan 8, 2024

I was thinking about &Bytes rather than Arc<Bytes> (Bytes is Sync, right?), but yes - taking &mut self in this method should ensure this never happens (:

@Darksonn
Copy link
Contributor

Darksonn commented Jan 8, 2024

Normally you would add a method to the vtable and call it, rather than compare with the different vtables. That makes it more difficult to add more vtables in the future.

@Cyborus04
Copy link
Contributor Author

Makes sense. One moment, then.

@Darksonn
Copy link
Contributor

Darksonn commented Jan 9, 2024

You can add a comment that mentions that this could happen if the Bytes is used concurrently, but other than that, I don't think the concern that @xonatius mentioned is a problem. You shouldn't use &mut self just for that reason.

@Cyborus04
Copy link
Contributor Author

I agreed with the &mut self change since I have safety on the mind, as I was making this change hoping to later also add a method to get mutable access if the Bytes is unique. Even then, try_get_mut (or any other name) could itself take &mut self instead of this needing to, so I'll still revert it.

@Cyborus04
Copy link
Contributor Author

I have detailed my motivation for this in hyperium/http#662

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Please add tests for this. I'd like to include one test for each of the following cases:

  • Backed by static slice.
  • Promotable vtable with KIND_VEC.
  • Promotable vtable with KIND_ARC.
  • Shared vtable.

Other than testing, I think this is ready to be merged.

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Your tests are excellent, thanks.

@Darksonn Darksonn merged commit 0864aea into tokio-rs:master Jan 19, 2024
15 checks passed
@braddunbar braddunbar mentioned this pull request Mar 22, 2024
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.

Add a way to tell if Bytes is unique
3 participants