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-24T01:33:52Z

r=me with commit message nit addressed.

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.

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

rustbot · 2026-02-24T21:39:45Z

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-24T21:44:28Z

I rebased.

@bors r=Zalathar p=1 (blocking other query-related work)

rust-bors · 2026-02-24T21:44:31Z

📌 Commit 4a5ee04 has been approved by Zalathar

It is now in the queue for this repository.

rust-bors · 2026-02-25T05:49:04Z

☀️ Test successful - CI
Approved by: Zalathar
Duration: 3h 22m 32s
Pushing 11ad63a to main...

github-actions · 2026-02-25T05:51:53Z

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 859951e (parent) -> 11ad63a (this PR)

Test differences

Show 4 test diffs

4 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 11ad63a942c380b83a1fbfe5f0ecb1caaad4c7c1 --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

aarch64-apple: 4h 17m -> 3h 15m (-23.9%)
dist-x86_64-apple: 3h 17m -> 2h 30m (-23.8%)
dist-apple-various: 3h 2m -> 2h 20m (-23.0%)
dist-aarch64-apple: 2h 9m -> 2h 36m (+21.2%)
x86_64-gnu-llvm-20: 1h 21m -> 1h 10m (-13.6%)
dist-aarch64-llvm-mingw: 1h 35m -> 1h 47m (+12.5%)
pr-check-1: 27m 54s -> 31m 13s (+11.9%)
test-various: 1h 53m -> 2h 5m (+10.5%)
aarch64-msvc-2: 1h 43m -> 1h 54m (+10.4%)
x86_64-gnu-llvm-20-2: 1h 30m -> 1h 38m (+9.0%)

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 · 2026-02-25T06:30:19Z

Finished benchmarking commit (11ad63a): 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.2%	[-0.3%, -0.1%]	5
Improvements ✅ (secondary)	-0.2%	[-0.3%, -0.1%]	16
All ❌✅ (primary)	-0.2%	[-0.3%, -0.1%]	5

Max RSS (memory usage)

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

Cycles

Results (primary 2.2%, secondary 2.6%)

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

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

Binary size

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

Bootstrap: 481s -> 482.035s (0.22%)
Artifact size: 397.83 MiB -> 397.81 MiB (-0.01%)

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

This comment has been minimized.

Sign in to view

nnethercote added 4 commits February 25, 2026 08:13

Merge QueryEngine into QueryVTable.

2bc5167

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

4a5ee04

The `Per` isn't really necessary.

nnethercote force-pushed the fix-query-state-query-cache branch from 7df5249 to 4a5ee04 Compare February 24, 2026 21:39

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 24, 2026

This comment has been minimized.

Sign in to view

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

rust-bors bot merged commit 11ad63a into rust-lang:main Feb 25, 2026
12 checks passed

rustbot added this to the 1.95.0 milestone Feb 25, 2026

rust-bors bot mentioned this pull request Feb 25, 2026

Remove _description_fns and _cache_on_disk_if_fns modules #153065

Open

nnethercote deleted the fix-query-state-query-cache branch February 25, 2026 06:21

Uh oh!

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

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.

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!

This comment has been minimized.

rustbot commented Feb 24, 2026

Uh oh!

nnethercote commented Feb 24, 2026

Uh oh!

rust-bors bot commented Feb 24, 2026

Uh oh!

This comment has been minimized.

rust-bors bot commented Feb 25, 2026

Uh oh!

Uh oh!

github-actions bot commented Feb 25, 2026

Test differences

Job duration changes

Uh oh!

rust-timer commented Feb 25, 2026

Overall result: ✅ improvements - no action needed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nnethercote commented Feb 19, 2026 •

edited by rustbot

Loading

nnethercote commented Feb 22, 2026 •

edited

Loading