Repository navigation
Fancy paramenv accessors that communicate the access pattern better. - #163477
Conversation
6d71f6c to
82b91b0
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
82b91b0 to
b45b7bd
Compare
b45b7bd to
d7aa906
Compare
|
r? project-trait-system-refactor |
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred in cc @BoxyUwU |
This comment has been minimized.
This comment has been minimized.
|
cc @rust-lang/clippy |
This comment has been minimized.
This comment has been minimized.
|
Let me check the perf, just in case 😄 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
d368c73 to
08fe89e
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Fancy paramenv accessors that communicate the access pattern better.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (aa31ab2): comparison URL. Overall result: ❌ regressions - please read:Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=iffy 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 -0.5%, secondary -0.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.7%)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: 480.462s -> 481.851s (0.29%) |
|
smol regressions, unfortunate but I'll keep working on those in subsequent PRs. Without this change more changes are impossible, and I do not wish to ask someone to review this PR together with subtler changes. @bors r=ShoyuVanilla |
|
Interesting. The regressed benchmarks have next-solver counterparts but they aren't regressed 🤔 |
|
yea that makes sense, in the next solver I already in this PR use slightly more specific accessors that probably perform a little better |
…=ShoyuVanilla Fancy paramenv accessors that communicate the access pattern better. r? @lcnr or @rust-lang/initiative-trait-system-refactor This is a single commit, to precede the future change of "compressed trait clauses". It adds a lot of the infrastructure, without making any real changes that may affect performance. As such it forms a separately reviewable step. Even though this isn't on its own such a useful change, it mostly refactors things, it does still make for a nicer API surface. Importantly, with compressed trait clauses (i.e. encoding some common traits as `where self_ty : <bitset of common traits>`), it becomes very important to be careful with how we iterate over caller bounds. I've tried other methods, but the typestate API (as proposed in this PR) is the only one I've found that's both ergonomic, and still allows for implementing compressed trait clauses performantly. > [!NOTE] > I've not used an LLM for any part of this PR, or any other PR I make. This includes any related work like research.
…=ShoyuVanilla Fancy paramenv accessors that communicate the access pattern better. r? @lcnr or @rust-lang/initiative-trait-system-refactor This is a single commit, to precede the future change of "compressed trait clauses". It adds a lot of the infrastructure, without making any real changes that may affect performance. As such it forms a separately reviewable step. Even though this isn't on its own such a useful change, it mostly refactors things, it does still make for a nicer API surface. Importantly, with compressed trait clauses (i.e. encoding some common traits as `where self_ty : <bitset of common traits>`), it becomes very important to be careful with how we iterate over caller bounds. I've tried other methods, but the typestate API (as proposed in this PR) is the only one I've found that's both ergonomic, and still allows for implementing compressed trait clauses performantly. > [!NOTE] > I've not used an LLM for any part of this PR, or any other PR I make. This includes any related work like research.
|
Yeah, I saw them and actually expected that this could be a perf improvement because of them, but looks like we have some subtlties that regressing this. But given that your fine grained accesses negates them and you are going to improve this further the perf regressions feel okayish to me (anyway, we are gonna deprecate the old solver eventually 😁) |
Rollup of 13 pull requests Successful merges: - #163477 (Fancy paramenv accessors that communicate the access pattern better.) - #160593 (turn ABI-controlling target features into a target modifier) - #164896 (offload: pass the linked device image to the host pass) - #165468 (Remove return type from `BuilderMethods::store`) - #159374 (Lower #[no_mangle] to #[symbol_name]/#[link_name]) - #164564 (Re-enable outline-atomics by default on more AArch64 platforms) - #164623 (add next-solver regression tests for two fixed ices) - #164678 (add next-solver regression test for recursive opaque ICE under validate-mir) - #165429 (Add tests for `-Zcrate-attr` edition and position) - #165430 ([tiny] Show context for found assoc item in E0107) - #165456 (std: Fix build for the L4Re targets) - #165463 (Add BTreeMap::allocator and BTreeSet::allocator) - #165466 (Don't "exit" using `panic` on teeos)
Rollup of 15 pull requests Successful merges: - #163477 (Fancy paramenv accessors that communicate the access pattern better.) - #160593 (turn ABI-controlling target features into a target modifier) - #164896 (offload: pass the linked device image to the host pass) - #165468 (Remove return type from `BuilderMethods::store`) - #164564 (Re-enable outline-atomics by default on more AArch64 platforms) - #164623 (add next-solver regression tests for two fixed ices) - #164653 (Deprecate internal function `vec::from_elem`) - #164678 (add next-solver regression test for recursive opaque ICE under validate-mir) - #165387 (windows OpenOptionsExt examples: replace winapi by windows crate) - #165429 (Add tests for `-Zcrate-attr` edition and position) - #165430 ([tiny] Show context for found assoc item in E0107) - #165456 (std: Fix build for the L4Re targets) - #165463 (Add BTreeMap::allocator and BTreeSet::allocator) - #165466 (Don't "exit" using `panic` on teeos) - #165491 (Remove unused compiler types and associated helpers)
Rollup merge of #163477 - jdonszelmann:paramenv-accessors, r=ShoyuVanilla Fancy paramenv accessors that communicate the access pattern better. r? @lcnr or @rust-lang/initiative-trait-system-refactor This is a single commit, to precede the future change of "compressed trait clauses". It adds a lot of the infrastructure, without making any real changes that may affect performance. As such it forms a separately reviewable step. Even though this isn't on its own such a useful change, it mostly refactors things, it does still make for a nicer API surface. Importantly, with compressed trait clauses (i.e. encoding some common traits as `where self_ty : <bitset of common traits>`), it becomes very important to be careful with how we iterate over caller bounds. I've tried other methods, but the typestate API (as proposed in this PR) is the only one I've found that's both ergonomic, and still allows for implementing compressed trait clauses performantly. > [!NOTE] > I've not used an LLM for any part of this PR, or any other PR I make. This includes any related work like research.
|
Note This PR was benchmarked as part of triage of its containing rollup: triage URL. Finished benchmarking commit (68cd6ad): comparison URL. Overall result: ❌ regressions - please read:Our 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)This perf run didn't have relevant results for this metric. CyclesResults (primary 2.7%)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. Artifact size: 406.29 MiB -> 406.33 MiB (0.01%) |
View all comments
r? @lcnr or @rust-lang/initiative-trait-system-refactor
This is a single commit, to precede the future change of "compressed trait clauses". It adds a lot of the infrastructure, without making any real changes that may affect performance. As such it forms a separately reviewable step. Even though this isn't on its own such a useful change, it mostly refactors things, it does still make for a nicer API surface.
Importantly, with compressed trait clauses (i.e. encoding some common traits as
where self_ty : <bitset of common traits>), it becomes very important to be careful with how we iterate over caller bounds. I've tried other methods, but the typestate API (as proposed in this PR) is the only one I've found that's both ergonomic, and still allows for implementing compressed trait clauses performantly.Note
I've not used an LLM for any part of this PR, or any other PR I make. This includes any related work like research.