Skip to content

Improve how QueryCaches and QueryStates are stored#152835

Open
nnethercote wants to merge 4 commits intorust-lang:mainfrom
nnethercote:fix-query-state-query-cache
Open

Improve how QueryCaches and QueryStates are stored#152835
nnethercote wants to merge 4 commits intorust-lang:mainfrom
nnethercote:fix-query-state-query-cache

Conversation

@nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Feb 19, 2026

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

@nnethercote
Avoid using QueryVTable in mk_query_stack_frame_extra.
a7a2a68 
The tuple used by `mk_query_stack_frame_extra` currently uses
`QueryVTable`. The next commit will make `QueryVTable` no longer impl
`DynSync`, which will prevent it from being used in the tuple. So this
commit changes the tuple to use just the three necessary fields from
`QueryVTable` instead of the whole thing.
@nnethercote
Improve how QueryCaches and QueryStates are stored.
a4be2f2 
`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` (via a
`SemiDynamicQueryDispatcher` wrapper) 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. `QueryVTable` no
longer impls `DynSync`, which required the change in the preceding
commit.
@nnethercote
Rename some Cache type variables as C.
156f355 
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`.
@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) 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. labels Feb 19, 2026
@rustbot
Copy link
Collaborator

rustbot commented Feb 19, 2026

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

@Zalathar
Copy link
Member

Zalathar commented Feb 19, 2026

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rust-bors

This comment has been minimized.

Sign in to view
rust-bors bot pushed a commit that referenced this pull request Feb 19, 2026
@bors
Auto merge of #152835 - nnethercote:fix-query-state-query-cache, r=<try>
6efacd9 
Improve how `QueryCache`s and `QueryState`s are stored
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 19, 2026
@Zalathar
Copy link
Member

Zalathar commented Feb 19, 2026

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
Copy link
Contributor Author

nnethercote commented Feb 19, 2026

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
Copy link
Member

Zalathar commented Feb 19, 2026

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
Copy link
Contributor

rust-bors bot commented Feb 19, 2026

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

@rust-timer

This comment has been minimized.

Sign in to view
@nnethercote
Merge QueryEngine into QueryVTable.
bc792c2 
`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.
@rust-timer
Copy link
Collaborator

rust-timer commented Feb 19, 2026

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

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 19, 2026
@nnethercote
Copy link
Contributor Author

nnethercote commented Feb 19, 2026

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.

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

Reviewers

No reviews

Assignees

@Zalathar Zalathar

Labels

A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) 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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nnethercote @rustbot @Zalathar @rust-timer

Comments