Skip to content

Reduce next-solver memory usage by interning CanonicalQueryInput - #162031

Merged
rust-bors[bot] merged 3 commits into
rust-lang:mainfrom
laundmo:opt-next-solver-memory
Sep 2, 2026
Merged

Reduce next-solver memory usage by interning CanonicalQueryInput#162031
rust-bors[bot] merged 3 commits into
rust-lang:mainfrom
laundmo:opt-next-solver-memory

Conversation

@laundmo

@laundmo laundmo commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

View all comments

Goal: Reduce memory usage of next-solver especially when exceeding recursion limit

Changes:

  • Added Interner::CanonicalInput associated type and surrounding interning machinery (mostly copied from ExternalConstraints)
  • Changed Cx::Input to be I:CanonicalInput and adjusted other places in the code to match, including calling the interning during canonicalize_goal

Results:
According to Heaptrack (and btop) peak memory usage for compiling bevy_render (like #161748) with next-solver enabled was reduced from ~15GiB to ~4GiB since most of it was the HashMap index for NestedGoals (and search_graph::GlobalCache), which used to be 56 bytes (IIRC), but now is a single reference thanks to Interned.

Closes (maybe): #161748
Tracking issue: rust-lang/goals#113 and #160895 (comment)

r? @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 30, 2026
@rustbot

rustbot commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the pull request, and welcome! The Rust Project has assigned @lcnr (or someone else) to review your changes, you should hear from them (or someone else) within the next two weeks.

Please see the contribution instructions and our LLM policy for more information.

@ShoyuVanilla

Copy link
Copy Markdown
Member

@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 31, 2026
@rust-bors

This comment has been minimized.

rust-bors Bot pushed a commit that referenced this pull request Aug 31, 2026
Reduce next-solver memory usage by interning CanonicalQueryInput
@rust-bors

rust-bors Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: 9d1834c (9d1834c69e38df691a50a3052833ce4cb83252c2)
Base parent: 5321a4f (5321a4f40c957cf3587c055e77461febc2ebc865)

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (9d1834c): 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
Regressions ❌
(secondary)
0.2% [0.1%, 0.2%] 8
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.4% [-0.4%, -0.4%] 3
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary 1.1%, secondary 0.9%)

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

