Improve how `QueryCache`/`QueryState`/`QueryEngine` are stored by nnethercote · Pull Request #152835 · rust-lang/rust

nnethercote · 2026-02-19T03:38:04Z

QueryVTable has two fields query_state and query_cache that are byte offsets of fields in other structs. They are used in unsafe combination with byte_add and cast to access said fields. I don't like this at all and this PR replaces them with something sensible.

r? @Zalathar

rustbot · 2026-02-19T03:38:09Z

Zalathar is not on the review rotation at the moment.
They may take a while to respond.

Zalathar · 2026-02-19T03:50:57Z

@bors try @rust-timer queue

Improve how `QueryCache`s and `QueryState`s are stored

Zalathar · 2026-02-19T04:01:26Z

Putting the state and cache directly in the vtable is not a possibility I had considered.

It stretches the concept of “vtable” a bit, and it gives up on being able to treat the vtable as mere metadata, but it does have advantages for actually looking up the state/cache given a vtable reference.

Genuinely unsure how to feel about this overall. It doesn’t jump out to me as something obviously-good or obviously-bad; the tradeoffs are more subtle.

nnethercote · 2026-02-19T04:33:36Z

I agree about stretching the meaning of "vtable".

Beyond that I think it's a clear win. The offset code is ridiculous, it screams "something went wrong here".

More generally, it makes more sense to have a struct containing (A, B, C) x 400 than having (A x 400, B x 400, C x 400) when A and B and C are closely related and used together.

Zalathar · 2026-02-19T05:10:03Z

This PR also touches on some areas that I haven’t been able to work on because they’re still waiting for #152747 to be merged, so having this PR also interfere with that is starting to feel overwhelming.

rust-bors · 2026-02-19T06:02:31Z

☀️ Try build successful (CI)
Build commit: 6efacd9 (6efacd91cd7cd06fc2c44d60f80077051f2fca7d, parent: e0cb264b814526acb82def4b5810e394a2ed294f)

rust-timer · 2026-02-19T06:42:35Z

Finished benchmarking commit (6efacd9): comparison URL.

Overall result: ✅ improvements - no action needed

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

@bors rollup=never
@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.3%	[-0.5%, -0.1%]	10
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

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

