From b413d0e90d74f9b8bb5e1cfdccbe0c0780a760f2 Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Fri, 7 Jan 2022 19:31:13 +0100 Subject: [PATCH 01/12] Fix amortized asymptotics of BytesMut --- src/bytes_mut.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/bytes_mut.rs b/src/bytes_mut.rs index 3026f0982..5af0c1083 100644 --- a/src/bytes_mut.rs +++ b/src/bytes_mut.rs @@ -579,17 +579,22 @@ impl BytesMut { // space. // // Otherwise, since backed by a vector, use `Vec::reserve` + // + // 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(); - // Only reuse space if we can satisfy the requested additional space. - if self.capacity() - self.len() + off >= additional { - // There's space - reuse it + // Only reuse space if we can satisfy the requested additional space, + // and if reusing the space (at least) doubles the capacity. + if self.capacity() - self.len() + off >= additional && off >= self.capacity() { + // There's space, and significant capacity increase - reuse the space // // Just move the pointer back to the start after copying // data back. let base_ptr = self.ptr.as_ptr().offset(-(off as isize)); - ptr::copy(self.ptr.as_ptr(), base_ptr, self.len); + // since off >= self.capacity() >= 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); @@ -597,7 +602,7 @@ impl BytesMut { // can gain capacity back. self.cap += off; } else { - // No space - allocate more + // No space, or not enough capacity gained - allocate more space let mut v = ManuallyDrop::new(rebuild_vec(self.ptr.as_ptr(), self.len, self.cap, off)); v.reserve(additional); From e7e6c7bc01e3b509a581d6a1727449e41e691b9e Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Fri, 7 Jan 2022 20:49:49 +0100 Subject: [PATCH 02/12] Make space-reclaiming a bit more applicable again In particular, now `bytes_buf_mut_reuse_when_fully_consumed` passes again. --- src/bytes_mut.rs | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/bytes_mut.rs b/src/bytes_mut.rs index 5af0c1083..b0a3c0a66 100644 --- a/src/bytes_mut.rs +++ b/src/bytes_mut.rs @@ -585,15 +585,24 @@ impl BytesMut { unsafe { let (off, prev) = self.get_vec_pos(); - // Only reuse space if we can satisfy the requested additional space, - // and if reusing the space (at least) doubles the capacity. - if self.capacity() - self.len() + off >= additional && off >= self.capacity() { - // There's space, and significant capacity increase - reuse the space + // Only reuse space if we can satisfy the requested additional space. + // + // Also check if the value of `off` suggests that enough bytes have been read + // to account for the overhead of shifting all the data (in an amortized analysis). + // Hence the condition `off >= self.len()`. + // + // This condition also already implies that the buffer is going to be + // (at least) half-empty in the end; so we do not break the (amortized) runtime + // with future resizes of the underlying `Vec`. + // + // [For more details check issue #524, and PR #525.] + if self.capacity() - self.len() + off >= additional && off >= self.len() { + // There's enough space, and it's not too much overhead - reuse the space // // Just move the pointer back to the start after copying // data back. let base_ptr = self.ptr.as_ptr().offset(-(off as isize)); - // since off >= self.capacity() >= self.len, the two regions don't overlap + // 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); @@ -602,7 +611,7 @@ impl BytesMut { // can gain capacity back. self.cap += off; } else { - // No space, or not enough capacity gained - allocate more space + // Not enough space, or reusing might be too much overhead - allocate more space let mut v = ManuallyDrop::new(rebuild_vec(self.ptr.as_ptr(), self.len, self.cap, off)); v.reserve(additional); From 2a9b95899cb7f846be0731787cb55a2bbdf51b56 Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Wed, 9 Feb 2022 21:11:49 +0100 Subject: [PATCH 03/12] Make documentation reflect new implementation of reserve --- src/bytes_mut.rs | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/bytes_mut.rs b/src/bytes_mut.rs index b0a3c0a66..305ec7711 100644 --- a/src/bytes_mut.rs +++ b/src/bytes_mut.rs @@ -511,11 +511,19 @@ impl BytesMut { /// reallocations. A call to `reserve` may result in an allocation. /// /// Before allocating new buffer space, the function will attempt to reclaim - /// space in the existing buffer. If the current handle references a small - /// view in the original buffer and all other handles have been dropped, - /// and the requested capacity is less than or equal to the existing - /// buffer's capacity, then the current view will be copied to the front of - /// the buffer and the handle will take ownership of the full buffer. + /// space in the existing buffer. If all other handles referencing part of the same + /// original buffer have been dropped and the current handle references a relatively small view + /// of the original buffer that's relatively far offset into the original buffer, then no new + /// allocation is necessary, as long as the original buffer's capacity is sufficiently large to + /// contain the current view _and_ the requested additional amount of bytes. In this case, the + /// current view will be copied to the front of the existing buffer, and the handle will take + /// ownership of the full buffer. Additional constraints will apply for when this can happen, + /// in order to prevent access patterns with surprising amounts of expensive copying operations. + /// If there is nothing to copy (i.e. the current view is empty), then reallocation is always + /// avoided whenever the original buffer is sufficiently large. In general, as of this writing, + /// the buffer will only be reclaimed if the offset of the current view (from the start + /// of the original buffer) is be greater than or equal to the length of the current view, + /// but this precise condition might change in the future. /// /// # Examples /// From ee3cafda8a175b56ccbd03e3d850e57bfea5accb Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Wed, 9 Feb 2022 21:42:14 +0100 Subject: [PATCH 04/12] Adapt logic of previous commits to the `kind == KIND_ARC` case --- src/bytes_mut.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/bytes_mut.rs b/src/bytes_mut.rs index 305ec7711..9e885facc 100644 --- a/src/bytes_mut.rs +++ b/src/bytes_mut.rs @@ -658,11 +658,17 @@ impl BytesMut { // sure that the vector has enough capacity. let v = &mut (*shared).vec; - if v.capacity() >= new_cap { - // The capacity is sufficient, reclaim the buffer - let ptr = v.as_mut_ptr(); + let v_capacity = v.capacity(); + let ptr = v.as_mut_ptr(); + let offset = self.ptr.as_ptr().offset_from(ptr) as usize; - ptr::copy(self.ptr.as_ptr(), ptr, len); + // Compare the condition in the `kind == KIND_VEC` case above for more details. + if v_capacity >= new_cap && offset >= len { + // The capacity is sufficient, and copying is not too much overhead: + // reclaim the buffer! + + // offset >= len implies no overlap + ptr::copy_nonoverlapping(self.ptr.as_ptr(), ptr, len); self.ptr = vptr(ptr); self.cap = v.capacity(); From 352899d1cbc194f43bb0714335d08390a8888957 Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Wed, 9 Feb 2022 21:51:09 +0100 Subject: [PATCH 05/12] Further improvements to docs --- src/bytes_mut.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/bytes_mut.rs b/src/bytes_mut.rs index 9e885facc..077d1a366 100644 --- a/src/bytes_mut.rs +++ b/src/bytes_mut.rs @@ -517,11 +517,13 @@ impl BytesMut { /// allocation is necessary, as long as the original buffer's capacity is sufficiently large to /// contain the current view _and_ the requested additional amount of bytes. In this case, the /// current view will be copied to the front of the existing buffer, and the handle will take - /// ownership of the full buffer. Additional constraints will apply for when this can happen, + /// ownership of the full buffer. + /// + /// There are additional constraints for when this reclaiming can happen, /// in order to prevent access patterns with surprising amounts of expensive copying operations. /// If there is nothing to copy (i.e. the current view is empty), then reallocation is always - /// avoided whenever the original buffer is sufficiently large. In general, as of this writing, - /// the buffer will only be reclaimed if the offset of the current view (from the start + /// avoided whenever the original buffer is sufficiently large. Otherwise, as of this writing, + /// the buffer will -- even if it is sufficiently large -- only be reclaimed if the offset of the current view (from the start /// of the original buffer) is be greater than or equal to the length of the current view, /// but this precise condition might change in the future. /// From 4e1e9f2114daef079c417ab99da9b73a9139a31a Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Wed, 9 Feb 2022 21:57:00 +0100 Subject: [PATCH 06/12] Readjust comments and doc-comments of this PR to 80 col width, and minor changes --- src/bytes_mut.rs | 60 +++++++++++++++++++++++++++--------------------- 1 file changed, 34 insertions(+), 26 deletions(-) diff --git a/src/bytes_mut.rs b/src/bytes_mut.rs index 077d1a366..c6e9fabb3 100644 --- a/src/bytes_mut.rs +++ b/src/bytes_mut.rs @@ -511,21 +511,24 @@ impl BytesMut { /// reallocations. A call to `reserve` may result in an allocation. /// /// Before allocating new buffer space, the function will attempt to reclaim - /// space in the existing buffer. If all other handles referencing part of the same - /// original buffer have been dropped and the current handle references a relatively small view - /// of the original buffer that's relatively far offset into the original buffer, then no new - /// allocation is necessary, as long as the original buffer's capacity is sufficiently large to - /// contain the current view _and_ the requested additional amount of bytes. In this case, the - /// current view will be copied to the front of the existing buffer, and the handle will take - /// ownership of the full buffer. + /// space in the existing buffer. If all other handles referencing part of + /// the same original buffer have been dropped and the current handle + /// references a relatively small view of the original buffer that's + /// relatively far offset into the original buffer, then no new allocation + /// is necessary, as long as the original buffer's capacity is sufficiently + /// large to contain the current view _and_ the requested additional amount + /// of bytes. In this case, the current view will be copied to the front of + /// the existing buffer, and the handle will take ownership of the full buffer. /// /// There are additional constraints for when this reclaiming can happen, - /// in order to prevent access patterns with surprising amounts of expensive copying operations. - /// If there is nothing to copy (i.e. the current view is empty), then reallocation is always - /// avoided whenever the original buffer is sufficiently large. Otherwise, as of this writing, - /// the buffer will -- even if it is sufficiently large -- only be reclaimed if the offset of the current view (from the start - /// of the original buffer) is be greater than or equal to the length of the current view, - /// but this precise condition might change in the future. + /// in order to prevent access patterns with surprising amounts of expensive + /// copying operations. If there is nothing to copy (i.e. the current view + /// is empty), then reallocation is always avoided whenever the original + /// buffer is sufficiently large. Otherwise, as of this writing, the buffer + /// will -- even if it is sufficiently large -- only be reclaimed if the + /// offset of the current view (from the start of the original buffer) is be + /// greater than or equal to the length of the current view, but this + /// precise condition might change in the future. /// /// # Examples /// @@ -590,24 +593,27 @@ impl BytesMut { // // Otherwise, since backed by a vector, use `Vec::reserve` // - // We need to make sure that this optimization does not kill the amortized - // runtimes of BytesMut's operations. + // 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(); // Only reuse space if we can satisfy the requested additional space. // - // Also check if the value of `off` suggests that enough bytes have been read - // to account for the overhead of shifting all the data (in an amortized analysis). + // Also check if the value of `off` suggests that enough bytes + // have been read to account for the overhead of shifting all + // the data (in an amortized analysis). // Hence the condition `off >= self.len()`. // - // This condition also already implies that the buffer is going to be - // (at least) half-empty in the end; so we do not break the (amortized) runtime - // with future resizes of the underlying `Vec`. + // This condition also already implies that the buffer is going + // to be (at least) half-empty in the end; so we do not break + // the (amortized) runtime with future resizes of the underlying + // `Vec`. // // [For more details check issue #524, and PR #525.] if self.capacity() - self.len() + off >= additional && off >= self.len() { - // There's enough space, and it's not too much overhead - reuse the space + // There's enough space, and it's not too much overhead: + // reuse the space! // // Just move the pointer back to the start after copying // data back. @@ -621,7 +627,8 @@ impl BytesMut { // can gain capacity back. self.cap += off; } else { - // Not enough space, or reusing might be too much overhead - allocate more space + // Not enough space, or reusing might be too much overhead: + // allocate more space! let mut v = ManuallyDrop::new(rebuild_vec(self.ptr.as_ptr(), self.len, self.cap, off)); v.reserve(additional); @@ -664,12 +671,13 @@ impl BytesMut { let ptr = v.as_mut_ptr(); let offset = self.ptr.as_ptr().offset_from(ptr) as usize; - // Compare the condition in the `kind == KIND_VEC` case above for more details. + // Compare the condition in the `kind == KIND_VEC` case above + // for more details. if v_capacity >= new_cap && offset >= len { - // The capacity is sufficient, and copying is not too much overhead: - // reclaim the buffer! + // The capacity is sufficient, and copying is not too much + // overhead: reclaim the buffer! - // offset >= len implies no overlap + // `offset >= len` means: no overlap ptr::copy_nonoverlapping(self.ptr.as_ptr(), ptr, len); self.ptr = vptr(ptr); From f7292d2ddb33e4c9cad0538892a7e78fc27e926d Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Wed, 9 Feb 2022 21:59:23 +0100 Subject: [PATCH 07/12] Another small documentation improvement --- src/bytes_mut.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/bytes_mut.rs b/src/bytes_mut.rs index c6e9fabb3..60c602537 100644 --- a/src/bytes_mut.rs +++ b/src/bytes_mut.rs @@ -512,18 +512,18 @@ impl BytesMut { /// /// Before allocating new buffer space, the function will attempt to reclaim /// space in the existing buffer. If all other handles referencing part of - /// the same original buffer have been dropped and the current handle + /// the same original buffer have been dropped and the current handle /// references a relatively small view of the original buffer that's /// relatively far offset into the original buffer, then no new allocation /// is necessary, as long as the original buffer's capacity is sufficiently /// large to contain the current view _and_ the requested additional amount /// of bytes. In this case, the current view will be copied to the front of /// the existing buffer, and the handle will take ownership of the full buffer. - /// + /// /// There are additional constraints for when this reclaiming can happen, /// in order to prevent access patterns with surprising amounts of expensive /// copying operations. If there is nothing to copy (i.e. the current view - /// is empty), then reallocation is always avoided whenever the original + /// is empty), then the reclaiming _always_ happens whenever the original /// buffer is sufficiently large. Otherwise, as of this writing, the buffer /// will -- even if it is sufficiently large -- only be reclaimed if the /// offset of the current view (from the start of the original buffer) is be From ef6f81e8882dab9044d9a22dba969188dfeed6a5 Mon Sep 17 00:00:00 2001 From: Frank Steffahn Date: Fri, 15 Apr 2022 18:06:38 +0200 Subject: [PATCH 08/12] Further improve documentation. --- src/bytes_mut.rs | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/src/bytes_mut.rs b/src/bytes_mut.rs index 60c602537..91fa8d04b 100644 --- a/src/bytes_mut.rs +++ b/src/bytes_mut.rs @@ -511,24 +511,20 @@ impl BytesMut { /// reallocations. A call to `reserve` may result in an allocation. /// /// Before allocating new buffer space, the function will attempt to reclaim - /// space in the existing buffer. If all other handles referencing part of - /// the same original buffer have been dropped and the current handle - /// references a relatively small view of the original buffer that's - /// relatively far offset into the original buffer, then no new allocation - /// is necessary, as long as the original buffer's capacity is sufficiently - /// large to contain the current view _and_ the requested additional amount - /// of bytes. In this case, the current view will be copied to the front of - /// the existing buffer, and the handle will take ownership of the full buffer. - /// - /// There are additional constraints for when this reclaiming can happen, - /// in order to prevent access patterns with surprising amounts of expensive - /// copying operations. If there is nothing to copy (i.e. the current view - /// is empty), then the reclaiming _always_ happens whenever the original - /// buffer is sufficiently large. Otherwise, as of this writing, the buffer - /// will -- even if it is sufficiently large -- only be reclaimed if the - /// offset of the current view (from the start of the original buffer) is be - /// greater than or equal to the length of the current view, but this - /// precise condition might change in the future. + /// space in the existing buffer. If the current handle references a view + /// into a larger original buffer, and all other handles referencing part + /// of the same original buffer have been dropped, then the current view + /// can be copied/shifted to the front of the buffer and the handle can take + /// ownership of the full buffer, provided that the full buffer is large + /// enough to fit the requested additional capacity. + /// + /// This optimization will only happen if shifting the data from the current + /// view to the front of the buffer is not too expensive in terms of the + /// (amortized) time required. The precise condition is subject to change; + /// as of now, the length of the data being shifted needs to be at least as + /// large as the distance that it's shifted by. If the current view is empty + /// and the original buffer is large enough to fit the requested additional + /// capacity, then reallocations will never happen. /// /// # Examples /// From bb6e2f063c65753061938825bd59bad8b5d22079 Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Tue, 19 Jul 2022 19:13:39 +1000 Subject: [PATCH 09/12] Fix CI minrust: Rm use of `offset_from` Signed-off-by: Jiahao XU --- src/bytes_mut.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/bytes_mut.rs b/src/bytes_mut.rs index 91fa8d04b..ba6f596da 100644 --- a/src/bytes_mut.rs +++ b/src/bytes_mut.rs @@ -665,7 +665,14 @@ impl BytesMut { let v_capacity = v.capacity(); let ptr = v.as_mut_ptr(); - let offset = self.ptr.as_ptr().offset_from(ptr) as usize; + + // The following line is equivalent to: + // + // self.ptr.as_ptr().offset_from(ptr) as usize; + // + // But due to min rust is 1.39 and it is only stablised + // in 1.47, we cannot use it. + let offset = self.ptr.as_ptr() as usize - ptr as usize; // Compare the condition in the `kind == KIND_VEC` case above // for more details. From 31e76895310223a81cee68546312dcc8e256c00e Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Tue, 19 Jul 2022 19:16:28 +1000 Subject: [PATCH 10/12] Fix "must-use" err in `release_shared` of both module `bytes_mut` and `bytes`. Signed-off-by: Jiahao XU --- src/bytes.rs | 2 +- src/bytes_mut.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/bytes.rs b/src/bytes.rs index 948b10e57..f8d3ce319 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -1190,7 +1190,7 @@ unsafe fn release_shared(ptr: *mut Shared) { (*ptr).ref_cnt.load(Ordering::Acquire); // Drop the data - Box::from_raw(ptr); + drop(Box::from_raw(ptr)); } // Ideally we would always use this version of `ptr_map` since it is strict diff --git a/src/bytes_mut.rs b/src/bytes_mut.rs index ba6f596da..506acad1c 100644 --- a/src/bytes_mut.rs +++ b/src/bytes_mut.rs @@ -1335,7 +1335,7 @@ unsafe fn release_shared(ptr: *mut Shared) { (*ptr).ref_count.load(Ordering::Acquire); // Drop the data - Box::from_raw(ptr); + drop(Box::from_raw(ptr)); } impl Shared { From f21957e6443e32826b19368c244c5818838378f6 Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Tue, 19 Jul 2022 21:04:18 +1000 Subject: [PATCH 11/12] Refactor: Extract new fn `bytes_mut::offset_from` Signed-off-by: Jiahao XU --- src/bytes_mut.rs | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/src/bytes_mut.rs b/src/bytes_mut.rs index 506acad1c..b0e960d4e 100644 --- a/src/bytes_mut.rs +++ b/src/bytes_mut.rs @@ -666,13 +666,7 @@ impl BytesMut { let v_capacity = v.capacity(); let ptr = v.as_mut_ptr(); - // The following line is equivalent to: - // - // self.ptr.as_ptr().offset_from(ptr) as usize; - // - // But due to min rust is 1.39 and it is only stablised - // in 1.47, we cannot use it. - let offset = self.ptr.as_ptr() as usize - ptr as usize; + let offset = offset_from(self.ptr.as_ptr(), ptr); // Compare the condition in the `kind == KIND_VEC` case above // for more details. @@ -1640,6 +1634,21 @@ fn invalid_ptr(addr: usize) -> *mut T { ptr.cast::() } +/// Precondition: dst >= original +/// +/// The following line is equivalent to: +/// +/// self.ptr.as_ptr().offset_from(ptr) as usize; +/// +/// But due to min rust is 1.39 and it is only stablised +/// in 1.47, we cannot use it. +#[inline] +fn offset_from(dst: *mut u8, original: *mut u8) -> usize { + debug_assert!(dst >= original); + + dst as usize - original as usize +} + unsafe fn rebuild_vec(ptr: *mut u8, mut len: usize, mut cap: usize, off: usize) -> Vec { let ptr = ptr.offset(-(off as isize)); len += off; From 325afa46d44974910aafa6f35cffc2d72f6f55af Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Tue, 19 Jul 2022 21:08:17 +1000 Subject: [PATCH 12/12] Fix doc of `bytes_mut::offset_from` Signed-off-by: Jiahao XU --- src/bytes_mut.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/bytes_mut.rs b/src/bytes_mut.rs index b0e960d4e..99e849716 100644 --- a/src/bytes_mut.rs +++ b/src/bytes_mut.rs @@ -1638,7 +1638,9 @@ fn invalid_ptr(addr: usize) -> *mut T { /// /// The following line is equivalent to: /// -/// self.ptr.as_ptr().offset_from(ptr) as usize; +/// ```rust,ignore +/// self.ptr.as_ptr().offset_from(ptr) as usize; +/// ``` /// /// But due to min rust is 1.39 and it is only stablised /// in 1.47, we cannot use it.