Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix in-place collect not reallocating when necessary #118460

Merged
merged 1 commit into from
Dec 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 27 additions & 7 deletions library/alloc/src/vec/in_place_collect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ const fn in_place_collectible<DEST, SRC>(
step_merge: Option<NonZeroUsize>,
step_expand: Option<NonZeroUsize>,
) -> bool {
if DEST::IS_ZST || mem::align_of::<SRC>() < mem::align_of::<DEST>() {
if const { SRC::IS_ZST || DEST::IS_ZST || mem::align_of::<SRC>() < mem::align_of::<DEST>() } {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is not part of the fix. the condition was already enforced before, I'm just making it more explicit for readability.

return false;
}

Expand All @@ -186,6 +186,27 @@ const fn in_place_collectible<DEST, SRC>(
}
}

const fn needs_realloc<SRC, DEST>(src_cap: usize, dst_cap: usize) -> bool {
if const { mem::align_of::<SRC>() != mem::align_of::<DEST>() } {
return src_cap > 0;
}

// If src type size is an integer multiple of the destination type size then
// the caller will have calculated a `dst_cap` that is an integer multiple of
// `src_cap` without remainder.
if const {
let src_sz = mem::size_of::<SRC>();
let dest_sz = mem::size_of::<DEST>();
dest_sz != 0 && src_sz % dest_sz == 0
} {
return false;
}

// type layouts don't guarantee a fit, so do a runtime check to see if
// the allocations happen to match
return src_cap > 0 && src_cap * mem::size_of::<SRC>() != dst_cap * mem::size_of::<DEST>();
}

/// This provides a shorthand for the source type since local type aliases aren't a thing.
#[rustc_specialization_trait]
trait InPlaceCollect: SourceIter<Source: AsVecIntoIter> + InPlaceIterable {
Expand Down Expand Up @@ -259,13 +280,10 @@ where
// that wasn't a multiple of the destination type size.
// Since the discrepancy should generally be small this should only result in some
// bookkeeping updates and no memmove.
if (const {
let src_sz = mem::size_of::<I::Src>();
src_sz > 0 && mem::size_of::<T>() % src_sz != 0
} && src_cap * mem::size_of::<I::Src>() != dst_cap * mem::size_of::<T>())
|| const { mem::align_of::<T>() != mem::align_of::<I::Src>() }
{
if needs_realloc::<I::Src, T>(src_cap, dst_cap) {
let alloc = Global;
debug_assert_ne!(src_cap, 0);
debug_assert_ne!(dst_cap, 0);
unsafe {
// The old allocation exists, therefore it must have a valid layout.
let src_align = mem::align_of::<I::Src>();
Expand All @@ -286,6 +304,8 @@ where
let Ok(reallocated) = result else { handle_alloc_error(new_layout) };
dst_buf = reallocated.as_ptr() as *mut T;
}
} else {
debug_assert_eq!(src_cap * mem::size_of::<I::Src>(), dst_cap * mem::size_of::<T>());
}

mem::forget(dst_guard);
Expand Down
8 changes: 8 additions & 0 deletions library/alloc/tests/vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1213,6 +1213,14 @@ fn test_in_place_specialization_step_up_down() {
assert_ne!(src_bytes, sink_bytes);
assert_eq!(sink.len(), 2);

let mut src: Vec<[u8; 3]> = Vec::with_capacity(17);
src.resize( 8, [0; 3]);
let iter = src.into_iter().map(|[a, b, _]| [a, b]);
assert_in_place_trait(&iter);
let sink: Vec<[u8; 2]> = iter.collect();
assert_eq!(sink.len(), 8);
assert!(sink.capacity() <= 25);

let src = vec![[0u8; 4]; 256];
let srcptr = src.as_ptr();
let iter = src
Expand Down
Loading