-
Notifications
You must be signed in to change notification settings - Fork 13k
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
miri: optimize zeroed alloc #136035
base: master
Are you sure you want to change the base?
miri: optimize zeroed alloc #136035
Changes from all commits
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 |
---|---|---|
|
@@ -294,7 +294,12 @@ impl<Prov: Provenance, Bytes: AllocBytes> Allocation<Prov, (), Bytes> { | |
Allocation::from_bytes(slice, Align::ONE, Mutability::Not) | ||
} | ||
|
||
fn uninit_inner<R>(size: Size, align: Align, fail: impl FnOnce() -> R) -> Result<Self, R> { | ||
fn new_inner<R>( | ||
size: Size, | ||
align: Align, | ||
zero_init: bool, | ||
fail: impl FnOnce() -> R, | ||
) -> Result<Self, R> { | ||
// We raise an error if we cannot create the allocation on the host. | ||
// This results in an error that can happen non-deterministically, since the memory | ||
// available to the compiler can change between runs. Normally queries are always | ||
|
@@ -306,7 +311,7 @@ impl<Prov: Provenance, Bytes: AllocBytes> Allocation<Prov, (), Bytes> { | |
Ok(Allocation { | ||
bytes, | ||
provenance: ProvenanceMap::new(), | ||
init_mask: InitMask::new(size, false), | ||
init_mask: InitMask::new(size, zero_init), | ||
align, | ||
mutability: Mutability::Mut, | ||
extra: (), | ||
|
@@ -315,8 +320,8 @@ impl<Prov: Provenance, Bytes: AllocBytes> Allocation<Prov, (), Bytes> { | |
|
||
/// Try to create an Allocation of `size` bytes, failing if there is not enough memory | ||
/// available to the compiler to do so. | ||
pub fn try_uninit<'tcx>(size: Size, align: Align) -> InterpResult<'tcx, Self> { | ||
Self::uninit_inner(size, align, || { | ||
pub fn try_new<'tcx>(size: Size, align: Align, zero_init: bool) -> InterpResult<'tcx, Self> { | ||
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. Bools are a bit icky, as the call sites don't make it clear what the bool means. Maybe an enum with 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. Yeah, I'm not happy with the unlabeled bools. An additional (very simple) possibility could be to use comments – this is used by existing code: 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. Yeah a dedicated |
||
Self::new_inner(size, align, zero_init, || { | ||
ty::tls::with(|tcx| tcx.dcx().delayed_bug("exhausted memory during interpretation")); | ||
InterpErrorKind::ResourceExhaustion(ResourceExhaustionInfo::MemoryExhausted) | ||
}) | ||
|
@@ -328,8 +333,8 @@ impl<Prov: Provenance, Bytes: AllocBytes> Allocation<Prov, (), Bytes> { | |
/// | ||
/// Example use case: To obtain an Allocation filled with specific data, | ||
/// first call this function and then call write_scalar to fill in the right data. | ||
pub fn uninit(size: Size, align: Align) -> Self { | ||
match Self::uninit_inner(size, align, || { | ||
pub fn new(size: Size, align: Align, zero_init: bool) -> Self { | ||
match Self::new_inner(size, align, zero_init, || { | ||
panic!( | ||
"interpreter ran out of memory: cannot create allocation of {} bytes", | ||
size.bytes() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we'll memcopy right over the alloc, zeroing is not needed for realloc, just use false here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The memcopy only initializes part of the allocation, but mremap needs everything to be initialized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I see, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment explaining this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also seems worth saying that we assume here that zeroing the entire allocation is more efficient than zeroing just the parts not copied from the old allocation.