Repository navigation
New solver opts for diesel - #163981
New solver opts for diesel#163981
diesel#163981Conversation
|
@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.
New solver opts for `diesel`
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (72d027b): comparison URL. Overall result: ❌✅ regressions and improvements - 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 -2.7%, secondary 2.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.4%, secondary -1.1%)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: 484.956s -> 485.101s (0.03%) |
|
LLM disclosure: some of these changes were based on ideas from LLM analysis of profiles. I wrote the code and text myself. |
|
|
|
The perf results show big wins for diesel and smaller wins for some other benchmarks, including some new solver ones. There are very small regressions on |
This comment has been minimized.
This comment has been minimized.
The rust-analyzer impls of these methods will need to be removed on the next sync.
It's faster than `extend` to initialize these structures.
A lot of work can be skipped if the two values being related are equal. (There are various early-return equality checks like this further down the call chain, this just adds one earlier.)
The former has an early return check and a single call site, so the check can be moved before the call site. Also, in the `Err(NoSolution)` case we can skip the `CandidateHeadUsages::default()` stuff because that's just an empty list and `merge_usages` is a no-op.
Instead of getting it within `fast_reject_assumption`. `fast_reject_assumption` has three call sites. For two of them this change doesn't help perf, but the third call occurs in a loop, and this lets us get the goal lang item once per loop instead of once per loop element.
- Increase the number of preinterned type vars and anon bound types. - Preintern some root-universe placeholder types with anon bounds. - Adjust some comments. - Also rename a lot of the preinterning names for more consistency.
072a4a4 to
d1998c0
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
I rebased. @jdonszelmann, this should be good to go. |
There was a problem hiding this comment.
Looking good! Sorry, took the week off. @bors r+ rollup
…zelmann New solver opts for `diesel` A collection of small optimizations. Details in individual commits. Local measurements: ``` solver icounts (B) ------ ----------- old 37.9 new (orig) 56.4 (1.49x) new (this PR) 49.5 (1.31x) ``` Also some small improvements on a range of other benchmarks. r? @jdonszelmann
|
⌛ Testing commit d1998c0 with merge 592e3a1... Workflow: https://github.com/rust-lang/rust/actions/runs/38217070561 |
New solver opts for `diesel` A collection of small optimizations. Details in individual commits. Local measurements: ``` solver icounts (B) ------ ----------- old 37.9 new (orig) 56.4 (1.49x) new (this PR) 49.5 (1.31x) ``` Also some small improvements on a range of other benchmarks. r? @jdonszelmann
|
@bors yield to rollup |
|
Auto build was cancelled. Cancelled workflows: The next pull request likely to be tested is #164193. |
Rollup of 5 pull requests Successful merges: - #163981 (New solver opts for `diesel`) - #164088 (const-check: clarify when and how we can rely on non-UB assumptions) - #164105 (Remove unnecessary indirection around `hir::ConstArg`) - #164181 (Tweak output on unused/unaccessible local macro) - #164186 (Update actions/upload-artifact action to v7.0.2)
|
⌛ Testing commit d1998c0 with merge 23d41dc... Workflow: https://github.com/rust-lang/rust/actions/runs/38219857701 |
New solver opts for `diesel` A collection of small optimizations. Details in individual commits. Local measurements: ``` solver icounts (B) ------ ----------- old 37.9 new (orig) 56.4 (1.49x) new (this PR) 49.5 (1.31x) ``` Also some small improvements on a range of other benchmarks. r? @jdonszelmann
|
@bors yield (spurious) |
|
Auto build was cancelled. Cancelled workflows: The next pull request likely to be tested is #164193. |
Rollup of 5 pull requests Successful merges: - #163981 (New solver opts for `diesel`) - #164088 (const-check: clarify when and how we can rely on non-UB assumptions) - #164105 (Remove unnecessary indirection around `hir::ConstArg`) - #164181 (Tweak output on unused/unaccessible local macro) - #164186 (Update actions/upload-artifact action to v7.0.2)
Rollup merge of #163981 - nnethercote:diesel-opts-1, r=jdonszelmann New solver opts for `diesel` A collection of small optimizations. Details in individual commits. Local measurements: ``` solver icounts (B) ------ ----------- old 37.9 new (orig) 56.4 (1.49x) new (this PR) 49.5 (1.31x) ``` Also some small improvements on a range of other benchmarks. r? @jdonszelmann
|
Note This PR was benchmarked as part of triage of its containing rollup: triage URL. Finished benchmarking commit (bc8e72c): comparison URL. Overall result: ❌✅ regressions and improvements - 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)Results (secondary -2.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -4.0%)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.60 MiB -> 407.61 MiB (0.25%) |
View all comments
A collection of small optimizations. Details in individual commits.
Local measurements:
Also some small improvements on a range of other benchmarks.
r? @jdonszelmann