Uh oh!
There was an error while loading. Please reload this page.
Remove alignment-changing in-place collect - #120116
Merged
Merged
Conversation
Currently stable users can't benefit from this because GlobaAlloc doesn't support alignment-changing realloc and neither do most posix allocators. So in practice it always results in an extra memcpy.
rustbot
commented
Jan 18, 2024
Collaborator
r? @thomcc (rustbot has picked a reviewer for you, use r? to override) |
cuviper
commented
Jan 19, 2024
Member
bors
commented
Jan 19, 2024
Collaborator
GuillaumeGomez added a commit
to GuillaumeGomez/rust
that referenced
this pull request
Jan 20, 2024
…viper 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.
This was referenced Jan 20, 2024
bors added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 20, 2024
…llaumeGomez Rollup of 6 pull requests Successful merges: - rust-lang#119997 (Fix impl stripped in rustdoc HTML whereas it should not be in case the impl is implemented on a type alias) - rust-lang#120000 (Ensure `callee_id`s are body owners) - rust-lang#120063 (Remove special handling of `box` expressions from parser) - rust-lang#120116 (Remove alignment-changing in-place collect) - rust-lang#120138 (Increase vscode settings.json `git.detectSubmodulesLimit`) - rust-lang#120169 (Spelling fix) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 20, 2024
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.
cuviper
commented
Jan 25, 2024
Member
Backport approved here: https://rust-lang.zulipchat.com/#narrow/stream/259402-t-libs.2Fmeetings/topic/Meeting.202024-01-24/near/417677351 @rustbot label +beta-accepted |
Merged
bors added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 27, 2024
[beta] backports - Remove alignment-changing in-place collect rust-lang#120116 - fix: Drop guard was deallocating with the incorrect size rust-lang#120145 r? ghost
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This removes the alignment-changing in-place collect optimization introduced in #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 #120091 (comment):