Skip to content

Remove fast path in reallocation for same layout sizes #75621

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Aug 18, 2020
Merged

Remove fast path in reallocation for same layout sizes #75621

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c619b36
Remove fast path in reallocation for same layout sizes
TimDiekmann Aug 17, 2020
c48f784
Fix typo in comment
TimDiekmann Aug 17, 2020
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
27 changes: 10 additions & 17 deletions library/alloc/src/alloc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,16 +209,14 @@ unsafe impl AllocRef for Global {
);

// SAFETY: `new_size` must be non-zero, which is checked in the match expression.
// If `new_size` is zero, then `old_size` has to be zero as well.
// Other conditions must be upheld by the caller
Copy link
Member

Choose a reason for hiding this comment

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

This is not documented though, is it? It least grow does not have a comment explaining the caller obligations.

Looks like new_size >= old_size would have to be among those.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a safety constraint, that new_size must be greater than or equal to old_size. If old_size is non-zero (was checked just before), new_size can't be zero.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have a commit ready to be pushed, which cleans up the implementations and documentation of AllocRef a bit, so those things get more clear. The unsafe should be moved to line 217 instead. The match arm, where layout.size() == 0 does not need unsafe at all.

Copy link
Member

@RalfJung RalfJung Aug 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Right, my only comment is that this is an undocumented safety constraint -- at least when looking just at the grow function, and not at the entire file.

EDIT: well, there is a debug assertion. But the usual comment explaining to the caller what they have to uphold is missing.

Copy link
Member Author

Choose a reason for hiding this comment

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

However, I didn't want to change implementations much in this PR because of the perf-run.

Copy link
Member Author

Choose a reason for hiding this comment

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

But the usual comment explaining to the caller what they have to uphold is missing.

This is stated in the safety-section of the trait.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see... in GH I couldn't see that this is a trait impl. Sorry.

Copy link
Member Author

Choose a reason for hiding this comment

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

No Problem!
In any case #75657 should make this more readable if you only read the code. :)

