Skip to content

Add an Mmap wrapper to rustc_data_structures #83682

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

Merged
merged 3 commits into from
Apr 3, 2021

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Mar 30, 2021

This wrapper implements StableAddress and falls back to directly reading the file on wasm32.

Taken from #83640, which I will close due to the perf regression.

This wrapper implements StableAddress and falls back to directly reading
the file on wasm32
@rust-highfive
Copy link
Contributor

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

@rust-highfive
Copy link
Contributor

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 30, 2021
@bjorn3
Copy link
Member Author

bjorn3 commented Mar 30, 2021

I don't think this is the cause of the perf regression in #83640, but just in case.

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 30, 2021
@bors
Copy link
Collaborator

bors commented Mar 30, 2021

⌛ Trying commit 8331dbe with merge 2d883ace5ff221db332b7155122d99e1cb8a3375...

@cjgillot
Copy link
Contributor

Commit 8fcd03e7358ccb8f568ab3b34f3f31fac4b6ede9 also seems interessant to keep.

@bjorn3
Copy link
Member Author

bjorn3 commented Mar 30, 2021

It seemed to be a tiny (<0.1%) regression on the other PR. I can to benchmark after this commit.

@bors
Copy link
Collaborator

bors commented Mar 30, 2021

☀️ Try build successful - checks-actions
Build commit: 2d883ace5ff221db332b7155122d99e1cb8a3375 (2d883ace5ff221db332b7155122d99e1cb8a3375)

@rust-timer
Copy link
Collaborator

Queued 2d883ace5ff221db332b7155122d99e1cb8a3375 with parent 16156fb, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (2d883ace5ff221db332b7155122d99e1cb8a3375): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 30, 2021
@bjorn3
Copy link
Member Author

bjorn3 commented Mar 31, 2021

This seems to be a slight regression on a few benchmarks. Trying with a few methods inlined.

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 31, 2021
@bors
Copy link
Collaborator

bors commented Mar 31, 2021

⌛ Trying commit 5773e51 with merge d95b884a112e6be1fc0a5c178a5404c031f491a8...

@bors
Copy link
Collaborator

bors commented Mar 31, 2021

☀️ Try build successful - checks-actions
Build commit: d95b884a112e6be1fc0a5c178a5404c031f491a8 (d95b884a112e6be1fc0a5c178a5404c031f491a8)

@rust-timer
Copy link
Collaborator

Queued d95b884a112e6be1fc0a5c178a5404c031f491a8 with parent 6ff482b, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (d95b884a112e6be1fc0a5c178a5404c031f491a8): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 31, 2021
@bjorn3
Copy link
Member Author

bjorn3 commented Mar 31, 2021

That seems to be slightly better. Now trying with 8fcd03e7358ccb8f568ab3b34f3f31fac4b6ede9 cherry-picked.

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 31, 2021
@bors
Copy link
Collaborator

bors commented Mar 31, 2021

⌛ Trying commit 88a7e8f2248260f4af7910a0e14e8d672afede9b with merge c6b3d6133f84d4c800af3b32f46663a91f0ab432...

@bors
Copy link
Collaborator

bors commented Mar 31, 2021

☀️ Try build successful - checks-actions
Build commit: c6b3d6133f84d4c800af3b32f46663a91f0ab432 (c6b3d6133f84d4c800af3b32f46663a91f0ab432)

@rust-timer
Copy link
Collaborator

Queued c6b3d6133f84d4c800af3b32f46663a91f0ab432 with parent a5029ac, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (c6b3d6133f84d4c800af3b32f46663a91f0ab432): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@bjorn3
Copy link
Member Author

bjorn3 commented Mar 31, 2021

Nope, that is a regression. Maybe due to the double metadata call by both this code and memmap2? I reverted it again. Ready for review.

@bjorn3 bjorn3 removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 31, 2021
@bjorn3
Copy link
Member Author

bjorn3 commented Apr 3, 2021

Added the safety comment.

@bors r=cjgillot

@bors
Copy link
Collaborator

bors commented Apr 3, 2021

📌 Commit bda6d1f has been approved by cjgillot

@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 Apr 3, 2021
@bors
Copy link
Collaborator

bors commented Apr 3, 2021

⌛ Testing commit bda6d1f with merge 97717a5...

@bors
Copy link
Collaborator

bors commented Apr 3, 2021

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 97717a5 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 3, 2021
@bors bors merged commit 97717a5 into rust-lang:master Apr 3, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 3, 2021
@bjorn3 bjorn3 deleted the mmap_wrapper branch April 3, 2021 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

7 participants