Resolve: more preperation work for parallelizing the import resolution loop - #159440
Conversation
This comment has been minimized.
This comment has been minimized.
0d9f4a2 to
aa051d9
Compare
|
I double-checked with @rustbot ready |
This comment has been minimized.
This comment has been minimized.
aa051d9 to
2e9c9d2
Compare
| self.write_import_resolutions(&to_resolve_imports); | ||
|
|
||
| self.indeterminate_imports = to_resolve_imports | ||
| .extract_if(.., |(_, _, count)| { |
There was a problem hiding this comment.
It would be nicer to extract_if into determined_imports instead and then do self.indeterminate_imports = imports_to_resolve;.
But it may be slower due to majority of the elements in the vector being determinate.
I'll benchmark it separately later after this PR lands.
There was a problem hiding this comment.
I would expect it to be faster, now that I thought about more clearly.
Currently we extract all indeterminate imports, which will leave small holes in the vector and thus requiring a lot of chunks to be moved. And then we still have to iterate over the vector such that we only get the import itself.
Extracting into determined imports is the same kind of map as we have now, with the occasional skip of an undetermined import. And then we just have a simple assignment. This looks like less work.
…par_slice`. Currently it is all still single threaded, but almost everything needed to make the loop use multithreaded code is done in this commit. For performance reasons we do the resolution now in place instead of recollecting the indeterminate imports ([see benchmark pr](rust-lang#159387) and pr of this commit). Populating external resolution tables now use `Once` to ensure only 1 thread builds the table and all others wait if they need it, this has no impact on single threaded behaviour. `Cache(Ref)Cells` are not tackled yet because they are not needed here (and require extra work).
|
Force pushed to keep it at 1 commit, @rustbot ready. |
2e9c9d2 to
2f3022a
Compare
|
Perf-sensitive change, benchmarked in #159387 (comment). |
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 cde3f8a (parent) -> f65b272 (this PR) Test differencesShow 14 test diffs14 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard f65b272fc92b1e7527dea4a224430a249ab25c2d --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 (f65b272): comparison URL. Overall result: ✅ improvements - no action needed@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 (primary 1.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis perf run didn't have relevant results for this metric. Binary sizeResults (primary -0.0%, secondary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 486.164s -> 487.415s (0.26%) |
View all comments
This is basically #158845 but we do not:
par_slicebecause:CmRefCellto useRwLocks(yet) because of perf reasons.The resolution loop is now in place instead of recollecting indeterminate imports.
r? @petrochenkov