Optimize dependency file search#153131
Conversation
|
Not expecting any big wins there, but let's check: @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Optimize dependency file search
This comment has been minimized.
This comment has been minimized.
|
Unfortunately I am unable to test because of #153077, I am unable to get rustc to build. |
|
Finished benchmarking commit (71d4456): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (secondary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 480.132s -> 480.258s (0.03%) |
|
OK, got some data: With this PR: Without: The tests are super noisy, though (I get significantly different results between runs). But it seems like a pretty good win. |
|
Nice wins. @bors r+ |
| Err(..) => Default::default(), | ||
| }; | ||
| files.sort_by(|lhs, rhs| lhs.file_name_str.cmp(&rhs.file_name_str)); | ||
| files.sort_unstable_by(|lhs, rhs| lhs.file_name_str.cmp(&rhs.file_name_str)); |
There was a problem hiding this comment.
Is there no possibility of two files with the same file_name_str?
There was a problem hiding this comment.
Although I guess that would result in either one being picked deterministically (if it is the only matching one), which one is picked not mattering (if both files having the same crate hash) or an error about multiple candidates getting emitted.
There was a problem hiding this comment.
We sorted (and searched) by the UTF-8 name before anyway, so there shouldn't hopefully be any change in behavior.
This comment has been minimized.
This comment has been minimized.
Optimize dependency file search I tried to look into the slowdown reported in rust-lang/cargo#16665. I created a Rust hello world program, and used this Python script to create a directory containing 200k files: ```python from pathlib import Path dir = Path("deps") dir.mkdir(parents=True, exist_ok=True) for i in range(200000): path = dir / f"file{i:07}.o" with open(path, "w") as f: f.write("\n") ``` Then I tried to do various small microoptimalizations and simplifications to the code that iterates the search directories. Each individual commit improved performance, with the third one having the biggest effect. Here are the results on `main` vs the last commit with the stage1 compiler on Linux, using `hyperfine "rustc +stage1 src/main.rs -L deps" -r 30` (there's IO involved, so it's good to let it run for a while): ```bash Benchmark 1: rustc +stage1 src/main.rs -L deps Time (mean ± σ): 299.4 ms ± 2.7 ms [User: 161.9 ms, System: 144.9 ms] Range (min … max): 294.8 ms … 307.1 ms 30 runs Benchmark 1: rustc +stage1 src/main.rs -L deps Time (mean ± σ): 208.1 ms ± 4.5 ms [User: 87.3 ms, System: 128.7 ms] Range (min … max): 202.4 ms … 219.6 ms 30 runs ``` Would be cool if someone could try this on macOS (maybe @ehuss - not sure if you have macOS or you only commented about its behavior on the Cargo issue :) ). I also tried to prefilter the paths (not in this PR); right now we load everything and then we filter files with given prefixes, that's wasteful. Filtering just files starting with `lib` would get us down to ~150ms here. (The baseline without `-L` is ~80ms on my PC). The rest of the 70ms is essentially allocations from iterating the directory entries and sorting. That would be very hard to change - iterating the directory entries (de)allocates a lot of intermediate paths :( We'd have to implement the iteration by hand with either arena allocation, or at least some better management of memory. r? @nnethercote
|
💔 Test for 07af97d failed: CI. Failed job:
|
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
@bors retry Spurious worker restart. |
This comment has been minimized.
This comment has been minimized.
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 8038127 (parent) -> e7d90c6 (this PR) Test differencesShow 2 test diffs2 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard e7d90c695a39426baf5ae705de2f9570a72229de --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (e7d90c6): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowOur benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (secondary -4.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeResults (secondary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 480.971s -> 480.149s (-0.17%) |
|
The regression seems like noise. Tiny wins on large-workspace, matching pre-merge run. @rustbot label: +perf-regression-triaged |
I tried to look into the slowdown reported in rust-lang/cargo#16665.
I created a Rust hello world program, and used this Python script to create a directory containing 200k files:
Then I tried to do various small microoptimalizations and simplifications to the code that iterates the search directories. Each individual commit improved performance, with the third one having the biggest effect.
Here are the results on
mainvs the last commit with the stage1 compiler on Linux, usinghyperfine "rustc +stage1 src/main.rs -L deps" -r 30(there's IO involved, so it's good to let it run for a while):Would be cool if someone could try this on macOS (maybe @ehuss - not sure if you have macOS or you only commented about its behavior on the Cargo issue :) ).
I also tried to prefilter the paths (not in this PR); right now we load everything and then we filter files with given prefixes, that's wasteful. Filtering just files starting with
libwould get us down to ~150ms here. (The baseline without-Lis ~80ms on my PC). The rest of the 70ms is essentially allocations from iterating the directory entries and sorting. That would be very hard to change - iterating the directory entries (de)allocates a lot of intermediate paths :( We'd have to implement the iteration by hand with either arena allocation, or at least some better management of memory.r? @nnethercote