Avoid copying some undef memory in MIR #62655
Closed
Conversation
This file contains 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
|
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
oli-obk
requested changes
Jul 14, 2019
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
r? @oli-obk |
RalfJung
reviewed
Jul 16, 2019
RalfJung
reviewed
Jul 16, 2019
RalfJung
reviewed
Jul 16, 2019
RalfJung
reviewed
Jul 16, 2019
RalfJung
reviewed
Jul 16, 2019
RalfJung
reviewed
Jul 16, 2019
RalfJung
reviewed
Jul 16, 2019
RalfJung
reviewed
Jul 16, 2019
RalfJung
reviewed
Jul 16, 2019
|
r? @RalfJung |
|
I think (hope, really) that @oli-obk and me will both review this.^^ Not sure why you switched us around here. I also still think it should be investigated why the test program that triggered this consumes 20GB as opposed to 2. There seems to be something deeply wrong somewhere and we shouldn't just paper over that. |
RalfJung
reviewed
Jul 16, 2019
Yes, this might not fix it entirely. The resulting constant in the program is not a |
To land this we should definitely have benchmarks that it helps, at least. Otherwise it seems pointless. |
oli-obk
reviewed
Jul 18, 2019
|
I don't think we need a benchmark. Simply having a compile-pass test with |
build-pass, you mean. :) |
|
I'm going to rename a few parts based on feedback above:
|
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
I think unit tests in rustc that would fail to compile without this PR, but do compile with it totally suffice |
|
@bors r- retry |
bors
added
S-waiting-on-author
|
That |
HeroicKatora
force-pushed
the
elide-uninit-static
branch
from
71b4350
to
c8c4734
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
HeroicKatora
force-pushed
the
elide-uninit-static
branch
from
c8c4734
to
12eb105
Compare
|
☔ The latest upstream changes (presumably #66225) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
S-waiting-on-author
|
@HeroicKatora I'm sorry I didn't see that this PR got updated. Can you please rebase again? Also there's two unresolved comments on the source left about updating comments to represent or explain the current situation. |
|
Ping from triage: |
|
Pinging again from triage: |
|
Was somewhat short on time in the last two weeks, the PR has stretched out for quite a while. I'll get to it this weekend. |
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.
HeroicKatora
force-pushed
the
elide-uninit-static
branch
from
12eb105
to
df72632
Compare
oli-obk
reviewed
Dec 8, 2019
| @@ -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.
Suggested change
| self.initial == false && self.ranges.len() == 1 | |
| // The `ranges` are run-length encoded and of alternating definedness. | |
| // So if `ranges.len() > 1` then the second block is a range of defined bytes. | |
| self.initial == false && self.ranges.len() == 1 |
|
Ping from triage - @HeroicKatora can you address the suggestions from oli-obk? Thanks. |
|
I'm going to close this, I just don't have the time and the effect of this is minimal on |
bors
added a commit
that referenced
this pull request
Dec 30, 2019
Avoid memory copy logic for zsts r? @oli-obk One of the included commits is work done by @HeroicKatora in #62655
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Copying memory between allocations in MIR is not required to preserve the current representation of undef bytes. When the complete source of a copy is in undef state then there are no bytes that need to be copied and the function can return nearly immediately. In some cases this completely avoids writing to the allocation's memory with should have various benefits.
This for example reduces required physical memory when interpreting
const fninvolving large uninitialized values, such asMaybeUninit::uninit().(*This description has been changed from the original one after parts have been integrated into #63561 *)