Mmap the incremental data instead of reading it. by cjgillot · Pull Request #832...
source link: https://github.com/rust-lang/rust/pull/83214
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
Instead of reading the full incremental state using fs::read_file, we memmap it using a private read-only file-backed map.
This allows the system to reclaim any memory we are not using, while ensuring we are not polluted by
outside modifications to the file.
Suggested in #83036 (comment) by @bjorn3
(rust-highfive has picked a reviewer for you, use r? to override)
Awaiting bors try build completion.
@rustbot label: +S-waiting-on-perf
Trying commit This comment has been hidden.
Awaiting bors try build completion.
@rustbot label: +S-waiting-on-perf
Try build successful - checks-actions
Build commit: 50118f3 (50118f3b1ac7884989bbef838e63e6f348478a59)
Finished benchmarking try commit (50118f3): 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
Instruction count regressions up to 0.4%. max-rss wins up to 40% on ctfe-stress and up to 15% excluding ctfe-stress. task-clock improvements up to 35% on ctfe-stress and up to 3.6% excluding ctfe-stress with a few regressions up to 2.8%, which is within noise for task-clock.
This looks good to me! We are already using memory mapped files for crate metadata.
One question: why use memmap2 even though we already have a dependency on memmap? Is the data readonly? In that case we don't need the MmapOptions::map_copy_read_only() -- the MmapOptions::map() call also available in memmap should do the trick.
memmap is no longer maintained. memmap2 is a fork that is still maintained. https://rustsec.org/advisories/RUSTSEC-2020-0077.html
The file should not be written to, but map_copy_read_only is a safe guard that may or may not prevent crashes or other UB when it is written to by another process. The man page says "It is unspecified whether changes made to the file after the mmap() call are visible in the mapped region.". It seems to not help on Linux, but may on other systems.
memmap is no longer maintained. memmap2 is a fork that is still maintained. https://rustsec.org/advisories/RUSTSEC-2020-0077.html
Good to know! I think in that case we should switch the entire codebase.
The file should not be written to, but map_copy_read_only is a safe guard that may or may not prevent crashes or other UB when it is written to by another process. The man page says "It is unspecified whether changes made to the file after the mmap() call are visible in the mapped region.". It seems to not help on Linux, but may on other systems.
I wonder how to interpret this. I suspect that neither map_copy_read_only nor a simple map can safeguard against other processes modifying the underlying file. I guess the only way to protect against the file being modified is to create a copy of it's data somewhere (unless there's a copy-on-write file system underneath -- but we can't assume that). But the creation of our private copy of the file's data is exactly we'd like to avoid with this PR.
I think the current implementation of incr. comp. makes sure that no rustc processes will try to modify files in the cache directory once they are "finalized" but we should review that before merging this PR.
This comment has been hidden.
memmap is no longer maintained. memmap2 is a fork that is still maintained. https://rustsec.org/advisories/RUSTSEC-2020-0077.html
Good to know! I think in that case we should switch the entire codebase.
Filed #83236
The file should not be written to, but map_copy_read_only is a safe guard that may or may not prevent crashes or other UB when it is written to by another process. The man page says "It is unspecified whether changes made to the file after the mmap() call are visible in the mapped region.". It seems to not help on Linux, but may on other systems.
I wonder how to interpret this. I suspect that neither
map_copy_read_onlynor a simplemapcan safeguard against other processes modifying the underlying file. I guess the only way to protect against the file being modified is to create a copy of it's data somewhere (unless there's a copy-on-write file system underneath -- but we can't assume that). But the creation of our private copy of the file's data is exactly we'd like to avoid with this PR.I think the current implementation of incr. comp. makes sure that no
rustcprocesses will try to modify files in the cache directory once they are "finalized" but we should review that before merging this PR.
rustc is not supposed to modify the incremental cache for another instance. This can only happen if the uses starts several rustc/cargo instances at the same moment on the same crate.
During one given session, the dep-graph and work product index are deserialized and the memory map is dropped. The only tricky case is the query result cache, which is kept as-is for the entire compilation. As the memory map needs to be dropped before removing the backing file, the cache promotions are moved to rustc_incremental::persist::save::save_dep_graph before dropping the memmap.
The current state passes tests in Linux. I don't have a Windows box to test on.
rustc is not supposed to modify the incremental cache for another instance. This can only happen if the uses starts several rustc/cargo instances at the same moment on the same crate.
rustc actually should be handle to synchronize even these cases via file locks:
As the memory map needs to be dropped before removing the backing file, the cache promotions are moved to rustc_incremental::persist::save::save_dep_graph before dropping the memmap.
I wonder if we can encode a proper protocol at the type level here: The memmap gets wrapped in a type with a Drop implementation. By default the drop implementation does nothing, but if the compiler decides that there's a proper replacement file, it will (1) drop the memmap and (2) replace the existing file with the replacement. This way we can write all new versions of a given file to a temporary file and then let the Drop impl take care of swapping things out. That would make it less likely that someone write to the old file while the memmap is still live.
I'm not sure how compatible that would be the current setup.
In general I think it would be great if we made sure that it's OK to keep an open memmap to the previous versions of all cache files. That would open up the possibility to encode the dep-graph in a way that does not need any up-front decoding at all and still be lazy about actually loading data from the disk.
OK, let's give it a try.
@bors r+ rollup=never
Commit bcefd48 has been approved by michaelwoerister
I think it would be sensible to
- Merge this PR at the beginning of a cycle so there's enough time to react to errors popping up in the wild, and
Note that we're at the very end of a cycle right now. The beta cutoff is in just a few days. So this change is going almost directly into beta.
@bors r-
Let's wait a few days.
Try build successful - checks-actions
Build commit: 544326c (544326c21b625fcf159686fe790f1672fba194e9)
Beta has branched.
@bors r=michaelwoerister rollup=never
Commit bcefd48 has been approved by michaelwoerister
Test successful - checks-actions
Approved by: michaelwoerister
Pushing 11bbb52 to master...
Finished benchmarking commit (11bbb52): comparison url.
Summary: This benchmark run did not return any relevant changes.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.
@rustbot label: -perf-regression
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK