Introduce for_each_query_vtable! to move more code out of query macros

[Zalathar]

View all comments

After #153114 moved a few for-each-query functions into the big rustc_query_impl::plumbing macro, I have found that those functions became much harder to navigate and modify, because they no longer have access to ordinary IDE features in rust-analyzer. Even finding the functions is considerably harder, because a plain go-to-definition no longer works smoothly.

This PR therefore tries to move as much of that code back out of the macro as possible, with the aid of a smaller for_each_query_vtable! helper macro. A typical use of that macro looks like this:

for_each_query_vtable!(ALL, tcx, |query| {
    query_key_hash_verify(query, tcx);
});

The result is an outer function consisting almost entirely of plain Rust code, with all of the usual IDE affordances expected of normal Rust code. Because it uses plain Rust syntax, it can also be formatted automatically by rustfmt.

Adding another layer of macro-defined macros is not something I propose lightly, but in this case I think the improvement is well worth it:

• The outer functions can once again be defined as “normal” Rust functions, right next to their corresponding inner functions, making navigation and modification much easier.
• The closure expression is ordinary Rust code that simply gets repeated ~300 times in the expansion, once for each query, in order to account for the variety of key/value/cache types used by different queries. Even within the closure expression, IDE features still mostly work, which is an improvement over the status quo.
• For future maintainers looking at the call site, the macro's effect should hopefully be pretty obvious and intuitive, reducing the need to even look at the helper macro. And the helper macro itself is largely straightforward, with its biggest complication being that it necessarily uses the $name metavar from the outer macro.

There should be no change to compiler behaviour.

r? nnethercote (or compiler)

[Zalathar]

• I found the navigation difficulties particularly frustrating while investigating Streamline active job collection. #153650.

[Zalathar]

I don't expect any perf difference, but I'm curious to see if there's a measurable difference in bootstrap time.

(Though I think that even an extra 1-2 seconds would still be worth the cost, if it came to that.)

@bors try @rust-timer queue

[nnethercote]

I initially thought that splitting it into two macros would be better, to eliminate the ALL and CACHE_ON_DISK, but it probably doesn't make a material difference.

View changes since this review

[nnethercote]

I have an item on my todo list to rename all these functions. Currently we have an inconsistent mess:

• collect_active_jobs_from_all_queries, gather_active_jobs
• alloc_self_profile_query_strings, alloc_self_profile_query_strings_for_query_cache
• encode_all_query_results, encode_query_results
• query_key_hash_verify_all, query_key_hash_verify

I'm considering these:

• collect_active_jobs_for_{all_queries,one_query}
• alloc_self_profile_strings_for_{all_queries,one_query}
• encode_results_for_{all_queries,one_query}
• verify_key_hashes_for_{all_queries,one_query}

They are long, but each one is only used a small number of times, and I think they score high for consistency and clarity. Shorter alternatives all seemed worse. What do you think?

[nnethercote]

(The function renaming would be a follow-up to this PR, of course.)

[Zalathar]

They are long, but each one is only used a small number of times, and I think they score high for consistency and clarity. Shorter alternatives all seemed worse. What do you think?

Seems pretty reasonable.

Also note that if we're moving in the direction of referring to query return values as “values” instead of ”results”, then that should probably be encode_values_for_{all_queries,one_query}.

[Zalathar]

I initially thought that splitting it into two macros would be better, to eliminate the ALL and CACHE_ON_DISK, but it probably doesn't make a material difference.

I considered a few different variations of how to indicate the filter condition, and having one macro with multiple arms and CONDITION_IN_CAPS felt like the best compromise overall.

[rust-bors]

☀️ Try build successful (CI)
Build commit: 45cd205 (45cd2051c9425da4b293edf8c5152780c9660aab, parent: b49ecc9eb70a51e89f32a7358e790f7b3808ccb3)

[rust-timer]

Finished benchmarking commit (45cd205): comparison URL.

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

Max RSS (memory usage)

Results (primary -0.2%, secondary -1.9%)

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

	mean	range	count
Regressions ❌ (primary)	1.4%	[0.7%, 2.0%]	3
Regressions ❌ (secondary)	1.9%	[1.9%, 1.9%]	1
Improvements ✅ (primary)	-1.8%	[-2.0%, -1.6%]	3
Improvements ✅ (secondary)	-2.9%	[-3.5%, -1.9%]	4
All ❌✅ (primary)	-0.2%	[-2.0%, 2.0%]	6

Cycles

Results (secondary 1.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)	4.1%	[3.1%, 5.1%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.7%	[-3.7%, -3.7%]	1
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 479.93s -> 483.213s (0.68%)
Artifact size: 396.95 MiB -> 394.92 MiB (-0.51%)

[nnethercote]

Hmm, +3.3s for bootstrap, I wonder if that's real.

[Zalathar]

Based on some local measurements, I think the bootstrap-time regression is real, though the full 3.3 seconds is probably exaggerated.

I have an alternate version that expands into N calls to a single named inner function, instead of N calls to N duplicated closures. I'll push that as a separate commit, to see what you think of the different tradeoffs.

[Zalathar]

Let’s see what perf has to say about the function-call version.

@bors try @rust-timer queue

[rustbot]

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]

@bors r+

[rust-bors]

📌 Commit d066ff7 has been approved by nnethercote

It is now in the queue for this repository.

[rust-bors]

⚠️ A new commit 9de761b23281f156a8556f85f0c7456faf4ffc0d was pushed.

This pull request was unapproved.

[Zalathar]

Ah, after I removed the alternative syntax, I was in the middle of a cosmetic update that makes the modifiers a bit more distinctive.

Thoughts on the changes? I think it's a little nicer, but I'd be happy to revert back to the approved version.

I'll just revert back to the approved version and reapprove.

[Zalathar]

To keep things simple, I reverted back to the previously-approved revision.

(No perf changes, and the bootstrap timing change is tiny.)

@bors r=nnethercote rollup=maybe

[rust-bors]

📌 Commit d066ff7 has been approved by nnethercote

It is now in the queue for this repository.