Skip to content

Commit

Permalink
core: Remove panics from some Layout methods
Browse files Browse the repository at this point in the history
`Layout` is often used at the core of allocation APIs and is as a result pretty
sensitive to codegen in various circumstances. I was profiling `-C opt-level=z`
with a wasm project recently and noticed that the `unwrap()` wasn't removed
inside of `Layout`, causing the program to be much larger than it otherwise
would be. If inlining were more aggressive LLVM would have figured out that the
panic could be eliminated, but in general the methods here can't panic in the
first place!

As a result this commit makes the following tweaks:

* Removes `unwrap()` and replaces it with `unsafe` in `Layout::new` and
  `Layout::for_value`. For posterity though a debug assertion was left behind.
* Removes an `unwrap()` in favor of `?` in the `repeat` method. The comment
  indicating that the function call couldn't panic wasn't quite right in that if
  `alloc_size` becomes too large and if `align` is high enough it could indeed
  cause a panic.

This'll hopefully mean that panics never get introduced into code in the first
place, ensuring that `opt-level=z` is closer to `opt-level=s` in this regard.
  • Loading branch information
alexcrichton committed Apr 12, 2018
1 parent 67712d7 commit ec3bccb
Showing 1 changed file with 14 additions and 8 deletions.
22 changes: 14 additions & 8 deletions src/libcore/heap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,15 +125,26 @@ impl Layout {
/// Constructs a `Layout` suitable for holding a value of type `T`.
pub fn new<T>() -> Self {
let (size, align) = size_align::<T>();
Layout::from_size_align(size, align).unwrap()
// Note that the align is guaranteed by rustc to be a power of two and
// the size+align combo is guaranteed to fit in our address space. As a
// result use the unchecked constructor here to avoid inserting code
// that panics if it isn't optimized well enough.
debug_assert!(Layout::from_size_align(size, align).is_some());
unsafe {
Layout::from_size_align_unchecked(size, align)
}
}

/// Produces layout describing a record that could be used to
/// allocate backing structure for `T` (which could be a trait
/// or other unsized type like a slice).
pub fn for_value<T: ?Sized>(t: &T) -> Self {
let (size, align) = (mem::size_of_val(t), mem::align_of_val(t));
Layout::from_size_align(size, align).unwrap()
// See rationale in `new` for why this us using an unsafe variant below
debug_assert!(Layout::from_size_align(size, align).is_some());
unsafe {
Layout::from_size_align_unchecked(size, align)
}
}

/// Creates a layout describing the record that can hold a value
Expand Down Expand Up @@ -212,12 +223,7 @@ impl Layout {
pub fn repeat(&self, n: usize) -> Option<(Self, usize)> {
let padded_size = self.size.checked_add(self.padding_needed_for(self.align))?;
let alloc_size = padded_size.checked_mul(n)?;

// We can assume that `self.align` is a power-of-two.
// Furthermore, `alloc_size` has already been rounded up
// to a multiple of `self.align`; therefore, the call to
// `Layout::from_size_align` below should never panic.
Some((Layout::from_size_align(alloc_size, self.align).unwrap(), padded_size))
Some((Layout::from_size_align(alloc_size, self.align)?, padded_size))
}

/// Creates a layout describing the record for `self` followed by
Expand Down

0 comments on commit ec3bccb

Please sign in to comment.