Cycles

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)	2.1%	[2.1%, 2.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.1%	[2.1%, 2.1%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 487.064s -> 482.434s (-0.95%)
Artifact size: 397.85 MiB -> 397.80 MiB (-0.01%)

nnethercote · 2026-02-19T07:02:34Z

QueryEngine also merges into QueryVTable really nicely. I've added a new commit doing that.

I've been asking you for a lot of reviews; I'm happy to ask a different reviewer if you want a break.

Zalathar · 2026-02-20T10:36:25Z

I was skeptical of this PR at first, because it was unclear to me whether moving state/cache into the vtable was a good idea overall, and I wasn't convinced by the motivation of wanting to get rid of the unsafe offset fields (even though I don't like the offset fields).

However, after some more consideration, I'm coming around to the idea that putting per-query state/caches in the vtable is a fine idea in its own right. So now I'm pretty happy with the direction of this PR.

I have some remarks to write up, and I'll want to do another pass, but overall this PR looks good to me.

compiler/rustc_query_impl/src/plumbing.rs

compiler/rustc_middle/src/query/inner.rs

compiler/rustc_query_impl/src/plumbing.rs

Zoxc · 2026-02-21T11:59:53Z

Pointers to fields can be done safely using say field-offset. I don't see much downside having the cache/state in QueryVTable besides preventing them from being 'static.

nnethercote · 2026-02-22T08:34:40Z

Pointers to fields can be done safely using say field-offset

Even so, that's (a) a new third-party crate, and (b) it's very non-idiomatic and strongly suggests a suboptimal data layout.

nnethercote · 2026-02-22T08:38:05Z

I have updated. Thanks for the great suggestions!

Zalathar · 2026-02-23T03:08:06Z

compiler/rustc_middle/src/query/plumbing.rs

    /// Used when reporting query cycle errors and similar problems.
    pub description_fn: fn(TyCtxt<'tcx>, C::Key) -> String,
+
+    pub execute_query_fn: fn(TyCtxt<'tcx>, Span, C::Key, QueryMode) -> Option<C::Value>,


Remark (pre-existing): This field could really use some docs and maybe a better name, because what it does and doesn't do is fairly subtle. But that's a pre-existing concern, so I don't want to block this PR on it.

If you want to suggest a comment I can drop it in.

I don’t have something prepared, but I’ll add it to my TODO list to come back and look at this for a follow-up PR.

Zalathar · 2026-02-23T03:09:46Z

Looking at this PR again, I realised that it's going to need some minor fixups after the dispatcher→vtable changes in #152791, which should combine for some nice little simplifications.

rustbot · 2026-02-24T00:35:00Z

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.

nnethercote · 2026-02-24T00:36:41Z

@Zalathar: I have updated for the #152791 merge. Should be ready for the final review, thanks.

Zalathar · 2026-02-24T01:28:50Z

Commit message nit: The first commit no longer needs to mention SemiDynamicQueryDispatcher. 🎉

Zalathar · 2026-02-24T01:33:52Z

r=me with commit message nit addressed.

`QuerySystem` has these fields: ``` pub states: QueryStates<'tcx>, pub caches: QueryCaches<'tcx>, pub query_vtables: PerQueryVTables<'tcx>, ``` Each one has an entry for each query. Some methods that take a query-specific `QueryVTable` need to access the corresponding query-specific states and/or caches. So `QueryVTable` stores the *byte offset* to the relevant fields within `states` and `caches`, and uses that to (with `unsafe`) access the fields. This is bizarre, and I hope it made sense in the past when the code was structured differently. This commit moves `QueryState` and `QueryCache` inside `QueryVTable`. As a result, the query-specific states and/or caches are directly accessible, and no unsafe offset computations are required. Much simpler and more normal. Also, `QueryCaches` and `QueryStates` are no longer needed, which makes `define_callbacks!` a bit simpler. The commit also rename the fields and their getters to `states` and `caches`.

Currently type variables that impl `QueryCache` are called either `C` or `Cache`. I find the former clearer because single char type variables names are very common and longer type variable names are easy to mistake for type or trait names -- e.g. I find the trait bound `C: QueryCache` much easier and faster to read than `Cache: QueryCache`.

`QueryEngine` is a struct with one function pointer per query. This commit removes it by moving each function pointer into the relevant query's vtable, which is already a collection of function pointers for that query. This makes things simpler.

The `Per` isn't really necessary.

nnethercote · 2026-02-24T02:02:30Z

I fixed the comment.

@bors r=Zalathar

rust-bors · 2026-02-24T02:02:33Z

📌 Commit 7df5249 has been approved by Zalathar

It is now in the queue for this repository.

bjorn3 · 2026-02-24T06:58:56Z

Pointers to fields can be done safely using say field-offset.

We used to use that crate before #136201, but removed it as it is not compatible with strict provenance.

rust-bors · 2026-02-24T19:03:39Z

☔ The latest upstream changes (presumably #153047) made this pull request unmergeable. Please resolve the merge conflicts.

This pull request was unapproved.

rustbot assigned Zalathar Feb 19, 2026

This comment has been minimized.

Sign in to view

rust-bors bot pushed a commit that referenced this pull request Feb 19, 2026

Auto merge of #152835 - nnethercote:fix-query-state-query-cache, r=<try>

6efacd9

Improve how `QueryCache`s and `QueryState`s are stored

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

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 19, 2026

This comment has been minimized.

Sign in to view

Zalathar reviewed Feb 20, 2026

View reviewed changes

compiler/rustc_query_impl/src/plumbing.rs Outdated Show resolved Hide resolved

Zalathar reviewed Feb 20, 2026

View reviewed changes

compiler/rustc_query_impl/src/plumbing.rs Outdated Show resolved Hide resolved

Zalathar reviewed Feb 20, 2026

View reviewed changes

compiler/rustc_middle/src/query/inner.rs Show resolved Hide resolved

Zalathar reviewed Feb 20, 2026

View reviewed changes

compiler/rustc_query_impl/src/plumbing.rs Outdated Show resolved Hide resolved

nnethercote force-pushed the fix-query-state-query-cache branch from bc792c2 to d13ad56 Compare February 22, 2026 08:37

This comment has been minimized.

Sign in to view

nnethercote force-pushed the fix-query-state-query-cache branch from d13ad56 to 271ad55 Compare February 23, 2026 00:15

This comment has been minimized.

Sign in to view

Zalathar reviewed Feb 23, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

nnethercote force-pushed the fix-query-state-query-cache branch from 271ad55 to f362d0e Compare February 24, 2026 00:34

nnethercote changed the title ~~Improve how QueryCaches and QueryStates are stored~~ Improve how QueryCache/QueryState/QueryEngine are stored Feb 24, 2026

nnethercote added 4 commits February 24, 2026 13:01

Merge QueryEngine into QueryVTable.

167aee2

`QueryEngine` is a struct with one function pointer per query. This commit removes it by moving each function pointer into the relevant query's vtable, which is already a collection of function pointers for that query. This makes things simpler.

Rename PerQueryVTables as QueryVTables.

7df5249

The `Per` isn't really necessary.

nnethercote force-pushed the fix-query-state-query-cache branch from f362d0e to 7df5249 Compare February 24, 2026 02:01

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 Feb 24, 2026

rust-bors bot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 24, 2026

Uh oh!

Comments

Conversation

nnethercote commented Feb 19, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Feb 19, 2026

Uh oh!

Zalathar commented Feb 19, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

Zalathar commented Feb 19, 2026

Uh oh!

nnethercote commented Feb 19, 2026

Uh oh!

Zalathar commented Feb 19, 2026

Uh oh!

rust-bors bot commented Feb 19, 2026

Uh oh!

This comment has been minimized.

rust-timer commented Feb 19, 2026

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

nnethercote commented Feb 19, 2026

Uh oh!

This comment has been minimized.

Zalathar commented Feb 20, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Zoxc commented Feb 21, 2026

Uh oh!

nnethercote commented Feb 22, 2026

Uh oh!

This comment has been minimized.

nnethercote commented Feb 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

Zalathar Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

nnethercote Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Zalathar Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Zalathar commented Feb 23, 2026

Uh oh!

This comment has been minimized.

rustbot commented Feb 24, 2026

Uh oh!

nnethercote commented Feb 24, 2026

Uh oh!

Zalathar commented Feb 24, 2026

Uh oh!

Zalathar commented Feb 24, 2026

Uh oh!

nnethercote commented Feb 24, 2026

Uh oh!

rust-bors bot commented Feb 24, 2026

Uh oh!

bjorn3 commented Feb 24, 2026

Uh oh!

rust-bors bot commented Feb 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

nnethercote commented Feb 19, 2026 •

edited by rustbot

Loading

nnethercote commented Feb 22, 2026 •

edited

Loading