Clean up query-forcing functions by Zalathar · Pull Request #154236 · rust-lang/rust

Zalathar · 2026-03-23T04:39:35Z

This PR takes the force_query function, inlines it into its only caller force_from_dep_node_inner, and renames the resulting function to force_query_dep_node. Combining the functions became possible after the removal of rustc_query_system, because they are now in the same crate.

There are two other notable cleanups along the way:

Removing an unhelpful assertion and its verbose comment
- The removed comment was originally added in 0454a41, but is too verbose to be worth preserving inline.
Removing a redundant cache lookup while forcing, as it is useless in the serial compiler and unnecessary (and probably unhelpful) in the parallel compiler

r? nnethercote

Zalathar · 2026-03-23T04:42:34Z

I'm curious to see whether removing the cache lookup has any measurable perf effect.

@bors try @rust-timer queue

Clean up query-forcing functions

nnethercote

Looks ok. A couple of comments. I'm a bit unsure about the second commit so I'll wait to see if @Zoxc or @zetanumbers has anything to say about it.

View changes since this review

nnethercote · 2026-03-23T05:08:03Z

compiler/rustc_codegen_ssa/src/base.rs

+    // lead to having to re-execute compile_codegen_unit, possibly unnecessarily.
+    //
+    // For more background, see the comment in `plumbing.rs` at
+    // <https://github.com/rust-lang/rust/commit/0454a41>.


It feels weird to refer to a deleted comment. Seems like either this reference should be removed, or part/all of the deleted comment should be replicated here.

The linked comment is basically a longer and more detailed version of the comment that's just above these changed lines.

The main reason I wanted to add a link here is that someone looking at these codegen_unit calls could reasonably want more context on why they were added, and the actual history is rather tricky to track down due to a number of intermediate moves. But at the same time I think it would be too disruptive to add much more inline context here.

Perhaps I could try to find a more agreeable way to do this.

nnethercote · 2026-03-23T05:14:24Z

compiler/rustc_query_impl/src/dep_kind_vtables.rs

+        ),
        promote_from_disk_fn: (can_recover && is_cache_on_disk)
            .then_some(promote_from_disk_inner::<Q>),
    }


Should the second Q::query_vtable call be hoisted into a closure here as well?

I suppose it could be.

Note that this is effectively me changing my mind on 4284edc. At that time I thought GetQueryVTable might be useful for other kinds of static lookup in the future, but after various other simplifications and cleanups we seem to be doing fine with just using QueryVTable almost everywhere.

rust-bors · 2026-03-23T06:51:22Z

☀️ Try build successful (CI)
Build commit: 00d810c (00d810cf78c57daea23ccd90a17ab8627f76570b, parent: 562dee4820c458d823175268e41601d4c060588a)

rust-timer · 2026-03-23T07:31:33Z

Finished benchmarking commit (00d810c): comparison URL.

Overall result: ❌✅ regressions and 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.2%	[0.2%, 0.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.3%]	3
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

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

Cycles

