Refactor query loading by nnethercote · Pull Request #154207 · rust-lang/rust

nnethercote · 2026-03-22T09:46:38Z

Some refactoring of code relating to query result loading. Details in individual commits.

It's very small and only has two call sites. The tiny loss of DRY is worth it to shrink `QueryVTable`.

We can only reach this point if `try_load_from_disk_fn` fails, and the condition of this assertion is basically just the inverse of what `try_load_from_disk_fn` does. Basically it's like this: ``` fn foo() -> bool { a && b } fn bar() { if foo() { return; } assert!(!a || !b); } ``` The assertion is just confusing and provides little value.

rustbot · 2026-03-22T09:46:45Z

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

nnethercote · 2026-03-22T09:46:51Z

@bors try @rust-timer queue

Refactor query loading

Zalathar · 2026-03-22T11:25:57Z

compiler/rustc_query_impl/src/execution.rs

    // we should actually be able to load it.
    debug_assert!(
-        !(query.is_loadable_from_disk_fn)(tcx, key, prev_index),
+        !((query.will_cache_on_disk_for_key_fn)(tcx, key) && loadable_from_disk(tcx, prev_index)),


Nit: It seems weird to inline this call site only to then immediately delete it, when we could rearrange the commits to do the deletion first.

I was feeling my way with the commits; the plan wasn't totally clear when I started. Inlining this first made it easier for me to see the correspondence between this assertion and load_from_disk_or_invoke_provider_green.

Zalathar · 2026-03-22T11:35:27Z

compiler/rustc_query_impl/src/execution.rs

+    let value = (query.try_load_from_disk_fn)(tcx, key, prev_index, dep_node_index);
+    let (value, verify) = if let Some(value) = value {


In this situation I think it would be good to:

Use match instead of if let, which I think is a better fit for the “similar conceptual weight” of the two arms, despite adding an extra layer of indentation. Match syntax also puts the Some(value) binding closer to the code that actually uses it.

Inline the value variable into the match scrutinee position, to avoid the confusion of it actually being optional.

Get rid of the let (value, verify) =, and instead declare two uninitialized variables outside the block, which are then assigned in the respective match arms:

let value; let verify; match (query.try_load_from_disk_fn)(tcx, key, prev_index, dep_node_index) { Some(value) => { ... value = ...; verify = ...; None => { ... value = ...; verify = ...; } }

Ok, I've done that, except I did Some(loaded_value) otherwise value gets shadowed.

Zalathar · 2026-03-22T11:38:35Z

compiler/rustc_query_impl/src/execution.rs

-        // We always expect to find a cached result for things that can be forced from `DepNode`.
-        debug_assert!(
-            !(query.will_cache_on_disk_for_key_fn)(tcx, key)
-                || !tcx.key_fingerprint_style(dep_node.kind).is_maybe_recoverable(),
-            "missing on-disk cache entry for {dep_node:?}"
-        );


Nit: If we're removing this assertion, we should rearrange the commits to remove it before re-indenting the surrounding code.

Ok. I put this commit last because I thought it might be controversial, but sounds like you're ok with it.

Zalathar · 2026-03-22T11:44:28Z

I like these changes overall BTW; I just happened to notice a bunch of possible style improvements along the way.

rust-bors · 2026-03-22T11:57:50Z

☀️ Try build successful (CI)
Build commit: eeb1c5b (eeb1c5bb096864ccede618219084efd96f988358, parent: 562dee4820c458d823175268e41601d4c060588a)

rust-timer · 2026-03-22T12:38:03Z

Finished benchmarking commit (eeb1c5b): comparison URL.

Overall result: no relevant changes - 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

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

Max RSS (memory usage)

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

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.5%, -2.5%]	1
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 485.484s -> 484.632s (-0.18%)
Artifact size: 394.88 MiB -> 394.87 MiB (-0.00%)

This one just irritates me, and I don't think it adds much value.

By removing the early return and using a `match` instead. - The two paths are of similar conceptual weight, and `match` reflects that. - This lets the `incremental_verify_ich` call be factored out.

nnethercote · 2026-03-22T22:28:55Z

@rustbot ready

Zalathar · 2026-03-23T01:47:42Z

@bors r+ rollup=maybe

rust-bors · 2026-03-23T01:47:46Z

📌 Commit 327216d has been approved by Zalathar

It is now in the queue for this repository.

@Zalathar

…, r=Zalathar Refactor query loading Some refactoring of code relating to query result loading. Details in individual commits. r? @Zalathar

@Zalathar

…, r=Zalathar Refactor query loading Some refactoring of code relating to query result loading. Details in individual commits. r? @Zalathar

…uwer Rollup of 13 pull requests Successful merges: - #154241 (`rust-analyzer` subtree update) - #153686 (`std`: include `dlmalloc` for all non-wasi Wasm targets) - #154105 (bootstrap: Pass `--features=rustc` to rustc_transmute) - #153069 ([BPF] add target feature allows-misaligned-mem-access) - #154085 (Parenthesize or-patterns in prefix pattern positions in pretty printer) - #154191 (refactor RangeFromIter overflow-checks impl) - #154207 (Refactor query loading) - #153540 (drop derive helpers during attribute parsing) - #154140 (Document consteval behavior of ub_checks, overflow_checks, is_val_statically_known.) - #154161 (On E0277 tweak help when single type impls traits) - #154218 (interpret/validity: remove unreachable error kind) - #154225 (diagnostics: avoid ICE in confusable_method_name for associated functions) - #154228 (Improve inline assembly error messages)

…uwer Rollup of 13 pull requests Successful merges: - rust-lang/rust#154241 (`rust-analyzer` subtree update) - rust-lang/rust#153686 (`std`: include `dlmalloc` for all non-wasi Wasm targets) - rust-lang/rust#154105 (bootstrap: Pass `--features=rustc` to rustc_transmute) - rust-lang/rust#153069 ([BPF] add target feature allows-misaligned-mem-access) - rust-lang/rust#154085 (Parenthesize or-patterns in prefix pattern positions in pretty printer) - rust-lang/rust#154191 (refactor RangeFromIter overflow-checks impl) - rust-lang/rust#154207 (Refactor query loading) - rust-lang/rust#153540 (drop derive helpers during attribute parsing) - rust-lang/rust#154140 (Document consteval behavior of ub_checks, overflow_checks, is_val_statically_known.) - rust-lang/rust#154161 (On E0277 tweak help when single type impls traits) - rust-lang/rust#154218 (interpret/validity: remove unreachable error kind) - rust-lang/rust#154225 (diagnostics: avoid ICE in confusable_method_name for associated functions) - rust-lang/rust#154228 (Improve inline assembly error messages)

nnethercote added 2 commits March 22, 2026 14:32

Inline and remove QueryVTable::is_loadable_from_disk_fn.

5841fa8

It's very small and only has two call sites. The tiny loss of DRY is worth it to shrink `QueryVTable`.

rustbot assigned Zalathar Mar 22, 2026

This comment has been minimized.

Sign in to view

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

Auto merge of #154207 - nnethercote:refactor-query-loading, r=<try>

eeb1c5b

Refactor query loading

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

Zalathar reviewed Mar 22, 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 22, 2026

nnethercote added 2 commits March 23, 2026 09:12

Remove another assertion in load_from_disk_or_invoke_provider_green.

cde59f0

This one just irritates me, and I don't think it adds much value.

Refactor load_from_disk_or_invoke_provider_green.

327216d

By removing the early return and using a `match` instead. - The two paths are of similar conceptual weight, and `match` reflects that. - This lets the `incremental_verify_ich` call be factored out.

nnethercote force-pushed the refactor-query-loading branch from 7d547af to 327216d Compare March 22, 2026 22:27

nnethercote marked this pull request as ready for review March 22, 2026 22:28

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 22, 2026

rust-bors bot added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 23, 2026

rust-bors bot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 23, 2026

JonathanBrouwer mentioned this pull request Mar 23, 2026

Rollup of 10 pull requests #154240

Closed

This was referenced Mar 23, 2026

Rollup of 13 pull requests #154242

Closed

Rollup of 13 pull requests #154253

Merged

rust-bors bot merged commit 83a0bd9 into rust-lang:main Mar 23, 2026
11 checks passed

rustbot added this to the 1.96.0 milestone Mar 23, 2026

nnethercote deleted the refactor-query-loading branch March 23, 2026 21:44

Zalathar mentioned this pull request Mar 25, 2026

Refactor execute_job_incr #154279

Draft

		let value = (query.try_load_from_disk_fn)(tcx, key, prev_index, dep_node_index);
		let (value, verify) = if let Some(value) = value {

Uh oh!

Conversation

nnethercote commented Mar 22, 2026

Uh oh!

rustbot commented Mar 22, 2026

Uh oh!

nnethercote commented Mar 22, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

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!

Choose a reason for hiding this comment

Uh oh!

Zalathar commented Mar 22, 2026

Uh oh!

rust-bors bot commented Mar 22, 2026

Uh oh!

This comment has been minimized.

rust-timer commented Mar 22, 2026

Overall result: no relevant changes - no action needed

Uh oh!

nnethercote commented Mar 22, 2026

Uh oh!

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

4 participants