Conversation
|
Are the changes in |
We need to update map from hashes to def indices while encoding |
|
@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.
Remap def indices for deterministic metadata encoding
This comment has been minimized.
This comment has been minimized.
cc58a69 to
9736cd8
Compare
|
Finished benchmarking commit (ceb9839): comparison URL. Overall result: ❌ regressions - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never rustc-perf 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 3.0%, secondary 4.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 10.2%, secondary 14.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.4%, secondary -0.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 496.137s -> 496.27s (0.03%) |
|
@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.
Remap def indices for deterministic metadata encoding
|
|
||
| #[inline] | ||
| fn map_index(&self, index: DefIndex) -> DefIndex { | ||
| self.def_indexes_remapping.get(&index).copied().unwrap_or(index) |
There was a problem hiding this comment.
This could be donewith a vec for better performance. Since we know that these indices are dense in a range. Either a full IndexVec mapping every LocalDefId, or a shorter Vec, that only contains the remapped ones, indexed by remapped_index - min_remapped_index
There was a problem hiding this comment.
It is very strong assumption that def ids that are needed to be remapped will be in the end, now it should be true, because resolution and AST -> HIR lowering is executed without any parallelism, however AST -> HIR lowering became more incremental recently by introducing lower_to_hir query which is very easy to parallelize in the beginning of the run_required_analysis function (now it is executed sequentially as we prefetch hir_crate_items which is a sequential HIR visit). If it is parallelized then from each lower_to_hir execution we may invoke arbitrary code that can allocate def ids that do not need to be remapped.
But I like the idea, I was thinking about something similar in a bit different direction: create separate IndexVecs in Definitions struct for different stages of compilation, thus we can maybe pre-allocate and remap ids only for selected parts.
I think that it is reasonable to track the last def id that is allocated during resolution stage, because it is not likely to get parallelized in the nearest time (I hope so) and then to remap all def ids that are allocated after it, I will try it tomorrow.
| let mut to_remap = vec![]; | ||
| let mut def_ids = vec![]; | ||
| for idx in 0..defs.num_definitions() { | ||
| let def_id = LocalDefId { local_def_index: idx.into() }; |
There was a problem hiding this comment.
I think this could iterate backwards, stopping when it hits an index not needing remapping. Remapped indices should be at the end
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (8bcd37a): comparison URL. Overall result: ❌ regressions - BENCHMARK(S) FAILEDBenchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never rustc-perf ❗ ❗ ❗ ❗ ❗
❗ ❗ ❗ ❗ ❗ 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.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -0.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 496.137s -> 495.635s (-0.10%) |
4f2feb6 to
e01cdf1
Compare
|
@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.
Remap def indices for deterministic metadata encoding
This comment has been minimized.
This comment has been minimized.
|
r? @oli-obk for example (so I'm not the only one who reviews @aerooneqq's PRs) |
|
Finished benchmarking commit (501ad34): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. @bors rollup=never rustc-perf 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 -2.3%, secondary 0.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 4.4%, secondary 4.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 500.132s -> 498.753s (-0.28%) |
|
☔ The latest upstream changes (presumably #162920) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
View all comments
Now at the very early stage (however fixes corresponding issues), draft primarily for CI.
cc #t-compiler/const-eval > DefId reproducibility with the parallel frontend
r? @petrochenkov