Skip to content

Add missing _excess methods to Alloc and use them in RawVec #50738

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

Closed
wants to merge 3 commits into from
Closed

Add missing _excess methods to Alloc and use them in RawVec #50738

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9b01ca8
add missing _excess methods to Alloc
gnzlbg May 14, 2018
3f9e3fd
use Excess in RawVec
gnzlbg May 14, 2018
6e2fcbd
the behavior of Vec::shrink_to_fit is allocator dependent
gnzlbg May 14, 2018
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
52 changes: 28 additions & 24 deletions src/liballoc/raw_vec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use core::ops::Drop;
use core::ptr::{self, NonNull, Unique};
use core::slice;

use alloc::{Alloc, Layout, Global, oom};
use alloc::{Alloc, Layout, Global, oom, Excess};
use alloc::CollectionAllocErr;
use alloc::CollectionAllocErr::*;
use boxed::Box;
Expand Down Expand Up @@ -92,17 +92,17 @@ impl<T, A: Alloc> RawVec<T, A> {
alloc_guard(alloc_size).unwrap_or_else(|_| capacity_overflow());

// handles ZSTs and `cap = 0` alike
let ptr = if alloc_size == 0 {
NonNull::<T>::dangling().as_opaque()
let (ptr, cap) = if alloc_size == 0 {
(NonNull::<T>::dangling().as_opaque(), cap)
} else {
let align = mem::align_of::<T>();
let result = if zeroed {
a.alloc_zeroed(Layout::from_size_align(alloc_size, align).unwrap())
a.alloc_zeroed_excess(Layout::from_size_align(alloc_size, align).unwrap())
} else {
a.alloc(Layout::from_size_align(alloc_size, align).unwrap())
a.alloc_excess(Layout::from_size_align(alloc_size, align).unwrap())
};
match result {
Ok(ptr) => ptr,
Ok(Excess(ptr, alloc_size)) => (ptr, alloc_size / elem_size),
Err(_) => oom(),
}
};
Expand Down Expand Up @@ -407,16 +407,17 @@ impl<T, A: Alloc> RawVec<T, A> {

alloc_guard(new_layout.size())?;

let res = match self.current_layout() {
let Excess(ptr, alloc_size) = match self.current_layout() {
Some(layout) => {
debug_assert!(new_layout.align() == layout.align());
self.a.realloc(NonNull::from(self.ptr).as_opaque(), layout, new_layout.size())
self.a.realloc_excess(NonNull::from(self.ptr).as_opaque(),
layout, new_layout.size())
}
None => self.a.alloc(new_layout),
};
None => self.a.alloc_excess(new_layout),
}?;

self.ptr = res?.cast().into();
self.cap = new_cap;
self.ptr = ptr.cast().into();
self.cap = alloc_size / mem::size_of::<T>();

Ok(())
}
Expand Down Expand Up @@ -485,16 +486,16 @@ impl<T, A: Alloc> RawVec<T, A> {
// FIXME: may crash and burn on over-reserve
alloc_guard(new_layout.size())?;

let res = match self.current_layout() {
let Excess(ptr, alloc_size) = match self.current_layout() {
Some(layout) => {
debug_assert!(new_layout.align() == layout.align());
self.a.realloc(NonNull::from(self.ptr).as_opaque(), layout, new_layout.size())
self.a.realloc_excess(NonNull::from(self.ptr).as_opaque(),
layout, new_layout.size())
}
None => self.a.alloc(new_layout),
};

self.ptr = res?.cast().into();
self.cap = new_cap;
None => self.a.alloc_excess(new_layout),
}?;
self.ptr = ptr.cast().into();
self.cap = alloc_size / mem::size_of::<T>();

Ok(())
}
Expand Down Expand Up @@ -604,11 +605,11 @@ impl<T, A: Alloc> RawVec<T, A> {
let new_layout = Layout::new::<T>().repeat(new_cap).unwrap().0;
// FIXME: may crash and burn on over-reserve
alloc_guard(new_layout.size()).unwrap_or_else(|_| capacity_overflow());
match self.a.grow_in_place(
match self.a.grow_in_place_excess(
NonNull::from(self.ptr).as_opaque(), old_layout, new_layout.size(),
) {
Ok(_) => {
self.cap = new_cap;
Ok(Excess(_, alloc_size)) => {
self.cap = alloc_size / mem::size_of::<T>();
true
}
Err(_) => {
Expand Down Expand Up @@ -666,10 +667,13 @@ impl<T, A: Alloc> RawVec<T, A> {
let new_size = elem_size * amount;
let align = mem::align_of::<T>();
let old_layout = Layout::from_size_align_unchecked(old_size, align);
match self.a.realloc(NonNull::from(self.ptr).as_opaque(),
match self.a.realloc_excess(NonNull::from(self.ptr).as_opaque(),
old_layout,
new_size) {
Ok(p) => self.ptr = p.cast().into(),
Ok(Excess(ptr, alloc_size)) => {
self.ptr = ptr.cast().into();
self.cap = alloc_size / mem::size_of::<T>();
},
Err(_) => oom(),
}
}
Expand Down
8 changes: 8 additions & 0 deletions src/liballoc/vec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -574,6 +574,10 @@ impl<T> Vec<T> {
/// It will drop down as close as possible to the length but the allocator
/// may still inform the vector that there is space for a few more elements.
///
/// Note: `shrink_to_fit` is a non-binding request. Whether the `Vec` size
Copy link
Member

Choose a reason for hiding this comment

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

In what way will this help someone reading the documentation?

/// will be shrunk at all, and if so by how much depends on the particular
/// allocator being used.
///
/// # Examples
///
/// ```
Expand All @@ -598,6 +602,10 @@ impl<T> Vec<T> {
/// Panics if the current capacity is smaller than the supplied
/// minimum capacity.
///
/// Note: `shrink_to` is a non-binding request. Whether the `Vec` size
/// will be shrunk at all, and if so by how much depends on the particular
/// allocator being used.
///
/// # Examples
///
/// ```
Expand Down
132 changes: 132 additions & 0 deletions src/libcore/alloc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -794,6 +794,28 @@ pub unsafe trait Alloc {
.map(|p| Excess(p, usable_size.1))
}

/// Behaves like `alloc`, but also ensures that the contents are set to zero
/// before being returned. For some `layout` inputs, like arrays, this may
/// include extra storage usable for additional data.
///
/// # Safety
///
/// This function is unsafe for the same reasons that `alloc` is.
///
/// # Errors
///
/// Returning `Err` indicates that either memory is exhausted or
/// `layout` does not meet allocator's size or alignment
/// constraints, just as in `alloc`.
///
/// Clients wishing to abort computation in response to an
/// allocation error are encouraged to call the allocator's `oom`
/// method, rather than directly invoking `panic!` or similar.
unsafe fn alloc_zeroed_excess(&mut self, layout: Layout) -> Result<Excess, AllocErr> {
let usable_size = self.usable_size(&layout);
self.alloc_zeroed(layout).map(|p| Excess(p, usable_size.1))
}

/// Attempts to extend the allocation referenced by `ptr` to fit `new_size`.
///
/// If this returns `Ok`, then the allocator has asserted that the
Expand Down Expand Up @@ -845,6 +867,60 @@ pub unsafe trait Alloc {
}
}

/// Attempts to extend the allocation referenced by `ptr` to fit `new_size`.
/// For some `layout` inputs, like arrays, this may include extra storage
/// usable for additional data.
///
/// If this returns `Ok`, then the allocator has asserted that the
/// memory block referenced by `ptr` now fits `new_size`, and thus can
/// be used to carry data of a layout of that size and same alignment as
/// `layout`. (The allocator is allowed to
/// expend effort to accomplish this, such as extending the memory block to
/// include successor blocks, or virtual memory tricks.)
///
/// Regardless of what this method returns, ownership of the
/// memory block referenced by `ptr` has not been transferred, and
/// the contents of the memory block are unaltered.
///
/// # Safety
///
/// This function is unsafe because undefined behavior can result
/// if the caller does not ensure all of the following:
///
/// * `ptr` must be currently allocated via this allocator,
///
/// * `layout` must *fit* the `ptr` (see above); note the
/// `new_size` argument need not fit it,
///
/// * `new_size` must not be less than `layout.size()`,
///
/// # Errors
///
/// Returns `Err(CannotReallocInPlace)` when the allocator is
/// unable to assert that the memory block referenced by `ptr`
/// could fit `layout`.
///
/// Note that one cannot pass `CannotReallocInPlace` to the `oom`
/// method; clients are expected either to be able to recover from
/// `grow_in_place` failures without aborting, or to fall back on
/// another reallocation method before resorting to an abort.
unsafe fn grow_in_place_excess(&mut self,
ptr: NonNull<Opaque>,
layout: Layout,
new_size: usize) -> Result<Excess, CannotReallocInPlace> {
let _ = ptr; // this default implementation doesn't care about the actual address.
debug_assert!(new_size >= layout.size);
let (_l, u) = self.usable_size(&layout);
// _l <= layout.size() [guaranteed by usable_size()]
// layout.size() <= new_layout.size() [required by this method]
if new_size <= u {
return Ok(Excess(ptr, u));
} else {
return Err(CannotReallocInPlace);
}
}


/// Attempts to shrink the allocation referenced by `ptr` to fit `new_size`.
///
/// If this returns `Ok`, then the allocator has asserted that the
Expand Down Expand Up @@ -900,6 +976,62 @@ pub unsafe trait Alloc {
}
}

/// Attempts to shrink the allocation referenced by `ptr` to fit `new_size`.
/// For some `layout` inputs, like arrays, this may include extra storage
/// usable for additional data.
///
/// If this returns `Ok`, then the allocator has asserted that the
/// memory block referenced by `ptr` now fits `new_size`, and
/// thus can only be used to carry data of that smaller
/// layout. (The allocator is allowed to take advantage of this,
/// carving off portions of the block for reuse elsewhere.) The
/// truncated contents of the block within the smaller layout are
/// unaltered, and ownership of block has not been transferred.
///
/// If this returns `Err`, then the memory block is considered to
/// still represent the original (larger) `layout`. None of the
/// block has been carved off for reuse elsewhere, ownership of
/// the memory block has not been transferred, and the contents of
/// the memory block are unaltered.
///
/// # Safety
///
/// This function is unsafe because undefined behavior can result
/// if the caller does not ensure all of the following:
///
/// * `ptr` must be currently allocated via this allocator,
///
/// * `layout` must *fit* the `ptr` (see above); note the
/// `new_size` argument need not fit it,
///
/// * `new_size` must not be greater than `layout.size()`
/// (and must be greater than zero),
///
/// # Errors
///
/// Returns `Err(CannotReallocInPlace)` when the allocator is
/// unable to assert that the memory block referenced by `ptr`
/// could fit `layout`.
///
/// Note that one cannot pass `CannotReallocInPlace` to the `oom`
/// method; clients are expected either to be able to recover from
/// `shrink_in_place` failures without aborting, or to fall back
/// on another reallocation method before resorting to an abort.
unsafe fn shrink_in_place_excess(&mut self,
ptr: NonNull<Opaque>,
layout: Layout,
new_size: usize) -> Result<Excess, CannotReallocInPlace> {
let _ = ptr; // this default implementation doesn't care about the actual address.
debug_assert!(new_size <= layout.size);
let (l, u) = self.usable_size(&layout);
// layout.size() <= _u [guaranteed by usable_size()]
// new_layout.size() <= layout.size() [required by this method]
if l <= new_size {
return Ok(Excess(ptr, u));
} else {
return Err(CannotReallocInPlace);
Copy link
Member

Choose a reason for hiding this comment

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

How can we end up in this situation? The allocation can always fit less data than it already contains, right?

Copy link
Contributor Author

@gnzlbg gnzlbg May 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

There are two issues that interact here:

  • the user is not allowed to call dealloc with any size, but the size used must be in the range returned by usable_size -> (l, u). Sizes in that range must not invoke undefined behavior when passed to dealloc.

  • the only requirement on the sizes (l, u) returned by usable_size is that l <= u. Therefore, a valid usable_size implementation is: usable_size() -> (u, u).

Therefore, we can end here if the lower bound on the allocation size l returned by usable_size -> (l, u) is larger than new_size, which happens if usable_size() -> (u, u) and new_size < u.

If we were to return Ok in this case, the user would be allowed to call dealloc with new_size, but since that's out of the [l, u] range, that would trigger undefined behavior when calling dealloc.

Does that make sense?

FWIW I think that the current API of shrink_in_place is a bit broken. For example, malloc does not care about the allocation size on deallocation, so a valid implementation of usable_size for malloc is to return (0, size). This would allow implementing shrink_in_place / shrink_in_place_excess for malloc by just doing nothing and returning Ok, but I don't think that's a useful implementation of shrink_in_place. Even if it were to return Ok only if it was able to shrink the allocation, that might not be useful either. If I have a 1Gb allocation, and want to shrink it in place to 100 Mb, and shrink_in_place only shrinks it by 1 byte, then that's not useful.

The wording that is problematic is that shrink_in_place is allowed to return more size than the one requested, which means that it can just return the original size and do nothing.

For grow_in_place this wording makes sense, but it seems that it was c&p for shrink_in_place. So either shrink_in_place should always return the allocation size so that the user can check whether it shrunk the allocation enough, or it should take a requested size and an upper bound, and return error if it can't shrink the allocation under the upper bound.

Also while I understand the rationale behind splitting realloc_in_place into {grow,shrink}_in_place, I am starting to doubt whether that was the right call. A single realloc_in_place method would reduce API surface and simplify things even if it adds a branch for allocators that can have completely different execution paths for growing and shrinking.

In any case all of this must be discussed in the RFC issue, and not here, but those are my thoughts on the matter.

}
}

// == COMMON USAGE PATTERNS ==
// alloc_one, dealloc_one, alloc_array, realloc_array. dealloc_array
Expand Down