-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Take advantage of known-valid-align in layout.rs #99136
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.
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,9 @@ | ||
// Seemingly inconsequential code changes to this file can lead to measurable | ||
// performance impact on compilation times, due at least in part to the fact | ||
// that the layout code gets called from many instantiations of the various | ||
// collections, resulting in having to optimize down excess IR multiple times. | ||
// Your performance intuition is useless. Run perf. | ||
|
||
use crate::cmp; | ||
use crate::fmt; | ||
use crate::mem::{self, ValidAlign}; | ||
|
@@ -85,6 +91,17 @@ impl Layout { | |
unsafe { Ok(Layout::from_size_align_unchecked(size, align)) } | ||
} | ||
|
||
/// Internal helper constructor to skip revalidating alignment validity. | ||
#[inline] | ||
const fn from_size_valid_align(size: usize, align: ValidAlign) -> Result<Self, LayoutError> { | ||
// See above for the correctness of this check. | ||
if size > isize::MAX as usize - (align.as_nonzero().get() - 1) { | ||
return Err(LayoutError); | ||
} | ||
// SAFTEY: as above, this check is sufficient. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pondering: does this need to be duplicated? Could pub const fn from_size_align(size: usize, align: usize) -> Result<Self, LayoutError> {
let align = ValidAlign::try_from(align).map_err(|_| LayoutError)?;
from_size_valid_align(size, align)
} to avoid the repetition? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would be correct, but for the time being I'm erring on the side of making small changes so there's a chance of saying whether a change is positive or not. |
||
Ok(Layout { size, align }) | ||
} | ||
|
||
/// Creates a layout, bypassing all checks. | ||
/// | ||
/// # Safety | ||
|
@@ -126,10 +143,9 @@ impl Layout { | |
#[inline] | ||
pub const fn new<T>() -> Self { | ||
let (size, align) = size_align::<T>(); | ||
// SAFETY: the align is guaranteed by Rust 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. | ||
// SAFETY: if the type is instantiated, rustc already ensures that its | ||
// layout is valid. Use the unchecked constructor to avoid inserting a | ||
// panicking codepath that needs to be optimized out. | ||
unsafe { Layout::from_size_align_unchecked(size, align) } | ||
} | ||
|
||
|
@@ -141,7 +157,6 @@ impl Layout { | |
#[inline] | ||
pub fn for_value<T: ?Sized>(t: &T) -> Self { | ||
let (size, align) = (mem::size_of_val(t), mem::align_of_val(t)); | ||
debug_assert!(Layout::from_size_align(size, align).is_ok()); | ||
// SAFETY: see rationale in `new` for why this is using the unsafe variant | ||
unsafe { Layout::from_size_align_unchecked(size, align) } | ||
} | ||
|
@@ -176,7 +191,6 @@ impl Layout { | |
pub unsafe fn for_value_raw<T: ?Sized>(t: *const T) -> Self { | ||
// SAFETY: we pass along the prerequisites of these functions to the caller | ||
let (size, align) = unsafe { (mem::size_of_val_raw(t), mem::align_of_val_raw(t)) }; | ||
debug_assert!(Layout::from_size_align(size, align).is_ok()); | ||
// SAFETY: see rationale in `new` for why this is using the unsafe variant | ||
unsafe { Layout::from_size_align_unchecked(size, align) } | ||
} | ||
|
@@ -280,8 +294,7 @@ impl Layout { | |
// > less than or equal to `isize::MAX`) | ||
let new_size = self.size() + pad; | ||
|
||
// SAFETY: self.align is already known to be valid and new_size has been | ||
// padded already. | ||
// SAFETY: padded size is guaranteed to not exceed `isize::MAX`. | ||
unsafe { Layout::from_size_align_unchecked(new_size, self.align()) } | ||
} | ||
|
||
|
@@ -304,7 +317,7 @@ impl Layout { | |
let alloc_size = padded_size.checked_mul(n).ok_or(LayoutError)?; | ||
|
||
// The safe constructor is called here to enforce the isize size limit. | ||
Layout::from_size_align(alloc_size, self.align()).map(|layout| (layout, padded_size)) | ||
Layout::from_size_valid_align(alloc_size, self.align).map(|layout| (layout, padded_size)) | ||
} | ||
|
||
/// Creates a layout describing the record for `self` followed by | ||
|
@@ -355,14 +368,14 @@ impl Layout { | |
#[stable(feature = "alloc_layout_manipulation", since = "1.44.0")] | ||
#[inline] | ||
pub fn extend(&self, next: Self) -> Result<(Self, usize), LayoutError> { | ||
let new_align = cmp::max(self.align(), next.align()); | ||
let new_align = cmp::max(self.align, next.align); | ||
let pad = self.padding_needed_for(next.align()); | ||
|
||
let offset = self.size().checked_add(pad).ok_or(LayoutError)?; | ||
let new_size = offset.checked_add(next.size()).ok_or(LayoutError)?; | ||
|
||
// The safe constructor is called here to enforce the isize size limit. | ||
let layout = Layout::from_size_align(new_size, new_align)?; | ||
let layout = Layout::from_size_valid_align(new_size, new_align)?; | ||
Ok((layout, offset)) | ||
} | ||
|
||
|
@@ -383,7 +396,7 @@ impl Layout { | |
pub fn repeat_packed(&self, n: usize) -> Result<Self, LayoutError> { | ||
let size = self.size().checked_mul(n).ok_or(LayoutError)?; | ||
// The safe constructor is called here to enforce the isize size limit. | ||
Layout::from_size_align(size, self.align()) | ||
Layout::from_size_valid_align(size, self.align) | ||
} | ||
|
||
/// Creates a layout describing the record for `self` followed by | ||
|
@@ -397,7 +410,7 @@ impl Layout { | |
pub fn extend_packed(&self, next: Self) -> Result<Self, LayoutError> { | ||
let new_size = self.size().checked_add(next.size()).ok_or(LayoutError)?; | ||
// The safe constructor is called here to enforce the isize size limit. | ||
Layout::from_size_align(new_size, self.align()) | ||
Layout::from_size_valid_align(new_size, self.align) | ||
} | ||
|
||
/// Creates a layout describing the record for a `[T; n]`. | ||
|
@@ -408,7 +421,7 @@ impl Layout { | |
pub fn array<T>(n: usize) -> Result<Self, LayoutError> { | ||
let array_size = mem::size_of::<T>().checked_mul(n).ok_or(LayoutError)?; | ||
// The safe constructor is called here to enforce the isize size limit. | ||
Layout::from_size_align(array_size, mem::align_of::<T>()) | ||
Layout::from_size_valid_align(array_size, ValidAlign::of::<T>()) | ||
} | ||
} | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.