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.

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

Avoid memory copy logic for zsts #67658

Merged
merged 2 commits into from Dec 30, 2019
Merged

Conversation

@spastorino
Copy link
Member

spastorino commented Dec 27, 2019

r? @oli-obk

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

@spastorino spastorino force-pushed the spastorino:do-not-copy-zsts branch from a8b54e3 to c56a89c Dec 27, 2019
Closes #67539
@spastorino spastorino force-pushed the spastorino:do-not-copy-zsts branch from c56a89c to bd93b77 Dec 27, 2019
@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Dec 27, 2019

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 27, 2019

⌛️ Trying commit bd93b77 with merge d5da6a7...

bors added a commit that referenced this pull request Dec 27, 2019
Avoid memory copy logic for zsts

Closes #67539

r? @oli-obk
@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Dec 27, 2019

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 27, 2019

Queued d5da6a7 with parent 8f5f8f9, future comparison URL.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 27, 2019

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

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 27, 2019

Finished benchmarking try commit d5da6a7, 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

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 27, 2019

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 spastorino force-pushed the spastorino:do-not-copy-zsts branch from 60dc96e to 4741139 Dec 27, 2019
@spastorino

This comment has been minimized.

Copy link
Member Author

spastorino commented Dec 27, 2019

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 27, 2019

Awaiting bors try build completion

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 27, 2019

⌛️ Trying commit 4741139 with merge e75db45...

bors added a commit that referenced this pull request Dec 27, 2019
[WIP] Avoid memory copy logic for zsts

r? @oli-obk
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 28, 2019

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

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 28, 2019

Queued e75db45 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
@spastorino spastorino force-pushed the spastorino:do-not-copy-zsts branch from 4741139 to 8ab0b2b Dec 28, 2019

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

This comment has been minimized.

Copy link
@RalfJung

RalfJung Dec 28, 2019

Member

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

This comment has been minimized.

Copy link
@bjorn3

bjorn3 Dec 28, 2019

Contributor

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 spastorino force-pushed the spastorino:do-not-copy-zsts branch from 8ab0b2b to 27bffab Dec 28, 2019
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.
@spastorino spastorino force-pushed the spastorino:do-not-copy-zsts branch from 27bffab to 250a636 Dec 28, 2019
@rust-lang rust-lang deleted a comment from rust-highfive Dec 28, 2019
@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 28, 2019

Finished benchmarking try commit e75db45, comparison URL.

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Dec 29, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 29, 2019

📌 Commit 250a636 has been approved by oli-obk

@bors

This comment has been minimized.

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
Avoid memory copy logic for zsts

r? @oli-obk

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

This comment has been minimized.

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 label Dec 30, 2019
@bors bors merged commit 250a636 into rust-lang:master Dec 30, 2019
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20191228.26 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.