-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Avoid copying some undef memory in MIR #62655
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
Closed
Closed
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next
Next commit
Avoid copying memory representation of undef data
During MIR interpretation it may happen that a place containing uninitialized bytes is copied. This would read the current representation of these bytes and write it to the destination even though they must, by definition, not matter to the execution. This elides that representation change when no bytes are defined in such a copy, saving some cpu cycles. In such a case, the memory of the target allocation is not touched at all which also means that sometimes no physical page backing the memory allocation of the representation needs to be provided by the OS at all, reducing memory pressure on the system.
- Loading branch information
commit e35d56b971f4e1dda8cde421d6de73d27f7a5b0f
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -695,6 +695,12 @@ impl<Tag, Extra> Allocation<Tag, Extra> { | |||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
impl AllocationDefinedness { | ||||||||||
pub fn all_bytes_undef(&self) -> bool { | ||||||||||
self.initial == false && self.ranges.len() == 1 | ||||||||||
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.
Suggested change
|
||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
/// Relocations. | ||||||||||
#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Debug, RustcEncodable, RustcDecodable)] | ||||||||||
pub struct Relocations<Tag = (), Id = AllocId>(SortedMap<Size, (Tag, Id)>); | ||||||||||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -855,6 +855,22 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { | |
let relocations = self.get_raw(src.alloc_id)? | ||
.prepare_relocation_copy(self, src, size, dest, length); | ||
|
||
// Prepare a copy of the undef mask. | ||
let compressed = self.get_raw(src.alloc_id)?.compress_undef_range(src, size); | ||
|
||
if compressed.all_bytes_undef() { | ||
// Fast path: If all bytes are `undef` then there is nothing to copy. The target range | ||
// is marked as undef but we otherwise omit changing the byte representation which may | ||
// be arbitrary for undef bytes. | ||
// This also avoids writing to the target bytes so that the backing allocation is never | ||
// touched if the bytes stay undef for the whole interpreter execution. On contemporary | ||
// operating system this can avoid physically allocating the page. | ||
let dest_alloc = self.get_raw_mut(dest.alloc_id)?; | ||
dest_alloc.mark_definedness(dest, size * length, false); | ||
dest_alloc.mark_relocation_range(relocations); | ||
return Ok(()); | ||
} | ||
|
||
let tcx = self.tcx.tcx; | ||
|
||
// This checks relocation edges on the src. | ||
197g marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
@@ -897,8 +913,10 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { | |
} | ||
} | ||
|
||
// copy definedness to the destination | ||
self.copy_undef_mask(src, dest, size, length)?; | ||
// now copy over the undef data | ||
self.get_raw_mut(dest.alloc_id)? | ||
.mark_compressed_undef_range(&compressed, dest, size, length); | ||
|
||
// copy the relocations to the destination | ||
self.get_raw_mut(dest.alloc_id)?.mark_relocation_range(relocations); | ||
|
||
|
@@ -908,27 +926,6 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { | |
|
||
/// Undefined bytes | ||
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 comment seems entirely inaccurate at this point... |
||
impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { | ||
// FIXME: Add a fast version for the common, nonoverlapping case | ||
fn copy_undef_mask( | ||
&mut self, | ||
src: Pointer<M::PointerTag>, | ||
dest: Pointer<M::PointerTag>, | ||
size: Size, | ||
repeat: u64, | ||
) -> InterpResult<'tcx> { | ||
// The bits have to be saved locally before writing to dest in case src and dest overlap. | ||
assert_eq!(size.bytes() as usize as u64, size.bytes()); | ||
|
||
let src_alloc = self.get_raw(src.alloc_id)?; | ||
let compressed = src_alloc.compress_undef_range(src, size); | ||
|
||
// now fill in all the data | ||
let dest_allocation = self.get_raw_mut(dest.alloc_id)?; | ||
dest_allocation.mark_compressed_undef_range(&compressed, dest, size, repeat); | ||
|
||
Ok(()) | ||
} | ||
|
||
pub fn force_ptr( | ||
&self, | ||
scalar: Scalar<M::PointerTag>, | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
if the
ranges.len()
is not1
, don't we have to iterate to check the other ranges?Uh oh!
There was an error while loading. Please reload this page.
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
ranges
are run-length encoded and of alternating definedness so ifranges.len() > 1
then the second is a range of defined 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.
Could you move this up one block, i.e. right below
struct AllocationDefinedness
?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 please add a comment here explaining what you said in the GH comment.
And what about
len() == 0
?