Skip to content

Commit 8f13fad

Browse files
Rollup merge of rust-lang#120116 - the8472:only-same-alignments, r=cuviper
Remove alignment-changing in-place collect This removes the alignment-changing in-place collect optimization introduced in rust-lang#110353 Currently stable users can't benefit from the optimization because GlobaAlloc doesn't support alignment-changing realloc and neither do most posix allocators. So in practice it has a negative impact on performance. Explanation from rust-lang#120091 (comment): > > You mention that in case of alignment mismatch -- when the new alignment is less than the old -- the implementation calls `mremap`. > > I was trying to note that this isn't really the case in practice, due to the semantics of Rust's allocator APIs. The only use of the allocator within the `in_place_collect` implementation itself is [a call to `Allocator::shrink()`](https://github.com/rust-lang/rust/blob/db7125f008cfd72e8951c9a863178956e2cbacc3/library/alloc/src/vec/in_place_collect.rs#L299-L303), which per its documentation [allows decreasing the required alignment](https://doc.rust-lang.org/1.75.0/core/alloc/trait.Allocator.html). However, in stable Rust, the only available `Allocator` is [`Global`](https://doc.rust-lang.org/1.75.0/alloc/alloc/struct.Global.html), which delegates to the registered `GlobalAlloc`. Since `GlobalAlloc::realloc()` [cannot change the required alignment](https://doc.rust-lang.org/1.75.0/core/alloc/trait.GlobalAlloc.html#method.realloc), the implementation of [`<Global as Allocator>::shrink()`](https://github.com/rust-lang/rust/blob/db7125f008cfd72e8951c9a863178956e2cbacc3/library/alloc/src/alloc.rs#L280-L321) must fall back to creating a brand-new allocation, `memcpy`ing the data into it, and freeing the old allocation, whenever the alignment doesn't remain exactly the same. > > Therefore, the underlying allocator, provided by libc or some other source, has no opportunity to internally `mremap()` the data when the alignment is changed, since it has no way of knowing that the allocation is the same.
2 parents c4ad0e7 + 85d1787 commit 8f13fad

File tree

2 files changed

+15
-12
lines changed

2 files changed

+15
-12
lines changed

library/alloc/src/vec/in_place_collect.rs

+8-5
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,9 @@ const fn in_place_collectible<DEST, SRC>(
168168
step_merge: Option<NonZeroUsize>,
169169
step_expand: Option<NonZeroUsize>,
170170
) -> bool {
171-
if const { SRC::IS_ZST || DEST::IS_ZST || mem::align_of::<SRC>() < mem::align_of::<DEST>() } {
171+
// Require matching alignments because an alignment-changing realloc is inefficient on many
172+
// system allocators and better implementations would require the unstable Allocator trait.
173+
if const { SRC::IS_ZST || DEST::IS_ZST || mem::align_of::<SRC>() != mem::align_of::<DEST>() } {
172174
return false;
173175
}
174176

@@ -188,7 +190,8 @@ const fn in_place_collectible<DEST, SRC>(
188190

189191
const fn needs_realloc<SRC, DEST>(src_cap: usize, dst_cap: usize) -> bool {
190192
if const { mem::align_of::<SRC>() != mem::align_of::<DEST>() } {
191-
return src_cap > 0;
193+
// FIXME: use unreachable! once that works in const
194+
panic!("in_place_collectible() prevents this");
192195
}
193196

194197
// If src type size is an integer multiple of the destination type size then
@@ -276,8 +279,8 @@ where
276279
let dst_guard = InPlaceDstBufDrop { ptr: dst_buf, len, cap: dst_cap };
277280
src.forget_allocation_drop_remaining();
278281

279-
// Adjust the allocation if the alignment didn't match or the source had a capacity in bytes
280-
// that wasn't a multiple of the destination type size.
282+
// Adjust the allocation if the source had a capacity in bytes that wasn't a multiple
283+
// of the destination type size.
281284
// Since the discrepancy should generally be small this should only result in some
282285
// bookkeeping updates and no memmove.
283286
if needs_realloc::<I::Src, T>(src_cap, dst_cap) {
@@ -290,7 +293,7 @@ where
290293
let src_size = mem::size_of::<I::Src>().unchecked_mul(src_cap);
291294
let old_layout = Layout::from_size_align_unchecked(src_size, src_align);
292295

293-
// The must be equal or smaller for in-place iteration to be possible
296+
// The allocation must be equal or smaller for in-place iteration to be possible
294297
// therefore the new layout must be ≤ the old one and therefore valid.
295298
let dst_align = mem::align_of::<T>();
296299
let dst_size = mem::size_of::<T>().unchecked_mul(dst_cap);

library/alloc/src/vec/spec_from_iter.rs

+7-7
Original file line numberDiff line numberDiff line change
@@ -13,13 +13,13 @@ use super::{IntoIter, SpecExtend, SpecFromIterNested, Vec};
1313
/// +-+-----------+
1414
/// |
1515
/// v
16-
/// +-+-------------------------------+ +---------------------+
17-
/// |SpecFromIter +---->+SpecFromIterNested |
18-
/// |where I: | | |where I: |
19-
/// | Iterator (default)----------+ | | Iterator (default) |
20-
/// | vec::IntoIter | | | TrustedLen |
21-
/// | SourceIterMarker---fallback-+ | +---------------------+
22-
/// +---------------------------------+
16+
/// +-+---------------------------------+ +---------------------+
17+
/// |SpecFromIter +---->+SpecFromIterNested |
18+
/// |where I: | | |where I: |
19+
/// | Iterator (default)------------+ | | Iterator (default) |
20+
/// | vec::IntoIter | | | TrustedLen |
21+
/// | InPlaceCollect--(fallback to)-+ | +---------------------+
22+
/// +-----------------------------------+
2323
/// ```
2424
pub(super) trait SpecFromIter<T, I> {
2525
fn from_iter(iter: I) -> Self;

0 commit comments

Comments
 (0)