Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Avoid memory copy logic for zsts #67658

Merged
merged 2 commits into from
Dec 30, 2019
Merged

Avoid memory copy logic for zsts #67658

merged 2 commits into from
Dec 30, 2019

Conversation

spastorino
Copy link
Member

@spastorino spastorino commented Dec 27, 2019 •
edited
Loading

r? @oli-obk

One of the included commits is work done by @HeroicKatora in #62655

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 27, 2019
@spastorino spastorino mentioned this pull request Dec 27, 2019
Merged
@oli-obk
Copy link
Contributor

oli-obk commented Dec 27, 2019

@bors try

@bors
Copy link
Contributor

bors commented Dec 27, 2019

⌛ Trying commit bd93b77 with merge d5da6a798d2c8b458a417747b9a77b048625569f...

@oli-obk
Copy link
Contributor

oli-obk commented Dec 27, 2019

@rust-timer build d5da6a798d2c8b458a417747b9a77b048625569f

@rust-timer
Copy link
Collaborator

Queued d5da6a798d2c8b458a417747b9a77b048625569f with parent 8f5f8f9, future comparison URL.

@bors
Copy link
Contributor

bors commented Dec 27, 2019

☀️ Try build successful - checks-azure
Build commit: d5da6a798d2c8b458a417747b9a77b048625569f (d5da6a798d2c8b458a417747b9a77b048625569f)

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit d5da6a798d2c8b458a417747b9a77b048625569f, comparison URL.

@spastorino spastorino changed the title Avoid memory copy logic for zsts [WIP] Avoid memory copy logic for zsts Dec 27, 2019
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log. 
2019-12-27T18:52:47.8624771Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-12-27T18:52:47.8636518Z ##[command]git config gc.auto 0
2019-12-27T18:52:47.8639285Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-12-27T18:52:47.8641105Z ##[command]git config --get-all http.proxy
2019-12-27T18:52:47.8643723Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67658/merge:refs/remotes/pull/67658/merge

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 @TimNN. (Feature Requests)

@spastorino
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Dec 27, 2019

⌛ Trying commit 474113928df0d1d93d1eab80c74419386917c240 with merge e75db45a11dc03a812447008dd53ad3a87e1afde...

@bors
Copy link
Contributor

bors commented Dec 28, 2019

☀️ Try build successful - checks-azure
Build commit: e75db45a11dc03a812447008dd53ad3a87e1afde (e75db45a11dc03a812447008dd53ad3a87e1afde)

@rust-timer
Copy link
Collaborator

Queued e75db45a11dc03a812447008dd53ad3a87e1afde with parent 74c4e6a, future comparison URL.

@spastorino spastorino changed the title [WIP] Avoid memory copy logic for zsts Avoid memory copy logic for zsts Dec 28, 2019
RalfJung
RalfJung reviewed Dec 28, 2019
View reviewed changes
src/librustc_mir/interpret/memory.rs Outdated

// If `dest_bytes` is empty we just optimize to not run anything for zsts.
// See #67539
if dest_bytes.is_empty() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this even reachable? Shouldn't all_bytes_undef always be true for empty ranges?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compress_undef_range sets initial to self.undef_mask.get(src.offset). If the memory immediately following the range is defined, then it will return true, thus causing all_bytes_undef to return false.

@spastorino
Avoid copying some undef memory in MIR
250a636 
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.
@rust-lang rust-lang deleted a comment from rust-highfive Dec 28, 2019
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit e75db45a11dc03a812447008dd53ad3a87e1afde, comparison URL.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 29, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Dec 29, 2019

📌 Commit 250a636 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 29, 2019
@bors
Copy link
Contributor

bors commented Dec 30, 2019

⌛ Testing commit 250a636 with merge 580ac0b...

bors added a commit that referenced this pull request Dec 30, 2019
@bors
Auto merge of #67658 - spastorino:do-not-copy-zsts, r=oli-obk
580ac0b 
Avoid memory copy logic for zsts

r? @oli-obk

One of the included commits is work done by @HeroicKatora in #62655
@bors
Copy link
Contributor

bors commented Dec 30, 2019

☀️ Test successful - checks-azure
Approved by: oli-obk
Pushing 580ac0b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 30, 2019
@bors bors merged commit 250a636 into rust-lang:master Dec 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

@oli-obk oli-obk oli-obk left review comments

@RalfJung RalfJung RalfJung left review comments

Assignees

@oli-obk oli-obk

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@spastorino @oli-obk @bors @rust-timer @rust-highfive @RalfJung @bjorn3