unsafe {
match layout.size() {
old_size if old_size == new_size => {
Ok(NonNull::slice_from_raw_parts(ptr, new_size))
}
0 => self.alloc(Layout::from_size_align_unchecked(new_size, layout.align())),
old_size => {
// `realloc` probably checks for `new_size > size` or something similar.
intrinsics::assume(new_size > old_size);
// `realloc` probably checks for `new_size >= size` or something similar.
intrinsics::assume(new_size >= old_size);
let raw_ptr = realloc(ptr.as_ptr(), layout, new_size);
let ptr = NonNull::new(raw_ptr).ok_or(AllocErr)?;
Ok(NonNull::slice_from_raw_parts(ptr, new_size))
Expand All @@ -240,16 +238,14 @@ unsafe impl AllocRef for Global {
);

// SAFETY: `new_size` must be non-zero, which is checked in the match expression.
// If `new_size` is zero, then `old_size` has to be zero as well.
// Other conditions must be upheld by the caller
unsafe {
match layout.size() {
old_size if old_size == new_size => {
Ok(NonNull::slice_from_raw_parts(ptr, new_size))
}
0 => self.alloc_zeroed(Layout::from_size_align_unchecked(new_size, layout.align())),
old_size => {
// `realloc` probably checks for `new_size > size` or something similar.
intrinsics::assume(new_size > old_size);
// `realloc` probably checks for `new_size >= size` or something similar.
intrinsics::assume(new_size >= old_size);
let raw_ptr = realloc(ptr.as_ptr(), layout, new_size);
raw_ptr.add(old_size).write_bytes(0, new_size - old_size);
let ptr = NonNull::new(raw_ptr).ok_or(AllocErr)?;
Expand All @@ -272,11 +268,8 @@ unsafe impl AllocRef for Global {
"`new_size` must be smaller than or equal to `layout.size()`"
);

let ptr = if new_size == old_size {
ptr
} else if new_size == 0 {
// SAFETY: `layout` is non-zero in size as `old_size` != `new_size`
// Other conditions must be upheld by the caller
let ptr = if new_size == 0 {
// SAFETY: conditions must be upheld by the caller
unsafe {
self.dealloc(ptr, layout);
}
Expand All @@ -285,8 +278,8 @@ unsafe impl AllocRef for Global {
// SAFETY: new_size is not zero,
// Other conditions must be upheld by the caller
let raw_ptr = unsafe {
// `realloc` probably checks for `new_size < old_size` or something similar.
intrinsics::assume(new_size < old_size);
// `realloc` probably checks for `new_size <= old_size` or something similar.
intrinsics::assume(new_size <= old_size);
realloc(ptr.as_ptr(), layout, new_size)
};
NonNull::new(raw_ptr).ok_or(AllocErr)?
Expand Down
18 changes: 0 additions & 18 deletions library/core/src/alloc/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,8 +163,6 @@ pub unsafe trait AllocRef {
/// * `new_size` must be greater than or equal to `layout.size()`, and
/// * `new_size`, when rounded up to the nearest multiple of `layout.align()`, must not overflow
/// (i.e., the rounded value must be less than or equal to `usize::MAX`).
// Note: We can't require that `new_size` is strictly greater than `layout.size()` because of ZSTs.
// alternative: `new_size` must be strictly greater than `layout.size()` or both are zero
///
/// [*currently allocated*]: #currently-allocated-memory
/// [*fit*]: #memory-fitting
Expand Down Expand Up @@ -194,10 +192,6 @@ pub unsafe trait AllocRef {
"`new_size` must be greater than or equal to `layout.size()`"
);

if size == new_size {
return Ok(NonNull::slice_from_raw_parts(ptr, size));
}

let new_layout =
// SAFETY: the caller must ensure that the `new_size` does not overflow.
// `layout.align()` comes from a `Layout` and is thus guaranteed to be valid for a Layout.
Expand Down Expand Up @@ -238,8 +232,6 @@ pub unsafe trait AllocRef {
/// * `new_size` must be greater than or equal to `layout.size()`, and
/// * `new_size`, when rounded up to the nearest multiple of `layout.align()`, must not overflow
/// (i.e., the rounded value must be less than or equal to `usize::MAX`).
// Note: We can't require that `new_size` is strictly greater than `layout.size()` because of ZSTs.
// alternative: `new_size` must be strictly greater than `layout.size()` or both are zero
///
/// [*currently allocated*]: #currently-allocated-memory
/// [*fit*]: #memory-fitting
Expand Down Expand Up @@ -269,10 +261,6 @@ pub unsafe trait AllocRef {
"`new_size` must be greater than or equal to `layout.size()`"
);

if size == new_size {
return Ok(NonNull::slice_from_raw_parts(ptr, size));
}

let new_layout =
// SAFETY: the caller must ensure that the `new_size` does not overflow.
// `layout.align()` comes from a `Layout` and is thus guaranteed to be valid for a Layout.
Expand Down Expand Up @@ -315,8 +303,6 @@ pub unsafe trait AllocRef {
/// * `ptr` must denote a block of memory [*currently allocated*] via this allocator,
/// * `layout` must [*fit*] that block of memory (The `new_size` argument need not fit it.), and
/// * `new_size` must be smaller than or equal to `layout.size()`.
// Note: We can't require that `new_size` is strictly smaller than `layout.size()` because of ZSTs.
// alternative: `new_size` must be smaller than `layout.size()` or both are zero
///
/// [*currently allocated*]: #currently-allocated-memory
/// [*fit*]: #memory-fitting
Expand Down Expand Up @@ -346,10 +332,6 @@ pub unsafe trait AllocRef {
"`new_size` must be smaller than or equal to `layout.size()`"
);

if size == new_size {
return Ok(NonNull::slice_from_raw_parts(ptr, size));
}

let new_layout =
// SAFETY: the caller must ensure that the `new_size` does not overflow.
// `layout.align()` comes from a `Layout` and is thus guaranteed to be valid for a Layout.
Expand Down
27 changes: 10 additions & 17 deletions library/std/src/alloc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,16 +185,14 @@ unsafe impl AllocRef for System {
);

// SAFETY: `new_size` must be non-zero, which is checked in the match expression.
// If `new_size` is zero, then `old_size` has to be zero as well.
// Other conditions must be upheld by the caller
unsafe {
match layout.size() {
old_size if old_size == new_size => {
Ok(NonNull::slice_from_raw_parts(ptr, new_size))
}
0 => self.alloc(Layout::from_size_align_unchecked(new_size, layout.align())),
old_size => {
// `realloc` probably checks for `new_size > size` or something similar.
intrinsics::assume(new_size > old_size);
// `realloc` probably checks for `new_size >= size` or something similar.
intrinsics::assume(new_size >= old_size);
let raw_ptr = GlobalAlloc::realloc(&System, ptr.as_ptr(), layout, new_size);
let ptr = NonNull::new(raw_ptr).ok_or(AllocErr)?;
Ok(NonNull::slice_from_raw_parts(ptr, new_size))
Expand All @@ -216,16 +214,14 @@ unsafe impl AllocRef for System {
);

// SAFETY: `new_size` must be non-zero, which is checked in the match expression.
// If `new_size` is zero, then `old_size` has to be zero as well.
// Other conditions must be upheld by the caller
unsafe {
match layout.size() {
old_size if old_size == new_size => {
Ok(NonNull::slice_from_raw_parts(ptr, new_size))
}
0 => self.alloc_zeroed(Layout::from_size_align_unchecked(new_size, layout.align())),
old_size => {
// `realloc` probably checks for `new_size > size` or something similar.
intrinsics::assume(new_size > old_size);
// `realloc` probably checks for `new_size >= size` or something similar.
intrinsics::assume(new_size >= old_size);
let raw_ptr = GlobalAlloc::realloc(&System, ptr.as_ptr(), layout, new_size);
raw_ptr.add(old_size).write_bytes(0, new_size - old_size);
let ptr = NonNull::new(raw_ptr).ok_or(AllocErr)?;
Expand All @@ -248,11 +244,8 @@ unsafe impl AllocRef for System {
"`new_size` must be smaller than or equal to `layout.size()`"
);

let ptr = if new_size == old_size {
ptr
} else if new_size == 0 {
// SAFETY: `layout` is non-zero in size as `old_size` != `new_size`
// Other conditions must be upheld by the caller
let ptr = if new_size == 0 {
// SAFETY: conditions must be upheld by the caller
unsafe {
self.dealloc(ptr, layout);
}
Expand All @@ -261,8 +254,8 @@ unsafe impl AllocRef for System {
// SAFETY: new_size is not zero,
// Other conditions must be upheld by the caller
let raw_ptr = unsafe {
// `realloc` probably checks for `new_size < old_size` or something similar.
intrinsics::assume(new_size < old_size);
// `realloc` probably checks for `new_size <= old_size` or something similar.
intrinsics::assume(new_size <= old_size);
GlobalAlloc::realloc(&System, ptr.as_ptr(), layout, new_size)
};
NonNull::new(raw_ptr).ok_or(AllocErr)?
Expand Down