Skip to content

add a cache to the WfPredicates visitor - #161274

Merged
rust-bors[bot] merged 1 commit into
rust-lang:mainfrom
sjwang05:fast-visit
Aug 22, 2026
Merged

rust-bors[bot] merged 1 commit into
rust-lang:mainfrom
sjwang05:fast-visit

Conversation

@sjwang05

@sjwang05 sjwang05 commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

View all comments

Part of speeding up compiling ReShell with -Znext-solver, see rust-lang/trait-system-refactor-initiative#272 and #t-types/trait-system-refactor > more `reshell` slowness.

The PR changes the WfPredicates visitor to only collect new bounds for unique types. That is, if we have a type like:

type T0 = Map<Then<Whitespace, Whitespace>>
type T1 = Map<Then<T0, T0>>
type T2 = Map<Then<T1, T1>>
...
type T_N = Map<Then<T_N-1, T_N-1>>

etc., the visitor used to end up collecting one WF obligation for each path from T_N to its Whitespace leaves, even though WF of a type (I believe) doesn't depend on the path the visitor took to get there, which allows us to deduplicate by Ty. Not deduplicating caused us to go O(2^N) here.

next-solver is still about ~5x slower than the old solver on the third reproducer due to some other hidden quadratics, and fixing that seems to be more involved, but I think this PR will still be ✨ An Improvement.

r? lcnr

@rustbot

rustbot commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

changes to the core type system

cc @lcnr

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Aug 18, 2026
@rust-log-analyzer

This comment has been minimized.

@sjwang05

This comment was marked as outdated.

@sjwang05 sjwang05 changed the title some misc. next-solver speedups speed up next-solver on some pathological cases Aug 18, 2026
@@ -5,6 +5,7 @@

use std::iter;

use rustc_data_structures::fx::FxHashSet;

@lcnr lcnr Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something something comparing this to DelayedSet 🤔

I would expect WF to mostly be for quite small types

View changes since the review

@lcnr lcnr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these UI tests you've added do compile even without this fix, don't they? They are just really slow. I am worried about using UI tests here as it's hard to deal with them again. I would instead prefer adding these as secondary benchmarks, cc @rust-lang/wg-compiler-performance

