From fad291f7e460304b3662db272c7f63fef5377cbc Mon Sep 17 00:00:00 2001 From: Adam Cimarosti Date: Mon, 21 Oct 2024 12:07:21 +0100 Subject: [PATCH 01/28] stub --- src/bytes.rs | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/src/bytes.rs b/src/bytes.rs index 8d7abd040..3b4cfe3bf 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -1012,6 +1012,73 @@ unsafe fn static_drop(_: &mut AtomicPtr<()>, _: *const u8, _: usize) { // nothing to drop for &'static [u8] } +// ===== impl ExternalVtable ===== + +/// Manage the lifetime of a [Bytes] buffer externally. +/// +/// The trait is marked as `unsafe` since it needs to ensure that the slice +/// of returned by [AsRef<[u8]>](AsRef) retains a fixed address in memory, +/// tied the lifetime of the trait implementor. +/// +/// A common use case for implementing the [ExternalBytesOwner] is when +/// the memory is owned by external means such as a memory mapped file. +pub unsafe trait ExternalBytesOwner : Drop + AsRef<[u8]> {} + +struct External { + owner: *const dyn ExternalBytesOwner, + ref_cnt: AtomicUsize, +} + +impl Clone for External { + fn clone(&self) -> Self { + todo!() + } +} + +impl Drop for External { + fn drop(&mut self) { + todo!() + } +} + +unsafe fn external_clone(data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> Bytes { + let external = data.load(Ordering::Acquire); + let external = &*external.cast::(); + todo!() +} + +unsafe fn external_to_vec(data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> Vec { + let external = data.load(Ordering::Acquire); + let external = &*external.cast::(); + todo!() +} + +unsafe fn external_to_mut(data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> BytesMut { + let external = data.load(Ordering::Acquire); + let external = &*external.cast::(); + todo!() +} + +unsafe fn external_is_unique(data: &AtomicPtr<()>) -> bool { + let external = data.load(Ordering::Acquire); + let external = &*external.cast::(); + todo!() +} + +unsafe fn external_drop(data: &mut AtomicPtr<()>, ptr: *const u8, len: usize) { + let external = data.load(Ordering::Acquire); + let external = &*external.cast::(); + todo!() +} + +static EXTERNAL_VTABLE: Vtable = Vtable { + clone: external_clone, + to_vec: external_to_vec, + to_mut: external_to_mut, + is_unique: external_is_unique, + drop: external_drop, +}; + // ===== impl PromotableVtable ===== static PROMOTABLE_EVEN_VTABLE: Vtable = Vtable { From 905b6af7b950626220e6fe65c6dd4340ee553e61 Mon Sep 17 00:00:00 2001 From: Adam Cimarosti Date: Mon, 21 Oct 2024 13:19:05 +0100 Subject: [PATCH 02/28] not to lose the train of thought.. --- src/bytes.rs | 45 ++++++++++++++++++++++++++++++++++----------- 1 file changed, 34 insertions(+), 11 deletions(-) diff --git a/src/bytes.rs b/src/bytes.rs index 3b4cfe3bf..e6f888101 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -200,6 +200,23 @@ impl Bytes { } } + pub unsafe fn from_external(owner: impl ExternalBytesOwner) -> Self { + let slice = owner.as_ref(); + let ptr = slice.as_ptr(); + let len = slice.len(); + let external = External { + ref_cnt: Default::default(), + owner, // TODO(amunra): Remove pointer of a pointer. + }; + let external = Box::into_raw(Box::new(external)); + Bytes { + ptr, + len, + data: AtomicPtr::new(external as _), + vtable: &EXTERNAL_VTABLE, + } + } + /// Returns the number of bytes contained in this `Bytes`. /// /// # Examples @@ -1024,18 +1041,13 @@ unsafe fn static_drop(_: &mut AtomicPtr<()>, _: *const u8, _: usize) { /// the memory is owned by external means such as a memory mapped file. pub unsafe trait ExternalBytesOwner : Drop + AsRef<[u8]> {} -struct External { - owner: *const dyn ExternalBytesOwner, +#[repr(C)] +struct External { ref_cnt: AtomicUsize, + owner: T, } -impl Clone for External { - fn clone(&self) -> Self { - todo!() - } -} - -impl Drop for External { +impl Drop for External { fn drop(&mut self) { todo!() } @@ -1043,8 +1055,19 @@ impl Drop for External { unsafe fn external_clone(data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> Bytes { let external = data.load(Ordering::Acquire); - let external = &*external.cast::(); - todo!() + let ref_cnt = &*external.cast::(); + let old_size = ref_cnt.fetch_add(1, Ordering::Relaxed); // TODO(amunra): Triple-check ordering. + + if old_size > usize::MAX >> 1 { + crate::abort() + } + + Bytes { + ptr, + len, + data: AtomicPtr::new(external as _), + vtable: &EXTERNAL_VTABLE + } } unsafe fn external_to_vec(data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> Vec { From e7ccb3102cfd8d361f16b84e90ed1fde1fe85d76 Mon Sep 17 00:00:00 2001 From: Adam Cimarosti Date: Tue, 22 Oct 2024 15:10:57 +0100 Subject: [PATCH 03/28] we're getting there.. --- src/bytes.rs | 68 ++++++++++++++++++++++++++++++---------------------- 1 file changed, 39 insertions(+), 29 deletions(-) diff --git a/src/bytes.rs b/src/bytes.rs index e6f888101..7538f2cbf 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -200,15 +200,15 @@ impl Bytes { } } - pub unsafe fn from_external(owner: impl ExternalBytesOwner) -> Self { - let slice = owner.as_ref(); - let ptr = slice.as_ptr(); - let len = slice.len(); - let external = External { - ref_cnt: Default::default(), - owner, // TODO(amunra): Remove pointer of a pointer. - }; - let external = Box::into_raw(Box::new(external)); + pub unsafe fn from_external(ptr: *const u8, len: usize, owner: T) -> Self { + let external = Box::into_raw(Box::new(External { + lifetime: ExternalLifetime { + ref_cnt: Default::default(), + drop: external_box_and_drop::, + }, + owner, + })); + Bytes { ptr, len, @@ -1039,24 +1039,30 @@ unsafe fn static_drop(_: &mut AtomicPtr<()>, _: *const u8, _: usize) { /// /// A common use case for implementing the [ExternalBytesOwner] is when /// the memory is owned by external means such as a memory mapped file. -pub unsafe trait ExternalBytesOwner : Drop + AsRef<[u8]> {} #[repr(C)] -struct External { +struct ExternalLifetime { ref_cnt: AtomicUsize, + drop: unsafe fn(*mut ()), +} + +#[repr(C)] +struct External { + lifetime: ExternalLifetime, owner: T, } -impl Drop for External { - fn drop(&mut self) { - todo!() - } +unsafe fn external_box_and_drop(ptr: *mut ()) { + let b: Box> = Box::from_raw(ptr as _); + drop(b); } + + unsafe fn external_clone(data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> Bytes { let external = data.load(Ordering::Acquire); - let ref_cnt = &*external.cast::(); - let old_size = ref_cnt.fetch_add(1, Ordering::Relaxed); // TODO(amunra): Triple-check ordering. + let ref_cnt = &(*external.cast::()).ref_cnt; + let old_size = ref_cnt.fetch_add(1, Ordering::Relaxed); if old_size > usize::MAX >> 1 { crate::abort() @@ -1070,28 +1076,32 @@ unsafe fn external_clone(data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> By } } -unsafe fn external_to_vec(data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> Vec { - let external = data.load(Ordering::Acquire); - let external = &*external.cast::(); - todo!() +unsafe fn external_to_vec(_data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> Vec { + let slice = slice::from_raw_parts(ptr, len); + slice.to_vec() } unsafe fn external_to_mut(data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> BytesMut { - let external = data.load(Ordering::Acquire); - let external = &*external.cast::(); - todo!() + BytesMut::from_vec(external_to_vec(data, ptr, len)) } unsafe fn external_is_unique(data: &AtomicPtr<()>) -> bool { let external = data.load(Ordering::Acquire); - let external = &*external.cast::(); - todo!() + let ref_cnt = &(*external.cast::()).ref_cnt; + ref_cnt.load(Ordering::Relaxed) == 1 } -unsafe fn external_drop(data: &mut AtomicPtr<()>, ptr: *const u8, len: usize) { +unsafe fn external_drop(data: &mut AtomicPtr<()>, _ptr: *const u8, _len: usize) { let external = data.load(Ordering::Acquire); - let external = &*external.cast::(); - todo!() + let lifetime = &*external.cast::(); + let ref_cnt = &lifetime.ref_cnt; + + if ref_cnt.fetch_sub(1, Ordering::Release) != 1 { + return; + } + + let drop = lifetime.drop; + drop(external) } static EXTERNAL_VTABLE: Vtable = Vtable { From 713e09c811ba0bf6257745210d153cf02a74b501 Mon Sep 17 00:00:00 2001 From: Adam Cimarosti Date: Tue, 22 Oct 2024 15:23:41 +0100 Subject: [PATCH 04/28] fmt fix --- src/bytes.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/bytes.rs b/src/bytes.rs index 7538f2cbf..36cc86dc6 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -1057,8 +1057,6 @@ unsafe fn external_box_and_drop(ptr: *mut ()) { drop(b); } - - unsafe fn external_clone(data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> Bytes { let external = data.load(Ordering::Acquire); let ref_cnt = &(*external.cast::()).ref_cnt; From 811c11c8da26259eba7b23f58b172089cb982c78 Mon Sep 17 00:00:00 2001 From: Adam Cimarosti Date: Tue, 22 Oct 2024 16:03:12 +0100 Subject: [PATCH 05/28] better naming --- src/bytes.rs | 97 +++++++++++++++++++++++++++++----------------------- 1 file changed, 55 insertions(+), 42 deletions(-) diff --git a/src/bytes.rs b/src/bytes.rs index 36cc86dc6..82ba57a1c 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -200,11 +200,33 @@ impl Bytes { } } - pub unsafe fn from_external(ptr: *const u8, len: usize, owner: T) -> Self { - let external = Box::into_raw(Box::new(External { - lifetime: ExternalLifetime { + /// Create [Bytes] with a buffer whose lifetime is controlled + /// via an explicit owner. + /// + /// A common use case is creating bytes to a memory mapped file, + /// allowing for zero-copy construction. + /// + /// ```no_run + /// use bytes::Bytes; + /// use std::fs::File; + /// use memmap2::Map; + /// + /// let mut file = File::open("upload_bundle.tar.gz"); + /// let b = unsafe { + /// let mmap = Mmap::map(&file); + /// let ptr = mmap.as_ptr(); + /// let len = mmap.len(); + /// Bytes::with_owner(ptr, len, mmap) + /// }; + /// ``` + /// + /// Converting to a vector or a [BytesMut] will create a deep copy of + /// the buffer. + pub unsafe fn with_owner(ptr: *const u8, len: usize, owner: T) -> Self { + let owned = Box::into_raw(Box::new(Owned { + lifetime: OwnedLifetime { ref_cnt: Default::default(), - drop: external_box_and_drop::, + drop: owned_box_and_drop::, }, owner, })); @@ -212,8 +234,8 @@ impl Bytes { Bytes { ptr, len, - data: AtomicPtr::new(external as _), - vtable: &EXTERNAL_VTABLE, + data: AtomicPtr::new(owned as _), + vtable: &OWNED_VTABLE, } } @@ -1029,37 +1051,28 @@ unsafe fn static_drop(_: &mut AtomicPtr<()>, _: *const u8, _: usize) { // nothing to drop for &'static [u8] } -// ===== impl ExternalVtable ===== - -/// Manage the lifetime of a [Bytes] buffer externally. -/// -/// The trait is marked as `unsafe` since it needs to ensure that the slice -/// of returned by [AsRef<[u8]>](AsRef) retains a fixed address in memory, -/// tied the lifetime of the trait implementor. -/// -/// A common use case for implementing the [ExternalBytesOwner] is when -/// the memory is owned by external means such as a memory mapped file. +// ===== impl OwnedVtable ===== #[repr(C)] -struct ExternalLifetime { +struct OwnedLifetime { ref_cnt: AtomicUsize, drop: unsafe fn(*mut ()), } #[repr(C)] -struct External { - lifetime: ExternalLifetime, +struct Owned { + lifetime: OwnedLifetime, owner: T, } -unsafe fn external_box_and_drop(ptr: *mut ()) { - let b: Box> = Box::from_raw(ptr as _); +unsafe fn owned_box_and_drop(ptr: *mut ()) { + let b: Box> = Box::from_raw(ptr as _); drop(b); } -unsafe fn external_clone(data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> Bytes { - let external = data.load(Ordering::Acquire); - let ref_cnt = &(*external.cast::()).ref_cnt; +unsafe fn owned_clone(data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> Bytes { + let owned = data.load(Ordering::Acquire); + let ref_cnt = &(*owned.cast::()).ref_cnt; let old_size = ref_cnt.fetch_add(1, Ordering::Relaxed); if old_size > usize::MAX >> 1 { @@ -1069,29 +1082,29 @@ unsafe fn external_clone(data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> By Bytes { ptr, len, - data: AtomicPtr::new(external as _), - vtable: &EXTERNAL_VTABLE + data: AtomicPtr::new(owned as _), + vtable: &OWNED_VTABLE } } -unsafe fn external_to_vec(_data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> Vec { +unsafe fn owned_to_vec(_data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> Vec { let slice = slice::from_raw_parts(ptr, len); slice.to_vec() } -unsafe fn external_to_mut(data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> BytesMut { - BytesMut::from_vec(external_to_vec(data, ptr, len)) +unsafe fn owned_to_mut(data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> BytesMut { + BytesMut::from_vec(owned_to_vec(data, ptr, len)) } -unsafe fn external_is_unique(data: &AtomicPtr<()>) -> bool { - let external = data.load(Ordering::Acquire); - let ref_cnt = &(*external.cast::()).ref_cnt; +unsafe fn owned_is_unique(data: &AtomicPtr<()>) -> bool { + let owned = data.load(Ordering::Acquire); + let ref_cnt = &(*owned.cast::()).ref_cnt; ref_cnt.load(Ordering::Relaxed) == 1 } -unsafe fn external_drop(data: &mut AtomicPtr<()>, _ptr: *const u8, _len: usize) { - let external = data.load(Ordering::Acquire); - let lifetime = &*external.cast::(); +unsafe fn owned_drop(data: &mut AtomicPtr<()>, _ptr: *const u8, _len: usize) { + let owned = data.load(Ordering::Acquire); + let lifetime = &*owned.cast::(); let ref_cnt = &lifetime.ref_cnt; if ref_cnt.fetch_sub(1, Ordering::Release) != 1 { @@ -1099,15 +1112,15 @@ unsafe fn external_drop(data: &mut AtomicPtr<()>, _ptr: *const u8, _len: usize) } let drop = lifetime.drop; - drop(external) + drop(owned) } -static EXTERNAL_VTABLE: Vtable = Vtable { - clone: external_clone, - to_vec: external_to_vec, - to_mut: external_to_mut, - is_unique: external_is_unique, - drop: external_drop, +static OWNED_VTABLE: Vtable = Vtable { + clone: owned_clone, + to_vec: owned_to_vec, + to_mut: owned_to_mut, + is_unique: owned_is_unique, + drop: owned_drop, }; // ===== impl PromotableVtable ===== From ddf5f282ecfc66075e3e1a4f2051d02c2e30d85a Mon Sep 17 00:00:00 2001 From: Adam Cimarosti Date: Tue, 22 Oct 2024 17:30:44 +0100 Subject: [PATCH 06/28] a few tests and bug fixes --- src/bytes.rs | 51 ++++++++++++++++++++++++++++++--------- src/bytes_mut.rs | 3 ++- tests/test_bytes.rs | 59 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 100 insertions(+), 13 deletions(-) diff --git a/src/bytes.rs b/src/bytes.rs index 82ba57a1c..31774e570 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -10,7 +10,7 @@ use alloc::{ string::String, vec::Vec, }; - +use std::eprintln; use crate::buf::IntoIter; #[allow(unused)] use crate::loom::sync::atomic::AtomicMut; @@ -116,6 +116,7 @@ pub(crate) struct Vtable { pub to_mut: unsafe fn(&AtomicPtr<()>, *const u8, usize) -> BytesMut, /// fn(data) pub is_unique: unsafe fn(&AtomicPtr<()>) -> bool, + pub cheap_into_mut: unsafe fn(&AtomicPtr<()>) -> bool, /// fn(data, ptr, len) pub drop: unsafe fn(&mut AtomicPtr<()>, *const u8, usize), } @@ -225,7 +226,7 @@ impl Bytes { pub unsafe fn with_owner(ptr: *const u8, len: usize, owner: T) -> Self { let owned = Box::into_raw(Box::new(Owned { lifetime: OwnedLifetime { - ref_cnt: Default::default(), + ref_cnt: AtomicUsize::new(1), drop: owned_box_and_drop::, }, owner, @@ -575,6 +576,9 @@ impl Bytes { /// If `self` is not unique for the entire original buffer, this will fail /// and return self. /// + /// This will also always fail if the buffer was constructed via + /// [with_owner](Bytes::with_owner). + /// /// # Examples /// /// ``` @@ -584,7 +588,7 @@ impl Bytes { /// assert_eq!(bytes.try_into_mut(), Ok(BytesMut::from(&b"hello"[..]))); /// ``` pub fn try_into_mut(self) -> Result { - if self.is_unique() { + if unsafe { (self.vtable.cheap_into_mut)(&self.data) } { Ok(self.into()) } else { Err(self) @@ -1025,6 +1029,7 @@ const STATIC_VTABLE: Vtable = Vtable { to_vec: static_to_vec, to_mut: static_to_mut, is_unique: static_is_unique, + cheap_into_mut: static_is_unique, drop: static_drop, }; @@ -1073,9 +1078,10 @@ unsafe fn owned_box_and_drop(ptr: *mut ()) { unsafe fn owned_clone(data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> Bytes { let owned = data.load(Ordering::Acquire); let ref_cnt = &(*owned.cast::()).ref_cnt; - let old_size = ref_cnt.fetch_add(1, Ordering::Relaxed); + let old_cnt = ref_cnt.fetch_add(1, Ordering::Relaxed); + eprintln!("owned_clone :: (A) owned: {owned:p}, old_cnt: {old_cnt} -> {}", old_cnt + 1); - if old_size > usize::MAX >> 1 { + if old_cnt > usize::MAX >> 1 { crate::abort() } @@ -1093,7 +1099,9 @@ unsafe fn owned_to_vec(_data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> Vec } unsafe fn owned_to_mut(data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> BytesMut { - BytesMut::from_vec(owned_to_vec(data, ptr, len)) + let bytes_mut = BytesMut::from_vec(owned_to_vec(data, ptr, len)); + owned_drop_impl(data.load(Ordering::Acquire)); + bytes_mut } unsafe fn owned_is_unique(data: &AtomicPtr<()>) -> bool { @@ -1102,24 +1110,40 @@ unsafe fn owned_is_unique(data: &AtomicPtr<()>) -> bool { ref_cnt.load(Ordering::Relaxed) == 1 } -unsafe fn owned_drop(data: &mut AtomicPtr<()>, _ptr: *const u8, _len: usize) { - let owned = data.load(Ordering::Acquire); - let lifetime = &*owned.cast::(); - let ref_cnt = &lifetime.ref_cnt; +unsafe fn owned_cheap_into_mut(data: &AtomicPtr<()>) -> bool { + // Since the memory's ownership is tied to an external owner + // it is never zero-copy to create a BytesMut. + false +} - if ref_cnt.fetch_sub(1, Ordering::Release) != 1 { +unsafe fn owned_drop_impl(owned: *mut ()) { + let lifetime = owned.cast::(); + let ref_cnt = &(*lifetime).ref_cnt; + + let old_cnt = ref_cnt.fetch_sub(1, Ordering::Release); + if old_cnt != 1 { + eprintln!("owned_drop :: (A) owned: {owned:p}, old_cnt: {old_cnt} -> {} -- CONTINUING!", + old_cnt - 1); return; } - let drop = lifetime.drop; + eprintln!("owned_drop :: (B) owned: {owned:p}, old_cnt: {old_cnt} -> 0 -- DROPPING!"); + + let drop = &(*lifetime).drop; drop(owned) } +unsafe fn owned_drop(data: &mut AtomicPtr<()>, _ptr: *const u8, _len: usize) { + let owned = data.load(Ordering::Acquire); + owned_drop_impl(owned); +} + static OWNED_VTABLE: Vtable = Vtable { clone: owned_clone, to_vec: owned_to_vec, to_mut: owned_to_mut, is_unique: owned_is_unique, + cheap_into_mut: owned_cheap_into_mut, drop: owned_drop, }; @@ -1130,6 +1154,7 @@ static PROMOTABLE_EVEN_VTABLE: Vtable = Vtable { to_vec: promotable_even_to_vec, to_mut: promotable_even_to_mut, is_unique: promotable_is_unique, + cheap_into_mut: promotable_is_unique, drop: promotable_even_drop, }; @@ -1138,6 +1163,7 @@ static PROMOTABLE_ODD_VTABLE: Vtable = Vtable { to_vec: promotable_odd_to_vec, to_mut: promotable_odd_to_mut, is_unique: promotable_is_unique, + cheap_into_mut: promotable_is_unique, drop: promotable_odd_drop, }; @@ -1314,6 +1340,7 @@ static SHARED_VTABLE: Vtable = Vtable { to_vec: shared_to_vec, to_mut: shared_to_mut, is_unique: shared_is_unique, + cheap_into_mut: shared_is_unique, drop: shared_drop, }; diff --git a/src/bytes_mut.rs b/src/bytes_mut.rs index a0d77bc24..bc52ab76e 100644 --- a/src/bytes_mut.rs +++ b/src/bytes_mut.rs @@ -13,7 +13,7 @@ use alloc::{ }; use crate::buf::{IntoIter, UninitSlice}; -use crate::bytes::Vtable; +use crate::bytes::{shared_is_unique, Vtable}; #[allow(unused)] use crate::loom::sync::atomic::AtomicMut; use crate::loom::sync::atomic::{AtomicPtr, AtomicUsize, Ordering}; @@ -1776,6 +1776,7 @@ static SHARED_VTABLE: Vtable = Vtable { to_vec: shared_v_to_vec, to_mut: shared_v_to_mut, is_unique: shared_v_is_unique, + cheap_into_mut: shared_is_unique, drop: shared_v_drop, }; diff --git a/tests/test_bytes.rs b/tests/test_bytes.rs index 71f0e6681..a5bfed213 100644 --- a/tests/test_bytes.rs +++ b/tests/test_bytes.rs @@ -1,5 +1,7 @@ #![warn(rust_2018_idioms)] +use std::cell::Cell; +use std::rc::Rc; use bytes::{Buf, BufMut, Bytes, BytesMut}; use std::usize; @@ -1479,3 +1481,60 @@ fn split_to_empty_addr_mut() { let _ = &empty_start[..]; let _ = &buf[..]; } + +// N.B.: Using `Rc` and `Cell` rather than `Arc` and `usize` +// since this forces the type to be !Send + !Sync, ensuring +// in this test that the owner remains a generic T. +#[derive(Clone)] +struct OwnedTester(Rc>); + +impl Drop for OwnedTester { + fn drop(&mut self) { + eprintln!("OwnedTester.drop :: (A) {:p}", self as *mut Self); + let current = self.0.get(); + self.0.set(current + 1) + } +} + +#[test] +fn owned_basic() { + let drop_counter = Rc::new(Cell::new(0)); + let owner = OwnedTester(drop_counter.clone()); + let b1 = unsafe { + Bytes::with_owner(SHORT.as_ptr(), SHORT.len(), owner) + }; + assert!(b1.is_unique()); + let b2 = b1.clone(); + assert!(!b1.is_unique()); + assert!(!b2.is_unique()); + assert_eq!(drop_counter.get(), 0); + drop(b1); + assert_eq!(drop_counter.get(), 0); + assert!(b2.is_unique()); + drop(b2); + assert_eq!(drop_counter.get(), 1); +} + +#[test] +fn owned_to_mut() { + let drop_counter = Rc::new(Cell::new(0)); + let owner = OwnedTester(drop_counter.clone()); + let b1 = unsafe { + Bytes::with_owner(SHORT.as_ptr(), SHORT.len(), owner) + }; + + // Holding an owner will fail converting to a BytesMut, + // even when the bytes instance is unique. + assert!(b1.is_unique()); + let b1 = b1.try_into_mut().unwrap_err(); + assert_eq!(drop_counter.get(), 0); + + // That said, it's still possible, just not cheap. + let bm1: BytesMut = b1.into(); + let new_buf = &bm1[..]; + assert_eq!(new_buf, SHORT); + + assert_eq!(drop_counter.get(), 1); + drop(bm1); + assert_eq!(drop_counter.get(), 1); +} \ No newline at end of file From dc984652cf1895dbed790da6f6cd39c3c86bef05 Mon Sep 17 00:00:00 2001 From: Adam Cimarosti Date: Tue, 22 Oct 2024 17:45:23 +0100 Subject: [PATCH 07/28] cargo fmt --- src/bytes.rs | 25 +++++++++++++++---------- tests/test_bytes.rs | 18 ++++++++++-------- 2 files changed, 25 insertions(+), 18 deletions(-) diff --git a/src/bytes.rs b/src/bytes.rs index 31774e570..0cd7ebfd6 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -3,6 +3,11 @@ use core::mem::{self, ManuallyDrop}; use core::ops::{Deref, RangeBounds}; use core::{cmp, fmt, hash, ptr, slice, usize}; +use crate::buf::IntoIter; +#[allow(unused)] +use crate::loom::sync::atomic::AtomicMut; +use crate::loom::sync::atomic::{AtomicPtr, AtomicUsize, Ordering}; +use crate::{offset_from, Buf, BytesMut}; use alloc::{ alloc::{dealloc, Layout}, borrow::Borrow, @@ -11,11 +16,6 @@ use alloc::{ vec::Vec, }; use std::eprintln; -use crate::buf::IntoIter; -#[allow(unused)] -use crate::loom::sync::atomic::AtomicMut; -use crate::loom::sync::atomic::{AtomicPtr, AtomicUsize, Ordering}; -use crate::{offset_from, Buf, BytesMut}; /// A cheaply cloneable and sliceable chunk of contiguous memory. /// @@ -1079,7 +1079,10 @@ unsafe fn owned_clone(data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> Bytes let owned = data.load(Ordering::Acquire); let ref_cnt = &(*owned.cast::()).ref_cnt; let old_cnt = ref_cnt.fetch_add(1, Ordering::Relaxed); - eprintln!("owned_clone :: (A) owned: {owned:p}, old_cnt: {old_cnt} -> {}", old_cnt + 1); + eprintln!( + "owned_clone :: (A) owned: {owned:p}, old_cnt: {old_cnt} -> {}", + old_cnt + 1 + ); if old_cnt > usize::MAX >> 1 { crate::abort() @@ -1089,7 +1092,7 @@ unsafe fn owned_clone(data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> Bytes ptr, len, data: AtomicPtr::new(owned as _), - vtable: &OWNED_VTABLE + vtable: &OWNED_VTABLE, } } @@ -1110,7 +1113,7 @@ unsafe fn owned_is_unique(data: &AtomicPtr<()>) -> bool { ref_cnt.load(Ordering::Relaxed) == 1 } -unsafe fn owned_cheap_into_mut(data: &AtomicPtr<()>) -> bool { +unsafe fn owned_cheap_into_mut(_data: &AtomicPtr<()>) -> bool { // Since the memory's ownership is tied to an external owner // it is never zero-copy to create a BytesMut. false @@ -1122,8 +1125,10 @@ unsafe fn owned_drop_impl(owned: *mut ()) { let old_cnt = ref_cnt.fetch_sub(1, Ordering::Release); if old_cnt != 1 { - eprintln!("owned_drop :: (A) owned: {owned:p}, old_cnt: {old_cnt} -> {} -- CONTINUING!", - old_cnt - 1); + eprintln!( + "owned_drop :: (A) owned: {owned:p}, old_cnt: {old_cnt} -> {} -- CONTINUING!", + old_cnt - 1 + ); return; } diff --git a/tests/test_bytes.rs b/tests/test_bytes.rs index a5bfed213..671595c2b 100644 --- a/tests/test_bytes.rs +++ b/tests/test_bytes.rs @@ -1,8 +1,8 @@ #![warn(rust_2018_idioms)] +use bytes::{Buf, BufMut, Bytes, BytesMut}; use std::cell::Cell; use std::rc::Rc; -use bytes::{Buf, BufMut, Bytes, BytesMut}; use std::usize; @@ -1500,9 +1500,7 @@ impl Drop for OwnedTester { fn owned_basic() { let drop_counter = Rc::new(Cell::new(0)); let owner = OwnedTester(drop_counter.clone()); - let b1 = unsafe { - Bytes::with_owner(SHORT.as_ptr(), SHORT.len(), owner) - }; + let b1 = unsafe { Bytes::with_owner(SHORT.as_ptr(), SHORT.len(), owner) }; assert!(b1.is_unique()); let b2 = b1.clone(); assert!(!b1.is_unique()); @@ -1511,7 +1509,13 @@ fn owned_basic() { drop(b1); assert_eq!(drop_counter.get(), 0); assert!(b2.is_unique()); + let b3 = b2.slice(1..b2.len() - 1); + assert!(!b2.is_unique()); + assert!(!b3.is_unique()); drop(b2); + assert_eq!(drop_counter.get(), 0); + assert!(b3.is_unique()); + drop(b3); assert_eq!(drop_counter.get(), 1); } @@ -1519,9 +1523,7 @@ fn owned_basic() { fn owned_to_mut() { let drop_counter = Rc::new(Cell::new(0)); let owner = OwnedTester(drop_counter.clone()); - let b1 = unsafe { - Bytes::with_owner(SHORT.as_ptr(), SHORT.len(), owner) - }; + let b1 = unsafe { Bytes::with_owner(SHORT.as_ptr(), SHORT.len(), owner) }; // Holding an owner will fail converting to a BytesMut, // even when the bytes instance is unique. @@ -1537,4 +1539,4 @@ fn owned_to_mut() { assert_eq!(drop_counter.get(), 1); drop(bm1); assert_eq!(drop_counter.get(), 1); -} \ No newline at end of file +} From bd895231153bee850e689b0c7986c5b8b5087a42 Mon Sep 17 00:00:00 2001 From: Adam Cimarosti Date: Tue, 22 Oct 2024 18:06:52 +0100 Subject: [PATCH 08/28] docs --- src/bytes.rs | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/bytes.rs b/src/bytes.rs index 0cd7ebfd6..543c18a2c 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -204,25 +204,31 @@ impl Bytes { /// Create [Bytes] with a buffer whose lifetime is controlled /// via an explicit owner. /// - /// A common use case is creating bytes to a memory mapped file, - /// allowing for zero-copy construction. + /// A common use case is to zero-copy construct from mapped memory. /// /// ```no_run /// use bytes::Bytes; /// use std::fs::File; /// use memmap2::Map; /// - /// let mut file = File::open("upload_bundle.tar.gz"); + /// let file = File::open("upload_bundle.tar.gz")?; /// let b = unsafe { - /// let mmap = Mmap::map(&file); + /// let mmap = Mmap::map(&file)?; /// let ptr = mmap.as_ptr(); /// let len = mmap.len(); /// Bytes::with_owner(ptr, len, mmap) /// }; /// ``` /// - /// Converting to a vector or a [BytesMut] will create a deep copy of - /// the buffer. + /// The function is marked `unsafe` because it requires the caller to ensure + /// that the memory region specified by `ptr` and `len` remains valid and + /// linked to the lifetime of the provided owner. + /// The `owner` will be transferred to the constructed [Bytes] object which + /// will ensure it is dropped once all remaining clones of the constructed + /// object are dropped. + /// + /// Note that converting [Bytes] with an owner into a [BytesMut] will + /// always create a deep copy of the buffer into newly allocated memory. pub unsafe fn with_owner(ptr: *const u8, len: usize, owner: T) -> Self { let owned = Box::into_raw(Box::new(Owned { lifetime: OwnedLifetime { From d419b2bc78a124d00e58adb545906ebb2166a198 Mon Sep 17 00:00:00 2001 From: Adam Cimarosti Date: Tue, 22 Oct 2024 23:24:50 +0100 Subject: [PATCH 09/28] code cleanup and added a missing test case --- src/bytes.rs | 26 ++++++++------------------ src/bytes_mut.rs | 4 ++-- tests/test_bytes.rs | 16 +++++++++++++++- 3 files changed, 25 insertions(+), 21 deletions(-) diff --git a/src/bytes.rs b/src/bytes.rs index 543c18a2c..bf9b9f44b 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -3,11 +3,6 @@ use core::mem::{self, ManuallyDrop}; use core::ops::{Deref, RangeBounds}; use core::{cmp, fmt, hash, ptr, slice, usize}; -use crate::buf::IntoIter; -#[allow(unused)] -use crate::loom::sync::atomic::AtomicMut; -use crate::loom::sync::atomic::{AtomicPtr, AtomicUsize, Ordering}; -use crate::{offset_from, Buf, BytesMut}; use alloc::{ alloc::{dealloc, Layout}, borrow::Borrow, @@ -15,7 +10,12 @@ use alloc::{ string::String, vec::Vec, }; -use std::eprintln; + +use crate::buf::IntoIter; +#[allow(unused)] +use crate::loom::sync::atomic::AtomicMut; +use crate::loom::sync::atomic::{AtomicPtr, AtomicUsize, Ordering}; +use crate::{offset_from, Buf, BytesMut}; /// A cheaply cloneable and sliceable chunk of contiguous memory. /// @@ -225,7 +225,8 @@ impl Bytes { /// linked to the lifetime of the provided owner. /// The `owner` will be transferred to the constructed [Bytes] object which /// will ensure it is dropped once all remaining clones of the constructed - /// object are dropped. + /// object are dropped. The owner will then be responsible for dropping the + /// specified region of memory as part of its [Drop] implementation. /// /// Note that converting [Bytes] with an owner into a [BytesMut] will /// always create a deep copy of the buffer into newly allocated memory. @@ -1085,11 +1086,6 @@ unsafe fn owned_clone(data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> Bytes let owned = data.load(Ordering::Acquire); let ref_cnt = &(*owned.cast::()).ref_cnt; let old_cnt = ref_cnt.fetch_add(1, Ordering::Relaxed); - eprintln!( - "owned_clone :: (A) owned: {owned:p}, old_cnt: {old_cnt} -> {}", - old_cnt + 1 - ); - if old_cnt > usize::MAX >> 1 { crate::abort() } @@ -1131,15 +1127,9 @@ unsafe fn owned_drop_impl(owned: *mut ()) { let old_cnt = ref_cnt.fetch_sub(1, Ordering::Release); if old_cnt != 1 { - eprintln!( - "owned_drop :: (A) owned: {owned:p}, old_cnt: {old_cnt} -> {} -- CONTINUING!", - old_cnt - 1 - ); return; } - eprintln!("owned_drop :: (B) owned: {owned:p}, old_cnt: {old_cnt} -> 0 -- DROPPING!"); - let drop = &(*lifetime).drop; drop(owned) } diff --git a/src/bytes_mut.rs b/src/bytes_mut.rs index bc52ab76e..48fb3e69c 100644 --- a/src/bytes_mut.rs +++ b/src/bytes_mut.rs @@ -13,7 +13,7 @@ use alloc::{ }; use crate::buf::{IntoIter, UninitSlice}; -use crate::bytes::{shared_is_unique, Vtable}; +use crate::bytes::Vtable; #[allow(unused)] use crate::loom::sync::atomic::AtomicMut; use crate::loom::sync::atomic::{AtomicPtr, AtomicUsize, Ordering}; @@ -1776,7 +1776,7 @@ static SHARED_VTABLE: Vtable = Vtable { to_vec: shared_v_to_vec, to_mut: shared_v_to_mut, is_unique: shared_v_is_unique, - cheap_into_mut: shared_is_unique, + cheap_into_mut: shared_v_is_unique, drop: shared_v_drop, }; diff --git a/tests/test_bytes.rs b/tests/test_bytes.rs index 671595c2b..08903e204 100644 --- a/tests/test_bytes.rs +++ b/tests/test_bytes.rs @@ -1490,7 +1490,6 @@ struct OwnedTester(Rc>); impl Drop for OwnedTester { fn drop(&mut self) { - eprintln!("OwnedTester.drop :: (A) {:p}", self as *mut Self); let current = self.0.get(); self.0.set(current + 1) } @@ -1540,3 +1539,18 @@ fn owned_to_mut() { drop(bm1); assert_eq!(drop_counter.get(), 1); } + +#[test] +fn owned_to_vec() { + let drop_counter = Rc::new(Cell::new(0)); + let owner = OwnedTester(drop_counter.clone()); + let b1 = unsafe { Bytes::with_owner(LONG.as_ptr(), LONG.len(), owner) }; + assert!(b1.is_unique()); + + let v1 = b1.to_vec(); + assert!(b1.is_unique()); + assert_eq!(&v1[..], &b1[..]); + + drop(b1); + assert_eq!(drop_counter.get(), 1); +} From 74f138bc9aed23b426ab66bfb27e4007ebd77385 Mon Sep 17 00:00:00 2001 From: Adam Cimarosti Date: Wed, 23 Oct 2024 13:28:00 +0100 Subject: [PATCH 10/28] fixed a doctest --- src/bytes.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bytes.rs b/src/bytes.rs index bf9b9f44b..4e8792ab2 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -206,7 +206,7 @@ impl Bytes { /// /// A common use case is to zero-copy construct from mapped memory. /// - /// ```no_run + /// ```ignore /// use bytes::Bytes; /// use std::fs::File; /// use memmap2::Map; From d3b6798d764d9619b947e7a0940f7375841c79c9 Mon Sep 17 00:00:00 2001 From: Adam Cimarosti Date: Wed, 23 Oct 2024 15:07:34 +0100 Subject: [PATCH 11/28] new safe API (thanks to @hniksic and @mtopolnik for the suggestions!) --- src/bytes.rs | 29 +++++++++++++++-------------- tests/test_bytes.rs | 42 +++++++++++++++++++++++++++++++----------- 2 files changed, 46 insertions(+), 25 deletions(-) diff --git a/src/bytes.rs b/src/bytes.rs index 4e8792ab2..26d712ec8 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -212,25 +212,18 @@ impl Bytes { /// use memmap2::Map; /// /// let file = File::open("upload_bundle.tar.gz")?; - /// let b = unsafe { - /// let mmap = Mmap::map(&file)?; - /// let ptr = mmap.as_ptr(); - /// let len = mmap.len(); - /// Bytes::with_owner(ptr, len, mmap) - /// }; + /// let mmap = unsafe { Mmap::map(&file) }?; + /// let b = Bytes::from_owner(mmap); /// ``` /// - /// The function is marked `unsafe` because it requires the caller to ensure - /// that the memory region specified by `ptr` and `len` remains valid and - /// linked to the lifetime of the provided owner. - /// The `owner` will be transferred to the constructed [Bytes] object which + /// The `owner` will be transferred to the constructed [Bytes] object, which /// will ensure it is dropped once all remaining clones of the constructed /// object are dropped. The owner will then be responsible for dropping the /// specified region of memory as part of its [Drop] implementation. /// - /// Note that converting [Bytes] with an owner into a [BytesMut] will - /// always create a deep copy of the buffer into newly allocated memory. - pub unsafe fn with_owner(ptr: *const u8, len: usize, owner: T) -> Self { + /// Note that converting [Bytes] constructed from an owner into a [BytesMut] + /// will always create a deep copy of the buffer into newly allocated memory. + pub fn from_owner>(owner: T) -> Self { let owned = Box::into_raw(Box::new(Owned { lifetime: OwnedLifetime { ref_cnt: AtomicUsize::new(1), @@ -239,6 +232,14 @@ impl Bytes { owner, })); + // Now that the ownership is moved to the Box its memory location is pinned. + // It's therefore safe to access the memory region, which will remain valid, + // even if the slice returned refers to memory within the owner object itself. + let owned_ref = unsafe { &(*owned) }; + let buf = owned_ref.owner.as_ref(); + let ptr = buf.as_ptr(); + let len = buf.len(); + Bytes { ptr, len, @@ -584,7 +585,7 @@ impl Bytes { /// and return self. /// /// This will also always fail if the buffer was constructed via - /// [with_owner](Bytes::with_owner). + /// [from_owner](Bytes::from_owner). /// /// # Examples /// diff --git a/tests/test_bytes.rs b/tests/test_bytes.rs index 08903e204..f142914ba 100644 --- a/tests/test_bytes.rs +++ b/tests/test_bytes.rs @@ -1486,20 +1486,37 @@ fn split_to_empty_addr_mut() { // since this forces the type to be !Send + !Sync, ensuring // in this test that the owner remains a generic T. #[derive(Clone)] -struct OwnedTester(Rc>); +struct OwnedTester { + buf: [u8; L], + drop_count: Rc>, +} + +impl OwnedTester { + fn new(buf: [u8; L], drop_count: Rc>) -> Self { + Self { buf, drop_count } + } +} + +impl AsRef<[u8]> for OwnedTester { + fn as_ref(&self) -> &[u8] { + self.buf.as_slice() + } +} -impl Drop for OwnedTester { +impl Drop for OwnedTester { fn drop(&mut self) { - let current = self.0.get(); - self.0.set(current + 1) + let current = self.drop_count.get(); + self.drop_count.set(current + 1) } } #[test] fn owned_basic() { + let buf: [u8; 5] = [1, 2, 3, 4, 5]; let drop_counter = Rc::new(Cell::new(0)); - let owner = OwnedTester(drop_counter.clone()); - let b1 = unsafe { Bytes::with_owner(SHORT.as_ptr(), SHORT.len(), owner) }; + let owner = OwnedTester::new(buf, drop_counter.clone()); + let b1 = Bytes::from_owner(owner); + assert_eq!(&buf[..], &b1[..]); assert!(b1.is_unique()); let b2 = b1.clone(); assert!(!b1.is_unique()); @@ -1520,9 +1537,10 @@ fn owned_basic() { #[test] fn owned_to_mut() { + let buf: [u8; 10] = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]; let drop_counter = Rc::new(Cell::new(0)); - let owner = OwnedTester(drop_counter.clone()); - let b1 = unsafe { Bytes::with_owner(SHORT.as_ptr(), SHORT.len(), owner) }; + let owner = OwnedTester::new(buf, drop_counter.clone()); + let b1 = Bytes::from_owner(owner); // Holding an owner will fail converting to a BytesMut, // even when the bytes instance is unique. @@ -1533,7 +1551,7 @@ fn owned_to_mut() { // That said, it's still possible, just not cheap. let bm1: BytesMut = b1.into(); let new_buf = &bm1[..]; - assert_eq!(new_buf, SHORT); + assert_eq!(new_buf, &buf[..]); assert_eq!(drop_counter.get(), 1); drop(bm1); @@ -1542,13 +1560,15 @@ fn owned_to_mut() { #[test] fn owned_to_vec() { + let buf: [u8; 10] = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]; let drop_counter = Rc::new(Cell::new(0)); - let owner = OwnedTester(drop_counter.clone()); - let b1 = unsafe { Bytes::with_owner(LONG.as_ptr(), LONG.len(), owner) }; + let owner = OwnedTester::new(buf, drop_counter.clone()); + let b1 = Bytes::from_owner(owner); assert!(b1.is_unique()); let v1 = b1.to_vec(); assert!(b1.is_unique()); + assert_eq!(&v1[..], &buf[..]); assert_eq!(&v1[..], &b1[..]); drop(b1); From 8f1ddeb8248fed4a5f62cd66869c882d3b46357d Mon Sep 17 00:00:00 2001 From: Adam Cimarosti Date: Wed, 23 Oct 2024 16:38:53 +0100 Subject: [PATCH 12/28] fixed ref count atomic ordering --- src/bytes.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bytes.rs b/src/bytes.rs index 26d712ec8..8e11ca12e 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -1126,7 +1126,7 @@ unsafe fn owned_drop_impl(owned: *mut ()) { let lifetime = owned.cast::(); let ref_cnt = &(*lifetime).ref_cnt; - let old_cnt = ref_cnt.fetch_sub(1, Ordering::Release); + let old_cnt = ref_cnt.fetch_sub(1, Ordering::Acquire); if old_cnt != 1 { return; } From 1d7ef8565de71a50bbe6e3def3cfae41ccba9ae1 Mon Sep 17 00:00:00 2001 From: Adam Cimarosti Date: Wed, 23 Oct 2024 22:48:36 +0100 Subject: [PATCH 13/28] fixed panic safety issue --- src/bytes.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/bytes.rs b/src/bytes.rs index 8e11ca12e..30641e04f 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -224,26 +224,25 @@ impl Bytes { /// Note that converting [Bytes] constructed from an owner into a [BytesMut] /// will always create a deep copy of the buffer into newly allocated memory. pub fn from_owner>(owner: T) -> Self { - let owned = Box::into_raw(Box::new(Owned { + let owned = Box::new(Owned { lifetime: OwnedLifetime { ref_cnt: AtomicUsize::new(1), drop: owned_box_and_drop::, }, owner, - })); + }); // Now that the ownership is moved to the Box its memory location is pinned. // It's therefore safe to access the memory region, which will remain valid, // even if the slice returned refers to memory within the owner object itself. - let owned_ref = unsafe { &(*owned) }; - let buf = owned_ref.owner.as_ref(); + let buf = owned.owner.as_ref(); let ptr = buf.as_ptr(); let len = buf.len(); Bytes { ptr, len, - data: AtomicPtr::new(owned as _), + data: AtomicPtr::new(Box::into_raw(owned) as _), vtable: &OWNED_VTABLE, } } From 6271eff2d3cb5984c66b5137b49701ce0a0df3ec Mon Sep 17 00:00:00 2001 From: Adam Cimarosti Date: Wed, 23 Oct 2024 23:03:22 +0100 Subject: [PATCH 14/28] enforced owner to be Send + 'static --- src/bytes.rs | 5 ++++- tests/test_bytes.rs | 36 ++++++++++++++++-------------------- 2 files changed, 20 insertions(+), 21 deletions(-) diff --git a/src/bytes.rs b/src/bytes.rs index 30641e04f..2762289b5 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -223,7 +223,10 @@ impl Bytes { /// /// Note that converting [Bytes] constructed from an owner into a [BytesMut] /// will always create a deep copy of the buffer into newly allocated memory. - pub fn from_owner>(owner: T) -> Self { + pub fn from_owner(owner: T) -> Self + where + T: AsRef<[u8]> + Send + 'static, + { let owned = Box::new(Owned { lifetime: OwnedLifetime { ref_cnt: AtomicUsize::new(1), diff --git a/tests/test_bytes.rs b/tests/test_bytes.rs index f142914ba..b86faf05c 100644 --- a/tests/test_bytes.rs +++ b/tests/test_bytes.rs @@ -1,8 +1,8 @@ #![warn(rust_2018_idioms)] use bytes::{Buf, BufMut, Bytes, BytesMut}; -use std::cell::Cell; -use std::rc::Rc; +use std::sync::atomic::{AtomicUsize, Ordering}; +use std::sync::Arc; use std::usize; @@ -1482,17 +1482,14 @@ fn split_to_empty_addr_mut() { let _ = &buf[..]; } -// N.B.: Using `Rc` and `Cell` rather than `Arc` and `usize` -// since this forces the type to be !Send + !Sync, ensuring -// in this test that the owner remains a generic T. #[derive(Clone)] struct OwnedTester { buf: [u8; L], - drop_count: Rc>, + drop_count: Arc, } impl OwnedTester { - fn new(buf: [u8; L], drop_count: Rc>) -> Self { + fn new(buf: [u8; L], drop_count: Arc) -> Self { Self { buf, drop_count } } } @@ -1505,15 +1502,14 @@ impl AsRef<[u8]> for OwnedTester { impl Drop for OwnedTester { fn drop(&mut self) { - let current = self.drop_count.get(); - self.drop_count.set(current + 1) + self.drop_count.fetch_add(1, Ordering::AcqRel); } } #[test] fn owned_basic() { let buf: [u8; 5] = [1, 2, 3, 4, 5]; - let drop_counter = Rc::new(Cell::new(0)); + let drop_counter = Arc::new(AtomicUsize::new(0)); let owner = OwnedTester::new(buf, drop_counter.clone()); let b1 = Bytes::from_owner(owner); assert_eq!(&buf[..], &b1[..]); @@ -1521,24 +1517,24 @@ fn owned_basic() { let b2 = b1.clone(); assert!(!b1.is_unique()); assert!(!b2.is_unique()); - assert_eq!(drop_counter.get(), 0); + assert_eq!(drop_counter.load(Ordering::Acquire), 0); drop(b1); - assert_eq!(drop_counter.get(), 0); + assert_eq!(drop_counter.load(Ordering::Acquire), 0); assert!(b2.is_unique()); let b3 = b2.slice(1..b2.len() - 1); assert!(!b2.is_unique()); assert!(!b3.is_unique()); drop(b2); - assert_eq!(drop_counter.get(), 0); + assert_eq!(drop_counter.load(Ordering::Acquire), 0); assert!(b3.is_unique()); drop(b3); - assert_eq!(drop_counter.get(), 1); + assert_eq!(drop_counter.load(Ordering::Acquire), 1); } #[test] fn owned_to_mut() { let buf: [u8; 10] = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]; - let drop_counter = Rc::new(Cell::new(0)); + let drop_counter = Arc::new(AtomicUsize::new(0)); let owner = OwnedTester::new(buf, drop_counter.clone()); let b1 = Bytes::from_owner(owner); @@ -1546,22 +1542,22 @@ fn owned_to_mut() { // even when the bytes instance is unique. assert!(b1.is_unique()); let b1 = b1.try_into_mut().unwrap_err(); - assert_eq!(drop_counter.get(), 0); + assert_eq!(drop_counter.load(Ordering::Acquire), 0); // That said, it's still possible, just not cheap. let bm1: BytesMut = b1.into(); let new_buf = &bm1[..]; assert_eq!(new_buf, &buf[..]); - assert_eq!(drop_counter.get(), 1); + assert_eq!(drop_counter.load(Ordering::Acquire), 1); drop(bm1); - assert_eq!(drop_counter.get(), 1); + assert_eq!(drop_counter.load(Ordering::Acquire), 1); } #[test] fn owned_to_vec() { let buf: [u8; 10] = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]; - let drop_counter = Rc::new(Cell::new(0)); + let drop_counter = Arc::new(AtomicUsize::new(0)); let owner = OwnedTester::new(buf, drop_counter.clone()); let b1 = Bytes::from_owner(owner); assert!(b1.is_unique()); @@ -1572,5 +1568,5 @@ fn owned_to_vec() { assert_eq!(&v1[..], &b1[..]); drop(b1); - assert_eq!(drop_counter.get(), 1); + assert_eq!(drop_counter.load(Ordering::Acquire), 1); } From 8e881a890cb706972e6699c830ad9bdf3567d679 Mon Sep 17 00:00:00 2001 From: Adam Cimarosti Date: Thu, 24 Oct 2024 09:17:33 +0100 Subject: [PATCH 15/28] taking @Darksonn's suggestion to resolve Miri issue --- src/bytes.rs | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/bytes.rs b/src/bytes.rs index 2762289b5..44212be30 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -1,6 +1,7 @@ use core::iter::FromIterator; use core::mem::{self, ManuallyDrop}; use core::ops::{Deref, RangeBounds}; +use core::ptr::NonNull; use core::{cmp, fmt, hash, ptr, slice, usize}; use alloc::{ @@ -227,27 +228,26 @@ impl Bytes { where T: AsRef<[u8]> + Send + 'static, { - let owned = Box::new(Owned { + let owned = Box::into_raw(Box::new(Owned { lifetime: OwnedLifetime { ref_cnt: AtomicUsize::new(1), drop: owned_box_and_drop::, }, owner, - }); - - // Now that the ownership is moved to the Box its memory location is pinned. - // It's therefore safe to access the memory region, which will remain valid, - // even if the slice returned refers to memory within the owner object itself. - let buf = owned.owner.as_ref(); - let ptr = buf.as_ptr(); - let len = buf.len(); + })); - Bytes { - ptr, - len, - data: AtomicPtr::new(Box::into_raw(owned) as _), + let mut ret = Bytes { + ptr: NonNull::dangling().as_ptr(), + len: 0, + data: AtomicPtr::new(owned.cast()), vtable: &OWNED_VTABLE, - } + }; + + let buf = unsafe { &*owned }.owner.as_ref(); + ret.ptr = buf.as_ptr(); + ret.len = buf.len(); + + ret } /// Returns the number of bytes contained in this `Bytes`. From d1aed1d90a8a4b7aea4b2183aa6b198bb2e61c61 Mon Sep 17 00:00:00 2001 From: Adam Cimarosti Date: Thu, 24 Oct 2024 10:23:28 +0100 Subject: [PATCH 16/28] Added comments on safety and Miri --- src/bytes.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/bytes.rs b/src/bytes.rs index 44212be30..c701e1bb1 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -228,6 +228,21 @@ impl Bytes { where T: AsRef<[u8]> + Send + 'static, { + // Safety & Miri: + // The ownership of `owner` is first transferred to the `Owned` wrapper and `Bytes` object. + // This ensures that the owner is pinned in memory, allowing us to call `.as_ref()` safely + // since the lifetime of the owner is controlled by the lifetime of the new `Bytes` object, + // and the lifetime of the resulting borrowed `&[u8]` matches that of the owner. + // Note that this remains safe so long as we only call `.as_ref()` once. + // + // There are some additional special considerations here: + // * We rely on Bytes's Drop impl to clean up memory should `.as_ref()` panic. + // * Setting the `ptr` and `len` on the bytes object last (after moving the owner to + // Bytes) allows Miri checks to pass since it avoids obtaining the `&[u8]` slice + // from a stack-owned Box. + // More details on this: https://github.com/tokio-rs/bytes/pull/742/#discussion_r1813375863 + // and: https://github.com/tokio-rs/bytes/pull/742/#discussion_r1813316032 + let owned = Box::into_raw(Box::new(Owned { lifetime: OwnedLifetime { ref_cnt: AtomicUsize::new(1), From ddcb7ce1fe971b46d56c9ff629ee752afb034776 Mon Sep 17 00:00:00 2001 From: Adam Cimarosti Date: Thu, 24 Oct 2024 10:44:55 +0100 Subject: [PATCH 17/28] testing drop on owner as_ref panic --- tests/test_bytes.rs | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/tests/test_bytes.rs b/tests/test_bytes.rs index b86faf05c..849e06b47 100644 --- a/tests/test_bytes.rs +++ b/tests/test_bytes.rs @@ -4,6 +4,7 @@ use bytes::{Buf, BufMut, Bytes, BytesMut}; use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::Arc; +use std::panic::{self, AssertUnwindSafe}; use std::usize; const LONG: &[u8] = b"mary had a little lamb, little lamb, little lamb"; @@ -1486,16 +1487,24 @@ fn split_to_empty_addr_mut() { struct OwnedTester { buf: [u8; L], drop_count: Arc, + pub panic_as_ref: bool, } impl OwnedTester { fn new(buf: [u8; L], drop_count: Arc) -> Self { - Self { buf, drop_count } + Self { + buf, + drop_count, + panic_as_ref: false, + } } } impl AsRef<[u8]> for OwnedTester { fn as_ref(&self) -> &[u8] { + if self.panic_as_ref { + panic!("test-triggered panic in `AsRef<[u8]> for OwnedTester`"); + } self.buf.as_slice() } } @@ -1570,3 +1579,18 @@ fn owned_to_vec() { drop(b1); assert_eq!(drop_counter.load(Ordering::Acquire), 1); } + +#[test] +fn owned_safe_drop_on_as_ref_panic() { + let buf: [u8; 10] = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]; + let drop_counter = Arc::new(AtomicUsize::new(0)); + let mut owner = OwnedTester::new(buf, drop_counter.clone()); + owner.panic_as_ref = true; + + let result = panic::catch_unwind(AssertUnwindSafe(|| { + let _ = Bytes::from_owner(owner); + })); + + assert!(result.is_err()); + assert_eq!(drop_counter.load(Ordering::Acquire), 1); +} From d6b35aec0113f23f0a7878ed56e5f22de945bab4 Mon Sep 17 00:00:00 2001 From: Adam Cimarosti Date: Thu, 24 Oct 2024 14:53:22 +0100 Subject: [PATCH 18/28] test case clean-up --- tests/test_bytes.rs | 46 ++++++++++++++++++++++++++++++++++----------- 1 file changed, 35 insertions(+), 11 deletions(-) diff --git a/tests/test_bytes.rs b/tests/test_bytes.rs index 849e06b47..c9eeb0b04 100644 --- a/tests/test_bytes.rs +++ b/tests/test_bytes.rs @@ -1516,23 +1516,49 @@ impl Drop for OwnedTester { } #[test] -fn owned_basic() { +fn owned_is_unique() { + let b1 = Bytes::from_owner([1, 2, 3, 4, 5, 6, 7]); + assert!(b1.is_unique()); + let b2 = b1.clone(); + assert!(!b1.is_unique()); + assert!(!b2.is_unique()); + drop(b1); + assert!(b2.is_unique()); +} + +#[test] +fn owned_buf_sharing() { + let buf = [1, 2, 3, 4, 5, 6, 7]; + let b1 = Bytes::from_owner(buf); + let b2 = b1.clone(); + assert_eq!(&buf[..], &b1[..]); + assert_eq!(&buf[..], &b2[..]); + assert_eq!(b1.as_ptr(), b2.as_ptr()); + assert_eq!(b1.len(), b2.len()); + assert_eq!(b1.len(), buf.len()); +} + +#[test] +fn owned_buf_slicing() { + let b1 = Bytes::from_owner(SHORT); + assert_eq!(SHORT, &b1[..]); + let b2 = b1.slice(1..(b1.len() - 1)); + assert_eq!(&SHORT[1..(SHORT.len() - 1)], b2); + assert_eq!(unsafe { SHORT.as_ptr().add(1) }, b2.as_ptr()); + assert_eq!(SHORT.len() - 2, b2.len()); +} + +#[test] +fn owned_dropped_exactly_once() { let buf: [u8; 5] = [1, 2, 3, 4, 5]; let drop_counter = Arc::new(AtomicUsize::new(0)); let owner = OwnedTester::new(buf, drop_counter.clone()); let b1 = Bytes::from_owner(owner); - assert_eq!(&buf[..], &b1[..]); - assert!(b1.is_unique()); let b2 = b1.clone(); - assert!(!b1.is_unique()); - assert!(!b2.is_unique()); assert_eq!(drop_counter.load(Ordering::Acquire), 0); drop(b1); assert_eq!(drop_counter.load(Ordering::Acquire), 0); - assert!(b2.is_unique()); let b3 = b2.slice(1..b2.len() - 1); - assert!(!b2.is_unique()); - assert!(!b3.is_unique()); drop(b2); assert_eq!(drop_counter.load(Ordering::Acquire), 0); assert!(b3.is_unique()); @@ -1551,15 +1577,13 @@ fn owned_to_mut() { // even when the bytes instance is unique. assert!(b1.is_unique()); let b1 = b1.try_into_mut().unwrap_err(); - assert_eq!(drop_counter.load(Ordering::Acquire), 0); // That said, it's still possible, just not cheap. let bm1: BytesMut = b1.into(); let new_buf = &bm1[..]; assert_eq!(new_buf, &buf[..]); - assert_eq!(drop_counter.load(Ordering::Acquire), 1); - drop(bm1); + // `.into::()` has correctly dropped the owner assert_eq!(drop_counter.load(Ordering::Acquire), 1); } From d170ece58c5286a7f8f6235c9c3da6cb2559e24d Mon Sep 17 00:00:00 2001 From: Adam Cimarosti Date: Mon, 28 Oct 2024 12:44:33 +0000 Subject: [PATCH 19/28] doctest improvement --- src/bytes.rs | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/src/bytes.rs b/src/bytes.rs index c701e1bb1..25cd49c67 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -207,14 +207,39 @@ impl Bytes { /// /// A common use case is to zero-copy construct from mapped memory. /// - /// ```ignore + /// ``` + /// # struct File; + /// # + /// # impl File { + /// # pub fn open(_: &str) -> Result { + /// # Ok(Self) + /// # } + /// # } + /// # + /// # mod memmap2 { + /// # pub struct Mmap; + /// # + /// # impl Mmap { + /// # pub unsafe fn map(_file: &super::File) -> Result { + /// # Ok(Self) + /// # } + /// # } + /// # + /// # impl AsRef<[u8]> for Mmap { + /// # fn as_ref(&self) -> &[u8] { + /// # b"buf" + /// # } + /// # } + /// # } /// use bytes::Bytes; - /// use std::fs::File; - /// use memmap2::Map; + /// use memmap2::Mmap; /// + /// # fn main() -> Result<(), ()> { /// let file = File::open("upload_bundle.tar.gz")?; /// let mmap = unsafe { Mmap::map(&file) }?; /// let b = Bytes::from_owner(mmap); + /// # Ok(()) + /// # } /// ``` /// /// The `owner` will be transferred to the constructed [Bytes] object, which From 01b9792a5e469300b5c0e915916b2a9bf6648d7f Mon Sep 17 00:00:00 2001 From: Adam Cimarosti Date: Mon, 28 Oct 2024 12:51:58 +0000 Subject: [PATCH 20/28] atomic ordering fix Co-authored-by: Alice Ryhl --- src/bytes.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bytes.rs b/src/bytes.rs index 25cd49c67..3d7f28001 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -1178,7 +1178,7 @@ unsafe fn owned_drop_impl(owned: *mut ()) { } unsafe fn owned_drop(data: &mut AtomicPtr<()>, _ptr: *const u8, _len: usize) { - let owned = data.load(Ordering::Acquire); + let owned = data.load(Ordering::Relaxed); owned_drop_impl(owned); } From 9b4e1bce23c073833e638be18d795d9fc3c54830 Mon Sep 17 00:00:00 2001 From: Adam Cimarosti Date: Mon, 28 Oct 2024 12:53:00 +0000 Subject: [PATCH 21/28] atomic ordering fix Co-authored-by: Alice Ryhl --- src/bytes.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bytes.rs b/src/bytes.rs index 3d7f28001..fbbd64560 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -1126,7 +1126,7 @@ unsafe fn owned_box_and_drop(ptr: *mut ()) { } unsafe fn owned_clone(data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> Bytes { - let owned = data.load(Ordering::Acquire); + let owned = data.load(Ordering::Relaxed); let ref_cnt = &(*owned.cast::()).ref_cnt; let old_cnt = ref_cnt.fetch_add(1, Ordering::Relaxed); if old_cnt > usize::MAX >> 1 { From 348b8226bf83c2111954f2e70f4fd39918df33a6 Mon Sep 17 00:00:00 2001 From: Adam Cimarosti Date: Mon, 28 Oct 2024 12:53:26 +0000 Subject: [PATCH 22/28] atomic ordering fix Co-authored-by: Alice Ryhl --- src/bytes.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bytes.rs b/src/bytes.rs index fbbd64560..169a0cc94 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -1148,7 +1148,7 @@ unsafe fn owned_to_vec(_data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> Vec unsafe fn owned_to_mut(data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> BytesMut { let bytes_mut = BytesMut::from_vec(owned_to_vec(data, ptr, len)); - owned_drop_impl(data.load(Ordering::Acquire)); + owned_drop_impl(data.load(Ordering::Relaxed)); bytes_mut } From 223d621a8ce669d41cc6900c6b81998a6478290e Mon Sep 17 00:00:00 2001 From: Adam Cimarosti Date: Mon, 28 Oct 2024 12:53:43 +0000 Subject: [PATCH 23/28] atomic ordering fix Co-authored-by: Alice Ryhl --- src/bytes.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bytes.rs b/src/bytes.rs index 169a0cc94..fd86fd4a5 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -1153,7 +1153,7 @@ unsafe fn owned_to_mut(data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> Byte } unsafe fn owned_is_unique(data: &AtomicPtr<()>) -> bool { - let owned = data.load(Ordering::Acquire); + let owned = data.load(Ordering::Relaxed); let ref_cnt = &(*owned.cast::()).ref_cnt; ref_cnt.load(Ordering::Relaxed) == 1 } From 63d25897d593e1592100d8e0706724e39d282482 Mon Sep 17 00:00:00 2001 From: Adam Cimarosti Date: Mon, 28 Oct 2024 12:54:35 +0000 Subject: [PATCH 24/28] atomic ordering improvement Co-authored-by: Alice Ryhl --- src/bytes.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bytes.rs b/src/bytes.rs index fd86fd4a5..186ba4126 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -1155,7 +1155,7 @@ unsafe fn owned_to_mut(data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> Byte unsafe fn owned_is_unique(data: &AtomicPtr<()>) -> bool { let owned = data.load(Ordering::Relaxed); let ref_cnt = &(*owned.cast::()).ref_cnt; - ref_cnt.load(Ordering::Relaxed) == 1 + ref_cnt.load(Ordering::Acquire) == 1 } unsafe fn owned_cheap_into_mut(_data: &AtomicPtr<()>) -> bool { From 020a5858d8adc5023cfefb8a436eaa65bdbbc80f Mon Sep 17 00:00:00 2001 From: Adam Cimarosti Date: Mon, 28 Oct 2024 12:55:10 +0000 Subject: [PATCH 25/28] naming clarity Co-authored-by: Alice Ryhl --- src/bytes.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/bytes.rs b/src/bytes.rs index 186ba4126..dcb961981 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -1173,8 +1173,8 @@ unsafe fn owned_drop_impl(owned: *mut ()) { return; } - let drop = &(*lifetime).drop; - drop(owned) + let drop_fn = &(*lifetime).drop; + drop_fn(owned) } unsafe fn owned_drop(data: &mut AtomicPtr<()>, _ptr: *const u8, _len: usize) { From e10cfb68d6b57eb336073d35fd8a00cf9e1d6399 Mon Sep 17 00:00:00 2001 From: Adam Cimarosti Date: Mon, 28 Oct 2024 12:58:29 +0000 Subject: [PATCH 26/28] atomic ordering fix Co-authored-by: Alice Ryhl --- src/bytes.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/bytes.rs b/src/bytes.rs index dcb961981..95fe96ff9 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -1168,10 +1168,11 @@ unsafe fn owned_drop_impl(owned: *mut ()) { let lifetime = owned.cast::(); let ref_cnt = &(*lifetime).ref_cnt; - let old_cnt = ref_cnt.fetch_sub(1, Ordering::Acquire); + let old_cnt = ref_cnt.fetch_sub(1, Ordering::Release); if old_cnt != 1 { return; } + ref_cnt.load(Ordering::Acquire); let drop_fn = &(*lifetime).drop; drop_fn(owned) From 01730616752fee5287b26bad7bd3a29695b82588 Mon Sep 17 00:00:00 2001 From: Adam Cimarosti Date: Mon, 28 Oct 2024 13:07:20 +0000 Subject: [PATCH 27/28] more readable tests --- tests/test_bytes.rs | 45 +++++++++++++++++++++++++++++++-------------- 1 file changed, 31 insertions(+), 14 deletions(-) diff --git a/tests/test_bytes.rs b/tests/test_bytes.rs index c9eeb0b04..53c923884 100644 --- a/tests/test_bytes.rs +++ b/tests/test_bytes.rs @@ -1483,15 +1483,32 @@ fn split_to_empty_addr_mut() { let _ = &buf[..]; } +#[derive(Clone)] +struct SharedAtomicCounter(Arc); + +impl SharedAtomicCounter { + pub fn new() -> Self { + SharedAtomicCounter(Arc::new(AtomicUsize::new(0))) + } + + pub fn increment(&self) { + self.0.fetch_add(1, Ordering::AcqRel); + } + + pub fn get(&self) -> usize { + self.0.load(Ordering::Acquire) + } +} + #[derive(Clone)] struct OwnedTester { buf: [u8; L], - drop_count: Arc, + drop_count: SharedAtomicCounter, pub panic_as_ref: bool, } impl OwnedTester { - fn new(buf: [u8; L], drop_count: Arc) -> Self { + fn new(buf: [u8; L], drop_count: SharedAtomicCounter) -> Self { Self { buf, drop_count, @@ -1511,7 +1528,7 @@ impl AsRef<[u8]> for OwnedTester { impl Drop for OwnedTester { fn drop(&mut self) { - self.drop_count.fetch_add(1, Ordering::AcqRel); + self.drop_count.increment(); } } @@ -1551,25 +1568,25 @@ fn owned_buf_slicing() { #[test] fn owned_dropped_exactly_once() { let buf: [u8; 5] = [1, 2, 3, 4, 5]; - let drop_counter = Arc::new(AtomicUsize::new(0)); + let drop_counter = SharedAtomicCounter::new(); let owner = OwnedTester::new(buf, drop_counter.clone()); let b1 = Bytes::from_owner(owner); let b2 = b1.clone(); - assert_eq!(drop_counter.load(Ordering::Acquire), 0); + assert_eq!(drop_counter.get(), 0); drop(b1); - assert_eq!(drop_counter.load(Ordering::Acquire), 0); + assert_eq!(drop_counter.get(), 0); let b3 = b2.slice(1..b2.len() - 1); drop(b2); - assert_eq!(drop_counter.load(Ordering::Acquire), 0); + assert_eq!(drop_counter.get(), 0); assert!(b3.is_unique()); drop(b3); - assert_eq!(drop_counter.load(Ordering::Acquire), 1); + assert_eq!(drop_counter.get(), 1); } #[test] fn owned_to_mut() { let buf: [u8; 10] = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]; - let drop_counter = Arc::new(AtomicUsize::new(0)); + let drop_counter = SharedAtomicCounter::new(); let owner = OwnedTester::new(buf, drop_counter.clone()); let b1 = Bytes::from_owner(owner); @@ -1584,13 +1601,13 @@ fn owned_to_mut() { assert_eq!(new_buf, &buf[..]); // `.into::()` has correctly dropped the owner - assert_eq!(drop_counter.load(Ordering::Acquire), 1); + assert_eq!(drop_counter.get(), 1); } #[test] fn owned_to_vec() { let buf: [u8; 10] = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]; - let drop_counter = Arc::new(AtomicUsize::new(0)); + let drop_counter = SharedAtomicCounter::new(); let owner = OwnedTester::new(buf, drop_counter.clone()); let b1 = Bytes::from_owner(owner); assert!(b1.is_unique()); @@ -1601,13 +1618,13 @@ fn owned_to_vec() { assert_eq!(&v1[..], &b1[..]); drop(b1); - assert_eq!(drop_counter.load(Ordering::Acquire), 1); + assert_eq!(drop_counter.get(), 1); } #[test] fn owned_safe_drop_on_as_ref_panic() { let buf: [u8; 10] = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]; - let drop_counter = Arc::new(AtomicUsize::new(0)); + let drop_counter = SharedAtomicCounter::new(); let mut owner = OwnedTester::new(buf, drop_counter.clone()); owner.panic_as_ref = true; @@ -1616,5 +1633,5 @@ fn owned_safe_drop_on_as_ref_panic() { })); assert!(result.is_err()); - assert_eq!(drop_counter.load(Ordering::Acquire), 1); + assert_eq!(drop_counter.get(), 1); } From 12de70803b5d695fab4ac2455d01316dd329fb44 Mon Sep 17 00:00:00 2001 From: Adam Cimarosti Date: Mon, 28 Oct 2024 15:59:05 +0000 Subject: [PATCH 28/28] always false from owned is_unique --- src/bytes.rs | 30 +++++++++--------------------- src/bytes_mut.rs | 1 - tests/test_bytes.rs | 12 ++++-------- 3 files changed, 13 insertions(+), 30 deletions(-) diff --git a/src/bytes.rs b/src/bytes.rs index 95fe96ff9..96f834f43 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -117,7 +117,6 @@ pub(crate) struct Vtable { pub to_mut: unsafe fn(&AtomicPtr<()>, *const u8, usize) -> BytesMut, /// fn(data) pub is_unique: unsafe fn(&AtomicPtr<()>) -> bool, - pub cheap_into_mut: unsafe fn(&AtomicPtr<()>) -> bool, /// fn(data, ptr, len) pub drop: unsafe fn(&mut AtomicPtr<()>, *const u8, usize), } @@ -320,14 +319,16 @@ impl Bytes { self.len == 0 } - /// Returns true if this is the only reference to the data. + /// Returns true if this is the only reference to the data and + /// `Into` would avoid cloning the underlying buffer. /// - /// Always returns false if the data is backed by a static slice. + /// Always returns false if the data is backed by a [static slice](Bytes::from_static), + /// or an [owner](Bytes::from_owner). /// /// The result of this method may be invalidated immediately if another /// thread clones this value while this is being called. Ensure you have /// unique access to this value (`&mut Bytes`) first if you need to be - /// certain the result is valid (i.e. for safety reasons) + /// certain the result is valid (i.e. for safety reasons). /// # Examples /// /// ``` @@ -626,8 +627,8 @@ impl Bytes { /// If `self` is not unique for the entire original buffer, this will fail /// and return self. /// - /// This will also always fail if the buffer was constructed via - /// [from_owner](Bytes::from_owner). + /// This will also always fail if the buffer was constructed via either + /// [from_owner](Bytes::from_owner) or [from_static](Bytes::from_static). /// /// # Examples /// @@ -638,7 +639,7 @@ impl Bytes { /// assert_eq!(bytes.try_into_mut(), Ok(BytesMut::from(&b"hello"[..]))); /// ``` pub fn try_into_mut(self) -> Result { - if unsafe { (self.vtable.cheap_into_mut)(&self.data) } { + if self.is_unique() { Ok(self.into()) } else { Err(self) @@ -1079,7 +1080,6 @@ const STATIC_VTABLE: Vtable = Vtable { to_vec: static_to_vec, to_mut: static_to_mut, is_unique: static_is_unique, - cheap_into_mut: static_is_unique, drop: static_drop, }; @@ -1152,15 +1152,7 @@ unsafe fn owned_to_mut(data: &AtomicPtr<()>, ptr: *const u8, len: usize) -> Byte bytes_mut } -unsafe fn owned_is_unique(data: &AtomicPtr<()>) -> bool { - let owned = data.load(Ordering::Relaxed); - let ref_cnt = &(*owned.cast::()).ref_cnt; - ref_cnt.load(Ordering::Acquire) == 1 -} - -unsafe fn owned_cheap_into_mut(_data: &AtomicPtr<()>) -> bool { - // Since the memory's ownership is tied to an external owner - // it is never zero-copy to create a BytesMut. +unsafe fn owned_is_unique(_data: &AtomicPtr<()>) -> bool { false } @@ -1188,7 +1180,6 @@ static OWNED_VTABLE: Vtable = Vtable { to_vec: owned_to_vec, to_mut: owned_to_mut, is_unique: owned_is_unique, - cheap_into_mut: owned_cheap_into_mut, drop: owned_drop, }; @@ -1199,7 +1190,6 @@ static PROMOTABLE_EVEN_VTABLE: Vtable = Vtable { to_vec: promotable_even_to_vec, to_mut: promotable_even_to_mut, is_unique: promotable_is_unique, - cheap_into_mut: promotable_is_unique, drop: promotable_even_drop, }; @@ -1208,7 +1198,6 @@ static PROMOTABLE_ODD_VTABLE: Vtable = Vtable { to_vec: promotable_odd_to_vec, to_mut: promotable_odd_to_mut, is_unique: promotable_is_unique, - cheap_into_mut: promotable_is_unique, drop: promotable_odd_drop, }; @@ -1385,7 +1374,6 @@ static SHARED_VTABLE: Vtable = Vtable { to_vec: shared_to_vec, to_mut: shared_to_mut, is_unique: shared_is_unique, - cheap_into_mut: shared_is_unique, drop: shared_drop, }; diff --git a/src/bytes_mut.rs b/src/bytes_mut.rs index 48fb3e69c..a0d77bc24 100644 --- a/src/bytes_mut.rs +++ b/src/bytes_mut.rs @@ -1776,7 +1776,6 @@ static SHARED_VTABLE: Vtable = Vtable { to_vec: shared_v_to_vec, to_mut: shared_v_to_mut, is_unique: shared_v_is_unique, - cheap_into_mut: shared_v_is_unique, drop: shared_v_drop, }; diff --git a/tests/test_bytes.rs b/tests/test_bytes.rs index 53c923884..fdc36ce8d 100644 --- a/tests/test_bytes.rs +++ b/tests/test_bytes.rs @@ -1533,14 +1533,14 @@ impl Drop for OwnedTester { } #[test] -fn owned_is_unique() { +fn owned_is_unique_always_false() { let b1 = Bytes::from_owner([1, 2, 3, 4, 5, 6, 7]); - assert!(b1.is_unique()); + assert!(!b1.is_unique()); // even if ref_cnt == 1 let b2 = b1.clone(); assert!(!b1.is_unique()); assert!(!b2.is_unique()); drop(b1); - assert!(b2.is_unique()); + assert!(!b2.is_unique()); // even if ref_cnt == 1 } #[test] @@ -1578,7 +1578,6 @@ fn owned_dropped_exactly_once() { let b3 = b2.slice(1..b2.len() - 1); drop(b2); assert_eq!(drop_counter.get(), 0); - assert!(b3.is_unique()); drop(b3); assert_eq!(drop_counter.get(), 1); } @@ -1591,8 +1590,7 @@ fn owned_to_mut() { let b1 = Bytes::from_owner(owner); // Holding an owner will fail converting to a BytesMut, - // even when the bytes instance is unique. - assert!(b1.is_unique()); + // even when the bytes instance has a ref_cnt == 1. let b1 = b1.try_into_mut().unwrap_err(); // That said, it's still possible, just not cheap. @@ -1610,10 +1608,8 @@ fn owned_to_vec() { let drop_counter = SharedAtomicCounter::new(); let owner = OwnedTester::new(buf, drop_counter.clone()); let b1 = Bytes::from_owner(owner); - assert!(b1.is_unique()); let v1 = b1.to_vec(); - assert!(b1.is_unique()); assert_eq!(&v1[..], &buf[..]); assert_eq!(&v1[..], &b1[..]);