Results (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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.5%	[-2.7%, -2.3%]	2
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 485.484s -> 483.408s (-0.43%)
Artifact size: 394.88 MiB -> 394.97 MiB (0.02%)

Zalathar · 2026-03-23T07:44:40Z

Perf results look like pure noise. No need to avoid rollup.
@bors rollup=maybe

This assertion and its comment are much visually heavier than the part of the function that actually performs important work. This assertion is also useless, because the `codegen_unit` query does not have a `force_from_dep_node_fn` in its DepKindVTable, so the assertion would never be reached even in the situation it is trying to guard against. This assertion is also redundant, because we have plenty of incremental tests that fail if the corresponding calls to `tcx.ensure_ok().codegen_unit(..)` are removed.

In the serial compiler, we should never be trying to force a query node that has a cached value, because we would have already marked the node red or green when putting its value in cache. In the parallel compiler, we immediately re-check the cache while holding the state shard lock anyway, and trying to avoid that lock doesn't seem worthwhile.

The different parts of this function used to be split across different crates, but nowadays they are both in `rustc_query_impl`, so they can be combined. This commit also hoists the vtable lookup to a closure in `make_dep_kind_vtable_for_query`, to reduce the amount of code that deals with `GetQueryVTable`, and to make the inner function more consistent with other functions in `execution`.

This effectively reverses <rust-lang@4284edc>. At that time, I thought GetQueryVTable might be useful for other kinds of static lookup in the future, but after various other simplifications and cleanups that now seems less likely, and this style is more consistent with other vtable-related functions.

zetanumbers · 2026-03-23T13:09:55Z

This is fine

Zoxc · 2026-03-23T17:56:12Z

LGTM

nnethercote · 2026-03-23T22:19:02Z

Thanks for the additional checking, @zetanumbers and @Zoxc.

@bors r+

rust-bors · 2026-03-23T22:19:05Z

📌 Commit 97b1c31 has been approved by nnethercote

It is now in the queue for this repository.

Clean up query-forcing functions This PR takes the `force_query` function, inlines it into its only caller `force_from_dep_node_inner`, and renames the resulting function to `force_query_dep_node`. Combining the functions became possible after the removal of `rustc_query_system`, because they are now in the same crate. There are two other notable cleanups along the way: - Removing an unhelpful assertion and its verbose comment - The removed comment was originally added in rust-lang@0454a41, but is too verbose to be worth preserving inline. - Removing a redundant cache lookup while forcing, as it is useless in the serial compiler and unnecessary (and probably unhelpful) in the parallel compiler r? nnethercote

Rollup of 10 pull requests Successful merges: - #153964 (Fix `doc_cfg` not working as expected on trait impls) - #153979 (Rename various query cycle things.) - #154132 (Add missing num_internals feature gate to coretests/benches) - #154153 (core: Implement `unchecked_funnel_{shl,shr}`) - #154236 (Clean up query-forcing functions) - #154252 (Don't store current-session side effects in `OnDiskCache`) - #154017 ( Fix invalid add of duplicated call locations for the rustdoc scraped examples feature) - #154163 (enzyme submodule update) - #154264 (Update books) - #154282 (rustc-dev-guide subtree update)

rustbot assigned nnethercote Mar 23, 2026

This comment has been minimized.

Sign in to view

rust-bors bot pushed a commit that referenced this pull request Mar 23, 2026

Auto merge of #154236 - Zalathar:force-query, r=<try>

00d810c

Clean up query-forcing functions

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

nnethercote reviewed Mar 23, 2026

View reviewed changes

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 Mar 23, 2026

Zalathar added 4 commits March 23, 2026 21:06

Zalathar force-pushed the force-query branch from 30d4d9b to 97b1c31 Compare March 23, 2026 10:07

nnethercote approved these changes Mar 23, 2026

View reviewed changes

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 Mar 23, 2026

JonathanBrouwer mentioned this pull request Mar 23, 2026

Rollup of 6 pull requests #154280

Closed

jhpratt mentioned this pull request Mar 24, 2026

Rollup of 10 pull requests #154289

Merged

rust-bors bot merged commit efa7a5e into rust-lang:main Mar 24, 2026
11 checks passed

rustbot added this to the 1.96.0 milestone Mar 24, 2026

Zalathar deleted the force-query branch March 24, 2026 08:08

Uh oh!

Conversation

Zalathar commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zalathar commented Mar 23, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

nnethercote left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rust-bors bot commented Mar 23, 2026

Uh oh!

This comment has been minimized.

rust-timer commented Mar 23, 2026

Overall result: ❌✅ regressions and improvements - no action needed

Uh oh!

Zalathar commented Mar 23, 2026

Uh oh!

zetanumbers commented Mar 23, 2026

Uh oh!

Zoxc commented Mar 23, 2026

Uh oh!

nnethercote commented Mar 23, 2026

Uh oh!

rust-bors bot commented Mar 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Zalathar commented Mar 23, 2026 •

edited

Loading

nnethercote left a comment •

edited by rustbot

Loading