View changes since this review

) -> Result<Option<I::Const>, RerunNonErased> {
if self.typing_mode().is_erased_not_coherence() {
match self.opaque_accesses.rerun_always(RerunReason::EvaluateConst)? {}
let resolved = self.resolve_vars_if_possible(alias_const);

@lcnr lcnr Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdonszelmann mind taking this over?

We originally bailed here because we didnt want to deal with the exact behavior of evaluate_const and how it accesses the TypingMode/opaque type storage. also cc https://rust-lang.zulipchat.com/#narrow/channel/260443-project-const-generics/topic/.60evaluate_const.60.20is.20scary/near/617438696 @BoxyUwU @zedddie as we're changing evaluate const to a next-solver only method at which point handling ErasedNonCoherence correctly becomes even easier 🤔

would you mind moving this change out of this PR as the other one feels a lot easier to merge :>

View changes since the review

@panstromek

Copy link
Copy Markdown
Contributor

Let's check whether this is visible on our current benchmarks. If not, then yes, it'd be nice to add new secondary one for this.

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 19, 2026
@rust-bors

This comment has been minimized.

rust-bors Bot pushed a commit that referenced this pull request Aug 19, 2026
speed up next-solver on some pathological cases
@rust-bors

rust-bors Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: 71c2d57 (71c2d570c2e8b858bf22783f39e7bfb4d503968d)
Base parent: f7d782a (f7d782a3be46d6bb4b9792fe69a61db389ba1769)

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (71c2d57): 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 @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never rustc-perf
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
0.3% [0.1%, 0.4%] 48
Regressions ❌
(secondary)
0.3% [0.2%, 0.5%] 39
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.6% [-2.0%, -0.1%] 32
All ❌✅ (primary) 0.3% [0.1%, 0.4%] 48

Max RSS (memory usage)

Results (primary 0.7%, secondary -2.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
2.6% [2.6%, 2.6%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.2% [-1.2%, -1.2%] 1
Improvements ✅
(secondary)
-2.5% [-2.7%, -2.3%] 2
All ❌✅ (primary) 0.7% [-1.2%, 2.6%] 2

Cycles

This perf run didn't have relevant results for this metric.

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 456.256s -> 457.184s (0.20%)
Artifact size: 398.95 MiB -> 398.94 MiB (-0.00%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Aug 19, 2026
@nnethercote

Copy link
Copy Markdown
Contributor

If we do add a secondary benchmark we should do it with intention. We probably have multiple pathological cases for the new trait solver. It would be good to gather them into a single benchmark rather than adding, say, five new tiny benchmarks. The ctfe-stress benchmark is an example where this has been done in the past.

@sjwang05

sjwang05 commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Ooh, interesting perf. For now, I got rid of the 2 uitests and the second commit, and replaced the FxHashSet with a DelayedSet. The const eval PR is here: #161380

@rust-log-analyzer

This comment has been minimized.

@lcnr

lcnr commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 20, 2026
@rust-bors

This comment has been minimized.

rust-bors Bot pushed a commit that referenced this pull request Aug 20, 2026
speed up next-solver on some pathological cases
@rust-bors

rust-bors Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: bd53954 (bd539541bc2d1fed4d588ce4064f9dd86c2b8544)
Base parent: f7d782a (f7d782a3be46d6bb4b9792fe69a61db389ba1769)

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (bd53954): comparison URL.

Overall result: ✅ improvements - no action needed

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.

@bors rollup=never rustc-perf
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.5% [-1.1%, -0.2%] 11
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary -2.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.1% [-2.1%, -2.1%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.1% [-2.1%, -2.1%] 1

Cycles

Results (primary 2.1%, secondary 2.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
2.1% [2.1%, 2.1%] 1
Regressions ❌
(secondary)
2.9% [2.9%, 2.9%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.1% [2.1%, 2.1%] 1

Binary size

Results (primary -0.0%, secondary -0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.0% [-0.1%, -0.0%] 2
Improvements ✅
(secondary)
-0.0% [-0.0%, -0.0%] 1
All ❌✅ (primary) -0.0% [-0.1%, -0.0%] 2

Bootstrap: 456.256s -> 456.614s (0.08%)
Artifact size: 398.95 MiB -> 398.92 MiB (-0.01%)

@rustbot rustbot removed perf-regression Performance regression. S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Aug 20, 2026
@lcnr lcnr changed the title speed up next-solver on some pathological cases add a cache to WfPredicates Aug 21, 2026
@lcnr

lcnr commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

@bors r+

@rust-bors

rust-bors Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

📌 Commit 4a582e4 has been approved by lcnr

It is now in the queue for this repository.

@rust-bors rust-bors Bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 21, 2026
@lcnr lcnr changed the title add a cache to WfPredicates add a cache to the WfPredicates visitor Aug 21, 2026
@rust-bors

This comment has been minimized.

@rust-bors rust-bors Bot added merged-by-bors This PR was explicitly merged by bors. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 22, 2026
@rust-bors

rust-bors Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

☀️ Test successful - CI
Approved by: lcnr
Duration: 2h 56m 49s
Pushing def3ee9 to main...

@rust-bors
rust-bors Bot merged commit def3ee9 into rust-lang:main Aug 22, 2026
15 checks passed
@rustbot rustbot added this to the 1.100.0 milestone Aug 22, 2026
@github-actions

Copy link
Copy Markdown
Contributor
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 8536f4d (parent) -> def3ee9 (this PR)

Test differences

Show 12 test diffs

12 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard def3ee9e4f0fe1429574f402bb624439bbd13fc6 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

  1. optional-x86_64-gnu-autodiff: 33m 37s -> 52m 14s (+55.4%)
  2. x86_64-gnu-stable: 1h 44m -> 2h 32m (+45.7%)
  3. x86_64-gnu-llvm-21: 39m 16s -> 54m 57s (+39.9%)
  4. i686-msvc-1: 2h 3m -> 2h 50m (+38.2%)
  5. x86_64-gnu: 1h 49m -> 2h 30m (+37.5%)
  6. dist-various-1: 53m 8s -> 33m 18s (-37.3%)
  7. x86_64-gnu-gcc-core-tests: 10m 50s -> 14m 36s (+34.8%)
  8. x86_64-gnu-llvm-21-1: 42m 26s -> 57m 6s (+34.6%)
  9. x86_64-msvc-ext2: 1h 47m -> 1h 11m (-33.3%)
  10. pr-check-2: 31m 55s -> 42m 11s (+32.2%)
How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (def3ee9): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.5% [-0.9%, -0.1%] 10
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary -1.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
0.6% [0.6%, 0.6%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-3.0% [-3.0%, -3.0%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.2% [-3.0%, 0.6%] 2

Cycles

Results (primary -2.5%, secondary 2.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.3% [1.6%, 3.3%] 3
Improvements ✅
(primary)
-2.5% [-2.5%, -2.5%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.5% [-2.5%, -2.5%] 1

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 468.334s -> 468.743s (0.09%)
Artifact size: 400.18 MiB -> 400.28 MiB (0.02%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged-by-bors This PR was explicitly merged by bors. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants