New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replacement over stale PR: Fix bytes mut asymptotics #555
Replacement over stale PR: Fix bytes mut asymptotics #555
Conversation
In particular, now `bytes_buf_mut_reuse_when_fully_consumed` passes again.
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
of both module `bytes_mut` and `bytes`. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks reasonable. The documentation is well written.
I have one nit:
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
They are actually written by @steffahn , I just rebased them against master and fixed the CI. |
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Darksonn Can we make a new release? It has been a long time since the last release. |
Yeah, it looks like it's time for a release. |
Ah, nice that you got it working. Did the issues with #525 just disappear? (Test failure, and some UB in miri.) Or did you do something to solve them? Maybe if their origin was in unrelated code, they got fixed in the meantime? Last time I took a look around for where the problems could come from, I got hung up on hard to understand code, and details such as code seemingly relying on (maybe(?) questionable) things such as that the uninitialized portion of a Footnotes
|
Rebasing against master fixed almost all of them except for minrust (due to use of I fixed them and it just works. |
The standard library |
Oh, well, that's nice I guess 😅. I wasn't really thinking there were any problems in my change of the code anyways (as also indicated by the fact that, at the time, problems only started to appear after some changes on master). |
Alright. I'll try to find where in |
If you look at the release PR I just posted, you'll see quite a few PRs related to fixing miri. It was probably those that fixed most of the CI issues on your PR.
Thanks! |
Haven’t looked back into the code yet, instead – as a first step – I tried a more general approach for determining whether or not there’s a problem at all, by utilizing miri, with a modified standard library that avoids copying any not-considered-initialized portion of the (click to see the diff)diff --git a/library/alloc/src/raw_vec.rs b/library/alloc/src/raw_vec.rs
index b0f4529abdf..b32afee9e43 100644
--- a/library/alloc/src/raw_vec.rs
+++ b/library/alloc/src/raw_vec.rs
@@ -397,7 +397,12 @@ fn grow_amortized(&mut self, len: usize, additional: usize) -> Result<(), TryRes
let new_layout = Layout::array::<T>(cap);
// `finish_grow` is non-generic over `T`.
- let ptr = finish_grow(new_layout, self.current_memory(), &mut self.alloc)?;
+ let ptr = finish_grow(
+ len * mem::size_of::<T>(),
+ new_layout,
+ self.current_memory(),
+ &mut self.alloc,
+ )?;
self.set_ptr_and_cap(ptr, cap);
Ok(())
}
@@ -416,7 +421,12 @@ fn grow_exact(&mut self, len: usize, additional: usize) -> Result<(), TryReserve
let new_layout = Layout::array::<T>(cap);
// `finish_grow` is non-generic over `T`.
- let ptr = finish_grow(new_layout, self.current_memory(), &mut self.alloc)?;
+ let ptr = finish_grow(
+ len * mem::size_of::<T>(),
+ new_layout,
+ self.current_memory(),
+ &mut self.alloc,
+ )?;
self.set_ptr_and_cap(ptr, cap);
Ok(())
}
@@ -446,6 +456,7 @@ fn shrink(&mut self, cap: usize) -> Result<(), TryReserveError> {
// much smaller than the number of `T` types.)
#[inline(never)]
fn finish_grow<A>(
+ len_u8: usize,
new_layout: Result<Layout, LayoutError>,
current_memory: Option<(NonNull<u8>, Layout)>,
alloc: &mut A,
@@ -463,7 +474,16 @@ fn finish_grow<A>(
unsafe {
// The allocator checks for alignment equality
intrinsics::assume(old_layout.align() == new_layout.align());
- alloc.grow(ptr, old_layout, new_layout)
+ let new = alloc.allocate(new_layout);
+ if let Ok(new_ptr) = new {
+ ptr::copy_nonoverlapping(
+ ptr.as_ptr() as *const MaybeUninit<u8>,
+ new_ptr.as_mut_ptr() as *mut MaybeUninit<u8>,
+ len_u8,
+ );
+ alloc.deallocate(ptr, old_layout);
+ }
+ new
}
} else {
alloc.allocate(new_layout) (applied to 866d5c621c9b87f82a5aa53c3dfc1e477181f33b) and provided I didn’t mess up that “growing” code, there is still a problem (using
|
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com> Co-authored-by: Frank Steffahn <frank.steffahn@stu.uni-kiel.de>
Replacement for #525 as it has been stale for really long.
Fixes #524
Closes #525