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

Read incremental files on-demand #83036

Closed
wants to merge 6 commits into from
Closed

Conversation

cjgillot
Copy link
Contributor

This should avoid a short-lived memory spike at the beginning of the compilation.

@rust-highfive
Copy link
Collaborator

r? @davidtwco

(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 11, 2021
@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Member

jyn514 commented Mar 12, 2021

cc @tgnottingham

@cjgillot
Copy link
Contributor Author

@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 12, 2021
@bors
Copy link
Contributor

bors commented Mar 12, 2021

⌛ Trying commit 3c25ddf with merge 833254a567948c75829d14b281da174002a6cfd5...

@bors
Copy link
Contributor

bors commented Mar 13, 2021

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

@rust-timer
Copy link
Collaborator

Queued 833254a567948c75829d14b281da174002a6cfd5 with parent b3e19a2, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (833254a567948c75829d14b281da174002a6cfd5): 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 13, 2021
@cjgillot cjgillot force-pushed the filedecoder branch 4 times, most recently from 6672a4e to 296ca4c Compare March 13, 2021 23:54
@cjgillot
Copy link
Contributor Author

@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 14, 2021
@bors
Copy link
Contributor

bors commented Mar 14, 2021

⌛ Trying commit 296ca4c with merge 4937463fe51189b570f25fbd1d948a163a1e89c9...

@bors
Copy link
Contributor

bors commented Mar 14, 2021

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

@rust-timer
Copy link
Collaborator

Queued 4937463fe51189b570f25fbd1d948a163a1e89c9 with parent f293f70, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (4937463fe51189b570f25fbd1d948a163a1e89c9): 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 14, 2021
@joshtriplett
Copy link
Member

This does substantially reduce memory usage for incremental builds, but increases compile times.

This doesn't seem like it inherently needs to be slower. I'm wondering if using the ReadBuf-based interfaces might help here, to avoid repeatedly having to initialize buffers and use resize.

@joshtriplett
Copy link
Member

I'm also wondering if it'd be possible to use BufRead for this, which does already have some of these optimizations (including seek_relative).

@jyn514 jyn514 added A-incr-comp Area: Incremental compilation I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 16, 2021
@bjorn3
Copy link
Member

bjorn3 commented Mar 16, 2021

Could mmap be used here? Maybe with madvise(MADV_SEQUENTIAL) though after #82183 that would be better avoided.

@cjgillot
Copy link
Contributor Author

I would rather avoid using mmap, because it's incompatible with #82780, and because it would be way too unsafe for my taste.
I tried to avoid touching the query result cache in anticipation for #82183.

@bjorn3
Copy link
Member

bjorn3 commented Mar 16, 2021

I switched the metadata reading in cg_clif from fs::read to mmap yesterday. (bjorn3/rustc_codegen_cranelift@b1d14ca) The performance improvements for at least incr-clean were significant. Also constantly seeking is relatively expensive as it needs a syscall every time. A final benefit of mmap is that it allows the kernel to discard the data again when there is little memory left.

@bjorn3
Copy link
Member

bjorn3 commented Mar 16, 2021

Also MAP_PRIVATE as used by memmap2's map_copy_read_only is a bit safer than MAP_SHARED. In addition mmap is used for the metadata reading by cg_llvm anyway.

@cjgillot
Copy link
Contributor Author

Closing in favour of #83214

@cjgillot cjgillot closed this Mar 17, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 22, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 6, 2021
Mmap the incremental data instead of reading it.

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 rust-lang#83036 (comment) by `@bjorn3`
@cjgillot cjgillot deleted the filedecoder branch September 21, 2021 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-incr-comp Area: Incremental compilation I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants