[perf experiment] Split the resolver tables into per-owner tables#138995
[perf experiment] Split the resolver tables into per-owner tables#138995oli-obk wants to merge 4 commits intorust-lang:mainfrom
Conversation
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
[perf experiment] Split the resolver tables into per-owner tables r? `@ghost` just doing some experiments to see if splitting `hir_crate` is feasible by checking if splitting the resolver's output into per-owner queries is feasible (rust-lang#95004) Basically necessary for rust-lang#138705 as that can't be landed perf-wise while the `hir_crate` query is still a thing
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (792af13): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 1.3%, secondary 0.5%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (secondary 1.5%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 777.548s -> 776.554s (-0.13%) |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
[perf experiment] Split the resolver tables into per-owner tables r? `@ghost` just doing some experiments to see if splitting `hir_crate` is feasible by checking if splitting the resolver's output into per-owner queries is feasible (rust-lang#95004) Basically necessary for rust-lang#138705 as that can't be landed perf-wise while the `hir_crate` query is still a thing
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (66f172c): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 1.6%, secondary 2.7%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (secondary 2.9%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 778.99s -> 777.791s (-0.15%) |
|
ok... better, but not great yet either |
|
@bors try @rust-timer queue |
[perf experiment] Split the resolver tables into per-owner tables r? `@ghost` just doing some experiments to see if splitting `hir_crate` is feasible by checking if splitting the resolver's output into per-owner queries is feasible (rust-lang#95004) Basically necessary for rust-lang#138705 as that can't be landed perf-wise while the `hir_crate` query is still a thing
This comment has been minimized.
This comment has been minimized.
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (e12315f): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 0.0%, secondary 1.0%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (secondary 2.5%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 781.035s -> 781.132s (0.01%) |
…etrochenkov Avoid a reverse map that is only used in diagnostics paths r? `@petrochenkov` iterating a map until a value matches and returning the key is bad obviously, but it happens very rarely and only on diagnostics paths. It would also be a lot cheaper with rust-lang#138995. Which is actually why I'm trying this out, that PR adds a new entry in `create_def`, which makes `create_def` show up in cachegrind. So I'm trying out if removing adding an entry in `create_def` is a perf improvement
Rollup merge of rust-lang#139584 - oli-obk:horrible-experiment-1, r=petrochenkov Avoid a reverse map that is only used in diagnostics paths r? `@petrochenkov` iterating a map until a value matches and returning the key is bad obviously, but it happens very rarely and only on diagnostics paths. It would also be a lot cheaper with rust-lang#138995. Which is actually why I'm trying this out, that PR adds a new entry in `create_def`, which makes `create_def` show up in cachegrind. So I'm trying out if removing adding an entry in `create_def` is a perf improvement
|
Ok, so the regressions are now limited to crates that have bazillions of owners
|
|
☔ The latest upstream changes (presumably #139897) made this pull request unmergeable. Please resolve the merge conflicts. |
|
In the same idea, two questions:
|
if we're doing any split, I think I'd prefer to use the same split that HIR uses, so per owner.
I would like this, but I'm fairly certain it will cause perf problems due to hash table accesses being slightly more expensive. It would allow making many hash table accesses be per-item index vecs so maybe it'll pay out. or we take the (likely small) perf hit |
ef6abe7 to
0959015
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.
[perf experiment] Split the resolver tables into per-owner tables
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (225d1a5): 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 (primary 0.8%, secondary 4.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 3.0%, secondary 7.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 480.915s -> 482.851s (0.40%) |
0959015 to
3e8279c
Compare
|
☔ The latest upstream changes (presumably #141295) made this pull request unmergeable. Please resolve the merge conflicts. |
r? @ghost
Basically necessary for #138705 as that can't be landed perf-wise (1% primary, 2% secondary regressions) while the
hir_cratequery is still a thing.My goal is to split the resolver tables into per-owner tables, so that all information that ast lowering needs from the resolver is separated by owners. This should allow us to fully split ast lowering to have one query invocation per owner that steal the individual resolver results for each owner.
Alternatively we keep the entire resolver state around until hir_crate_items has run and steal and drop it then.