Introduce for_each_query_vtable! to move more code out of query macros#153685
Introduce for_each_query_vtable! to move more code out of query macros#153685Zalathar wants to merge 2 commits intorust-lang:mainfrom
for_each_query_vtable! to move more code out of query macros#153685Conversation
|
|
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 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Introduce `for_each_query_vtable!` to move more code out of query macros
dcad3d3 to
82f9877
Compare
|
I have an item on my todo list to rename all these functions. Currently we have an inconsistent mess:
I'm considering these:
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? |
|
(The function renaming would be a follow-up to this PR, of course.) |
82f9877 to
7d69ab2
Compare
This simplifies the inner function's signature, and makes it more consistent with other uses of `for_each_query_vtable!`.
7d69ab2 to
50d4b2d
Compare
|
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. |
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 |
I considered a few different variations of how to indicate the filter condition, and having one macro with multiple arms and |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (45cd205): comparison URL. Overall result: ❌ regressions - no action neededBenchmarking 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 Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
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.
CyclesResults (secondary 1.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 479.93s -> 483.213s (0.68%) |
After #153114 moved a few for-each-query functions into the big
rustc_query_impl::plumbingmacro, 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: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:
$namemetavar from the outer macro.There should be no change to compiler behaviour.
r? nnethercote (or compiler)