From 0ba3b4c4cd74a0ad8566277e1a1533fa9e895756 Mon Sep 17 00:00:00 2001 From: Brad Dunbar Date: Sun, 28 Jan 2024 05:37:11 -0500 Subject: [PATCH 1/8] Remove unnecessary namespace qualifier (#660) --- src/bytes.rs | 2 +- src/bytes_mut.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/bytes.rs b/src/bytes.rs index 9eda9f463..3da747d44 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -580,7 +580,7 @@ impl Buf for Bytes { } } - fn copy_to_bytes(&mut self, len: usize) -> crate::Bytes { + fn copy_to_bytes(&mut self, len: usize) -> Self { if len == self.remaining() { core::mem::replace(self, Bytes::new()) } else { diff --git a/src/bytes_mut.rs b/src/bytes_mut.rs index 88d596cf6..1628a85a5 100644 --- a/src/bytes_mut.rs +++ b/src/bytes_mut.rs @@ -1078,7 +1078,7 @@ impl Buf for BytesMut { } } - fn copy_to_bytes(&mut self, len: usize) -> crate::Bytes { + fn copy_to_bytes(&mut self, len: usize) -> Bytes { self.split_to(len).freeze() } } @@ -1110,7 +1110,7 @@ unsafe impl BufMut for BytesMut { // Specialize these methods so they can skip checking `remaining_mut` // and `advance_mut`. - fn put(&mut self, mut src: T) + fn put(&mut self, mut src: T) where Self: Sized, { From 9257a6ea0852c03f4672e5f8346d3d614543e270 Mon Sep 17 00:00:00 2001 From: Brad Dunbar Date: Sun, 28 Jan 2024 05:50:56 -0500 Subject: [PATCH 2/8] Remove an unnecessary else branch (#662) --- src/bytes_mut.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/bytes_mut.rs b/src/bytes_mut.rs index 1628a85a5..d143f605c 100644 --- a/src/bytes_mut.rs +++ b/src/bytes_mut.rs @@ -726,11 +726,11 @@ impl BytesMut { } return; - } else { - new_cap = cmp::max(new_cap, original_capacity); } } + new_cap = cmp::max(new_cap, original_capacity); + // Create a new vector to store the data let mut v = ManuallyDrop::new(Vec::with_capacity(new_cap)); From e24587dd6197dbc58d6c2b6eb7186df99b04d881 Mon Sep 17 00:00:00 2001 From: Brad Dunbar Date: Sun, 28 Jan 2024 07:00:30 -0500 Subject: [PATCH 3/8] Remove unreachable else branch (#661) --- src/bytes_mut.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/bytes_mut.rs b/src/bytes_mut.rs index d143f605c..8783ae7f3 100644 --- a/src/bytes_mut.rs +++ b/src/bytes_mut.rs @@ -1628,7 +1628,7 @@ impl From for Vec { let (off, _) = bytes.get_vec_pos(); rebuild_vec(bytes.ptr.as_ptr(), bytes.len, bytes.cap, off) } - } else if kind == KIND_ARC { + } else { let shared = bytes.data as *mut Shared; if unsafe { (*shared).is_unique() } { @@ -1640,8 +1640,6 @@ impl From for Vec { } else { return bytes.deref().to_vec(); } - } else { - return bytes.deref().to_vec(); }; let len = bytes.len; From d2e7abdb290e663f025a22a7d9e14e019b6abdb2 Mon Sep 17 00:00:00 2001 From: Brad Dunbar Date: Wed, 31 Jan 2024 09:41:23 -0500 Subject: [PATCH 4/8] refactor: make parameter mut in From (#667) Instead of re-declaring `vec`, we can just use a mut parameter. --- src/bytes.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/bytes.rs b/src/bytes.rs index 3da747d44..0b443c85b 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -828,8 +828,7 @@ impl From<&'static str> for Bytes { } impl From> for Bytes { - fn from(vec: Vec) -> Bytes { - let mut vec = vec; + fn from(mut vec: Vec) -> Bytes { let ptr = vec.as_mut_ptr(); let len = vec.len(); let cap = vec.capacity(); From 8bcac21cb44c112f20e8dd31475033ff448e35ce Mon Sep 17 00:00:00 2001 From: Brad Dunbar Date: Tue, 6 Feb 2024 13:17:01 -0500 Subject: [PATCH 5/8] Restore commented tests (#665) These seem to have been commented by accident in #298, and are still passing. --- src/bytes_mut.rs | 79 +++++++++++++++++++++++++----------------------- 1 file changed, 41 insertions(+), 38 deletions(-) diff --git a/src/bytes_mut.rs b/src/bytes_mut.rs index 8783ae7f3..5566f2d1a 100644 --- a/src/bytes_mut.rs +++ b/src/bytes_mut.rs @@ -1409,56 +1409,59 @@ fn original_capacity_from_repr(repr: usize) -> usize { 1 << (repr + (MIN_ORIGINAL_CAPACITY_WIDTH - 1)) } -/* -#[test] -fn test_original_capacity_to_repr() { - assert_eq!(original_capacity_to_repr(0), 0); +#[cfg(test)] +mod tests { + use super::*; - let max_width = 32; + #[test] + fn test_original_capacity_to_repr() { + assert_eq!(original_capacity_to_repr(0), 0); - for width in 1..(max_width + 1) { - let cap = 1 << width - 1; + let max_width = 32; - let expected = if width < MIN_ORIGINAL_CAPACITY_WIDTH { - 0 - } else if width < MAX_ORIGINAL_CAPACITY_WIDTH { - width - MIN_ORIGINAL_CAPACITY_WIDTH - } else { - MAX_ORIGINAL_CAPACITY_WIDTH - MIN_ORIGINAL_CAPACITY_WIDTH - }; + for width in 1..(max_width + 1) { + let cap = 1 << width - 1; - assert_eq!(original_capacity_to_repr(cap), expected); + let expected = if width < MIN_ORIGINAL_CAPACITY_WIDTH { + 0 + } else if width < MAX_ORIGINAL_CAPACITY_WIDTH { + width - MIN_ORIGINAL_CAPACITY_WIDTH + } else { + MAX_ORIGINAL_CAPACITY_WIDTH - MIN_ORIGINAL_CAPACITY_WIDTH + }; - if width > 1 { - assert_eq!(original_capacity_to_repr(cap + 1), expected); - } + assert_eq!(original_capacity_to_repr(cap), expected); - // MIN_ORIGINAL_CAPACITY_WIDTH must be bigger than 7 to pass tests below - if width == MIN_ORIGINAL_CAPACITY_WIDTH + 1 { - assert_eq!(original_capacity_to_repr(cap - 24), expected - 1); - assert_eq!(original_capacity_to_repr(cap + 76), expected); - } else if width == MIN_ORIGINAL_CAPACITY_WIDTH + 2 { - assert_eq!(original_capacity_to_repr(cap - 1), expected - 1); - assert_eq!(original_capacity_to_repr(cap - 48), expected - 1); + if width > 1 { + assert_eq!(original_capacity_to_repr(cap + 1), expected); + } + + // MIN_ORIGINAL_CAPACITY_WIDTH must be bigger than 7 to pass tests below + if width == MIN_ORIGINAL_CAPACITY_WIDTH + 1 { + assert_eq!(original_capacity_to_repr(cap - 24), expected - 1); + assert_eq!(original_capacity_to_repr(cap + 76), expected); + } else if width == MIN_ORIGINAL_CAPACITY_WIDTH + 2 { + assert_eq!(original_capacity_to_repr(cap - 1), expected - 1); + assert_eq!(original_capacity_to_repr(cap - 48), expected - 1); + } } } -} -#[test] -fn test_original_capacity_from_repr() { - assert_eq!(0, original_capacity_from_repr(0)); + #[test] + fn test_original_capacity_from_repr() { + assert_eq!(0, original_capacity_from_repr(0)); - let min_cap = 1 << MIN_ORIGINAL_CAPACITY_WIDTH; + let min_cap = 1 << MIN_ORIGINAL_CAPACITY_WIDTH; - assert_eq!(min_cap, original_capacity_from_repr(1)); - assert_eq!(min_cap * 2, original_capacity_from_repr(2)); - assert_eq!(min_cap * 4, original_capacity_from_repr(3)); - assert_eq!(min_cap * 8, original_capacity_from_repr(4)); - assert_eq!(min_cap * 16, original_capacity_from_repr(5)); - assert_eq!(min_cap * 32, original_capacity_from_repr(6)); - assert_eq!(min_cap * 64, original_capacity_from_repr(7)); + assert_eq!(min_cap, original_capacity_from_repr(1)); + assert_eq!(min_cap * 2, original_capacity_from_repr(2)); + assert_eq!(min_cap * 4, original_capacity_from_repr(3)); + assert_eq!(min_cap * 8, original_capacity_from_repr(4)); + assert_eq!(min_cap * 16, original_capacity_from_repr(5)); + assert_eq!(min_cap * 32, original_capacity_from_repr(6)); + assert_eq!(min_cap * 64, original_capacity_from_repr(7)); + } } -*/ unsafe impl Send for BytesMut {} unsafe impl Sync for BytesMut {} From 47e83056f28e15e4ca68056a0136f3920b753783 Mon Sep 17 00:00:00 2001 From: Brad Dunbar Date: Tue, 6 Feb 2024 13:41:44 -0500 Subject: [PATCH 6/8] Use sub instead of offset (#668) We're always subtracting here, and we already have a usize, so `sub` seems like a more appropriate usage to me. --- src/bytes_mut.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/bytes_mut.rs b/src/bytes_mut.rs index 5566f2d1a..d1c141122 100644 --- a/src/bytes_mut.rs +++ b/src/bytes_mut.rs @@ -617,7 +617,7 @@ impl BytesMut { // // Just move the pointer back to the start after copying // data back. - let base_ptr = self.ptr.as_ptr().offset(-(off as isize)); + let base_ptr = self.ptr.as_ptr().sub(off); // Since `off >= self.len()`, the two regions don't overlap. ptr::copy_nonoverlapping(self.ptr.as_ptr(), base_ptr, self.len); self.ptr = vptr(base_ptr); @@ -1697,7 +1697,7 @@ fn offset_from(dst: *mut u8, original: *mut u8) -> usize { } unsafe fn rebuild_vec(ptr: *mut u8, mut len: usize, mut cap: usize, off: usize) -> Vec { - let ptr = ptr.offset(-(off as isize)); + let ptr = ptr.sub(off); len += off; cap += off; From c6972d61328be113ec8e80c207815a4b84fe616c Mon Sep 17 00:00:00 2001 From: Brad Dunbar Date: Tue, 6 Feb 2024 13:46:49 -0500 Subject: [PATCH 7/8] Calculate original capacity only if necessary (#666) We don't need the original capacity if the shared data is unique, so let's not calculate it until after that check. --- src/bytes_mut.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/bytes_mut.rs b/src/bytes_mut.rs index d1c141122..619defc21 100644 --- a/src/bytes_mut.rs +++ b/src/bytes_mut.rs @@ -652,13 +652,7 @@ impl BytesMut { // Compute the new capacity let mut new_cap = len.checked_add(additional).expect("overflow"); - let original_capacity; - let original_capacity_repr; - unsafe { - original_capacity_repr = (*shared).original_capacity_repr; - original_capacity = original_capacity_from_repr(original_capacity_repr); - // First, try to reclaim the buffer. This is possible if the current // handle is the only outstanding handle pointing to the buffer. if (*shared).is_unique() { @@ -729,6 +723,9 @@ impl BytesMut { } } + let original_capacity_repr = unsafe { (*shared).original_capacity_repr }; + let original_capacity = original_capacity_from_repr(original_capacity_repr); + new_cap = cmp::max(new_cap, original_capacity); // Create a new vector to store the data From f586ffc52589f01be1b4a44d6544b3d0226773d6 Mon Sep 17 00:00:00 2001 From: Brad Dunbar Date: Tue, 6 Feb 2024 14:03:37 -0500 Subject: [PATCH 8/8] set_vec_pos does not need a second parameter (#672) The second argument to `set_vec_pos` always contains the value of `self.data`. Let's just use `self.data` and remove the second parameter altogether. --- src/bytes_mut.rs | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/src/bytes_mut.rs b/src/bytes_mut.rs index 619defc21..bb72a2107 100644 --- a/src/bytes_mut.rs +++ b/src/bytes_mut.rs @@ -247,7 +247,7 @@ impl BytesMut { if self.kind() == KIND_VEC { // Just re-use `Bytes` internal Vec vtable unsafe { - let (off, _) = self.get_vec_pos(); + let off = self.get_vec_pos(); let vec = rebuild_vec(self.ptr.as_ptr(), self.len, self.cap, off); mem::forget(self); let mut b: Bytes = vec.into(); @@ -596,7 +596,7 @@ impl BytesMut { // We need to make sure that this optimization does not kill the // amortized runtimes of BytesMut's operations. unsafe { - let (off, prev) = self.get_vec_pos(); + let off = self.get_vec_pos(); // Only reuse space if we can satisfy the requested additional space. // @@ -621,7 +621,7 @@ impl BytesMut { // Since `off >= self.len()`, the two regions don't overlap. ptr::copy_nonoverlapping(self.ptr.as_ptr(), base_ptr, self.len); self.ptr = vptr(base_ptr); - self.set_vec_pos(0, prev); + self.set_vec_pos(0); // Length stays constant, but since we moved backwards we // can gain capacity back. @@ -867,11 +867,10 @@ impl BytesMut { // complicated. First, we have to track how far ahead the // "start" of the byte buffer from the beginning of the vec. We // also have to ensure that we don't exceed the maximum shift. - let (mut pos, prev) = self.get_vec_pos(); - pos += start; + let pos = self.get_vec_pos() + start; if pos <= MAX_VEC_POS { - self.set_vec_pos(pos, prev); + self.set_vec_pos(pos); } else { // The repr must be upgraded to ARC. This will never happen // on 64 bit systems and will only happen on 32 bit systems @@ -979,19 +978,18 @@ impl BytesMut { } #[inline] - unsafe fn get_vec_pos(&mut self) -> (usize, usize) { + unsafe fn get_vec_pos(&mut self) -> usize { debug_assert_eq!(self.kind(), KIND_VEC); - let prev = self.data as usize; - (prev >> VEC_POS_OFFSET, prev) + self.data as usize >> VEC_POS_OFFSET } #[inline] - unsafe fn set_vec_pos(&mut self, pos: usize, prev: usize) { + unsafe fn set_vec_pos(&mut self, pos: usize) { debug_assert_eq!(self.kind(), KIND_VEC); debug_assert!(pos <= MAX_VEC_POS); - self.data = invalid_ptr((pos << VEC_POS_OFFSET) | (prev & NOT_VEC_POS_MASK)); + self.data = invalid_ptr((pos << VEC_POS_OFFSET) | (self.data as usize & NOT_VEC_POS_MASK)); } /// Returns the remaining spare capacity of the buffer as a slice of `MaybeUninit`. @@ -1040,7 +1038,7 @@ impl Drop for BytesMut { if kind == KIND_VEC { unsafe { - let (off, _) = self.get_vec_pos(); + let off = self.get_vec_pos(); // Vector storage, free the vector let _ = rebuild_vec(self.ptr.as_ptr(), self.len, self.cap, off); @@ -1625,7 +1623,7 @@ impl From for Vec { let mut vec = if kind == KIND_VEC { unsafe { - let (off, _) = bytes.get_vec_pos(); + let off = bytes.get_vec_pos(); rebuild_vec(bytes.ptr.as_ptr(), bytes.len, bytes.cap, off) } } else {