mean range count
Regressions ❌
(primary)
1.1% [1.1%, 1.1%] 1
Regressions ❌
(secondary)
6.7% [4.0%, 10.6%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.5% [-2.3%, -0.7%] 7
All ❌✅ (primary) 1.1% [1.1%, 1.1%] 1

Cycles

Results (primary -1.9%, secondary 10.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)
10.0% [10.0%, 10.0%] 1
Improvements ✅
(primary)
-1.9% [-1.9%, -1.9%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.9% [-1.9%, -1.9%] 1

Binary size

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

Bootstrap: 477.635s -> 478.315s (0.14%)
Artifact size: 402.71 MiB -> 402.95 MiB (0.06%)

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

impl<'tcx> QueryKey for (crate::traits::solve::CanonicalInput<'tcx>, usize) {

@lcnr lcnr Sep 1, 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.

that means you can remove the existing QueryKey for (CanonicalQueryInput<'tcx, T>, bool) impls? 🤔

View changes since the review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, they're generic over T, while the newly added ones are not. Removing them causes many errors.

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.

what other query takes a tuple involving bool/usize? I do expect that we need the non-tuple one, but the other 2 surprise me

Comment thread compiler/rustc_middle/src/traits/solve.rs Outdated
Comment thread compiler/rustc_middle/src/traits/solve.rs
Comment thread compiler/rustc_type_ir/src/solve/mod.rs Outdated
Comment thread compiler/rustc_middle/src/ty/context/impl_interner.rs Outdated

@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.

think we should merge this and then go from there to remove the N^2 behavior

View changes since this review

@laundmo

laundmo commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

I'm not so sure anymore. I've been considering more and i think the fact NestedGoals stores a reference now is a drawback over, for example, storing CanonicalInputs in a Vec and handing out indices, kinda like you mentioned in the zulip at some point.

To me it seems likely that's the direction where other improvements can be made.

@lcnr

lcnr commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

This PR feels small enough for us to easily revert this as part of a PR using indices, and using indices likely takes more than a few days, so preventing people compiling bevy from getting an OOM is probably worth it anyways

@laundmo

laundmo commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

@lcnr i resolved your reviews, what should we do about the perf regressions?


pub type CanonicalInput<I, T = <I as Interner>::Predicate> =
ty::CanonicalQueryInput<I, QueryInput<I, T>>;
pub type CanonicalInputData<I> =

@lcnr lcnr Sep 1, 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.

oh wait, CanonicalInputData is not a wrapper type, it's just the name of a type alias?

🤔 want to instead do what we do for I::Probe which is just a &'tcx inspect::Probe? don't think we necessarily need a new type here

View changes since the review

@laundmo laundmo Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It was a type alias before this whole PR too, then i made a wrapper around the type alias, then you reviewed asking if the wrapper needed to exist, i noticed it didn't, and removed it. kept the new name to not be ambiguous in other places, tho.

@lcnr

lcnr commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

I do think we should be able to remove some QueryKey impls, but that's fine and can happen separately

@bors r+ rollup=never

@rust-bors

rust-bors Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

📌 Commit 4189938 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 Sep 2, 2026
@JonathanBrouwer

Copy link
Copy Markdown
Member

@bors rollup=iffy
Creating a rollup of perf-sensitive PRs

rust-bors Bot pushed a commit that referenced this pull request Sep 2, 2026
…uwer

Rollup of 3 perf-sensitive pull requests

Successful merges:

 - #161850 (Store LiveLoans more densely packed)
 - #162031 (Reduce next-solver memory usage by interning CanonicalQueryInput)
 - #162047 (Optimize empty token streams)
@rust-bors
rust-bors Bot merged commit 604a1ee into rust-lang:main Sep 2, 2026
13 checks passed
@rustbot rustbot added this to the 1.100.0 milestone Sep 2, 2026
rust-bors Bot pushed a commit that referenced this pull request Sep 2, 2026
Rollup merge of #162031 - laundmo:opt-next-solver-memory, r=lcnr

Reduce next-solver memory usage by interning CanonicalQueryInput

Goal: Reduce memory usage of next-solver especially when exceeding recursion limit

Changes:
- Added Interner::CanonicalInput associated type and surrounding interning machinery (mostly copied from ExternalConstraints)
- Changed Cx::Input to be I:CanonicalInput and adjusted other places in the code to match, including calling the interning during canonicalize_goal

Results:
According to Heaptrack (and btop) peak memory usage for compiling `bevy_render` (like #161748) with next-solver enabled was reduced from ~15GiB to ~4GiB since most of it was the HashMap index for NestedGoals (and search_graph::GlobalCache), which used to be 56 bytes (IIRC), but now is a single reference thanks to Interned.

Closes (maybe): #161748
Tracking issue: rust-lang/goals#113 and #160895 (comment)

r? @lcnr
@rust-timer

Copy link
Copy Markdown
Collaborator

Note

This PR was benchmarked as part of triage of its containing rollup: triage URL.

Finished benchmarking commit (cd59165): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

  • If the regression was expected or you think it can be justified,
    please write a comment with sufficient written justification, and add
    @rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
  • If you think that you know of a way to resolve the regression, try to create
    a new PR with a fix for the regression.
  • If you do not understand the regression or you think that it is just noise,
    you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
    were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.2% [0.1%, 0.3%] 5
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.3% [-0.5%, -0.0%] 6
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (secondary -1.6%)

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
Improvements ✅
(secondary)
-1.6% [-2.3%, -0.8%] 6
All ❌✅ (primary) - - 0

Cycles

Results (secondary -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
Regressions ❌
(secondary)
10.4% [9.1%, 12.7%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-5.5% [-8.8%, -2.2%] 8
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: missing data
Artifact size: 400.83 MiB -> 400.94 MiB (0.03%)

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

Labels

perf-regression Performance regression. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

6 participants