Further simplify the macros generated by `rustc_queries` #101173

jyn514 · 2022-08-29T23:41:10Z

This doesn't actually move anything outside the macros, but it makes them simpler to read.

Add a new rustc_query_names macro. This allows a much simpler syntax for the matchers in the macros passed to it as a callback.
Convert define_dep_nodes and alloc_once to use rustc_query_names. This is possible because they only use the names
(despite the quite complicated matchers in define_dep_nodes, none of the other arguments are used).
Get rid of rustc_dep_node_append.

cjgillot · 2022-08-31T21:52:17Z

compiler/rustc_middle/src/dep_graph/dep_node.rs

-        [$($attrs:tt)*]
-        $variant:ident $(( $tuple_arg_ty:ty $(,)? ))*
-      ,)*
+        $( $( #[$attr:meta] )* $variant:ident, )+


We lose the documentation for variants defined using the proc-macro, but get some for the nodes defined in this file. Shouldn't we have documentation for all or nothing?

Oh, excellent catch. Yes, I forgot that previously attrs could include doc-comments, I agree we should keep those.

I almost wonder if we should remove rustc_query_names altogether and force everyone to match on the tokens for rustc_query_append. It's not that much extra work and it allows getting rid of another macro, and makes it easy to extend all the callsites to use the additional info in the future if they need it.

(Just as a note, we weren't including the doc-comments even before my change, we would match on them and then immediately discard them.)

Having just rustc_query_append seems great. We'll just need a bit of creativity to give a signature to the 5 extra dep-nodes. Actually, could we declare those 5 extra dep-nodes inside rustc_middle::query directly, and get rid of the _append part of the macro?

Yes, I think I can do both those things :) and it should be possible to get rid of rustc_cached_queries at the same time, since we can match on the cache modifier in the macro rather than requiring it be done by the proc-macro.

I'm going to avoid declaring the 5 dep-nodes in rustc_middle::query for now; ignoring the snake_case warning is harder than I expected (at least until #101512 is fixed), and it's not that hard to add the extra matchers in the other macros. I'll try and follow up on that later.

Ok, got rid of rustc_query_names and rustc_cached_queries. I think it might be possible to remove rustc_query_description after #101307 is merged, using (cache = $expr) and (desc = $fmt_str, $($arg),*).

- Add a new `rustc_query_names` macro. This allows a much simpler syntax for the matchers in the macros passed to it as a callback. - Convert `define_dep_nodes` and `alloc_once` to use `rustc_query_names`. This is possible because they only use the names (despite the quite complicated matchers in `define_dep_nodes`, none of the other arguments are used). - Get rid of `rustc_dep_node_append`.

… macro We can avoid these by adding slightly more information to `rustc_query_append` instead.

compiler/rustc_macros/src/query.rs

cjgillot · 2022-09-14T17:12:25Z

@bors try @rust-timer perf

bors · 2022-09-14T17:12:36Z

⌛ Trying commit cb2949e with merge 39c01438377c09997b8cb33020a9c4636ea8d7f8...

bors · 2022-09-14T19:03:20Z

☀️ Try build successful - checks-actions
Build commit: 39c01438377c09997b8cb33020a9c4636ea8d7f8 (39c01438377c09997b8cb33020a9c4636ea8d7f8)

jyn514 · 2022-09-14T23:54:05Z

@rust-timer build 39c01438377c09997b8cb33020a9c4636ea8d7f8

rust-timer · 2022-09-14T23:54:07Z

Queued 39c01438377c09997b8cb33020a9c4636ea8d7f8 with parent c97922d, future comparison URL.

rust-timer · 2022-09-15T02:10:18Z

Finished benchmarking commit (39c01438377c09997b8cb33020a9c4636ea8d7f8): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -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

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.8%	[3.8%, 3.8%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	11.7%	[11.7%, 11.7%]	1
Regressions ❌ (secondary)	2.7%	[2.7%, 2.7%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.7%	[-3.0%, -2.3%]	4
All ❌✅ (primary)	11.7%	[11.7%, 11.7%]	1

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

cjgillot · 2022-09-15T06:05:17Z

@bors r+

bors · 2022-09-15T06:05:18Z

📌 Commit cb2949e has been approved by cjgillot

It is now in the queue for this repository.

bors · 2022-09-15T11:54:06Z

⌛ Testing commit cb2949e with merge 294f0ee...

bors · 2022-09-15T14:53:38Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 294f0ee to master...

rust-timer · 2022-09-15T16:12:45Z

Finished benchmarking commit (294f0ee): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	1.3%	[1.2%, 1.3%]	2
Regressions ❌ (secondary)	1.8%	[1.5%, 2.1%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.9%	[-3.9%, -3.9%]	1
All ❌✅ (primary)	1.3%	[1.2%, 1.3%]	2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	6.7%	[2.1%, 11.3%]	2
Regressions ❌ (secondary)	2.1%	[2.1%, 2.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	6.7%	[2.1%, 11.3%]	2

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

Use function pointers instead of macro-unrolled loops in rustc_query_impl By making these standalone functions, we a) allow making them extensible in the future with a new `QueryStruct` b) greatly decrease the amount of code in each individual function, avoiding exponential blowup in llvm Helps with rust-lang#96524. Based on rust-lang#101173; only the last commit is relevant. r? `@cjgillot`

jyn514 added the A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) label Aug 29, 2022

rust-highfive assigned cjgillot Aug 29, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 29, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 29, 2022

jyn514 force-pushed the simplify-macro-arguments branch from 7b9cf74 to f96d7fa Compare August 29, 2022 23:41

jyn514 added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Aug 29, 2022

cjgillot reviewed Aug 31, 2022

View reviewed changes

jyn514 force-pushed the simplify-macro-arguments branch 3 times, most recently from 5210df9 to 64aa648 Compare September 7, 2022 02:37

jyn514 added 2 commits September 6, 2022 21:43

Support doc-comments in define_dep_nodes

c630c87

This comment has been minimized.

Sign in to view

Get rid of the emitted rustc_query_names and rustc_cached_queries…

3a4e3c7

… macro We can avoid these by adding slightly more information to `rustc_query_append` instead.

jyn514 force-pushed the simplify-macro-arguments branch from 64aa648 to 3a4e3c7 Compare September 7, 2022 02:46

jyn514 mentioned this pull request Sep 14, 2022

Use function pointers instead of macro-unrolled loops in rustc_query_impl #101785

Merged

cjgillot reviewed Sep 14, 2022

View reviewed changes

compiler/rustc_macros/src/query.rs Outdated Show resolved Hide resolved

Update compiler/rustc_macros/src/query.rs

cb2949e

bors 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 Sep 15, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 15, 2022

bors merged commit 294f0ee into rust-lang:master Sep 15, 2022

rustbot added this to the 1.65.0 milestone Sep 15, 2022

bors mentioned this pull request Sep 15, 2022

Rewrite representability #100720

Merged

jyn514 deleted the simplify-macro-arguments branch September 15, 2022 16:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Further simplify the macros generated by `rustc_queries` #101173

Further simplify the macros generated by `rustc_queries` #101173

jyn514 commented Aug 29, 2022

cjgillot Aug 31, 2022

jyn514 Sep 1, 2022 •

edited

Loading

cjgillot Sep 4, 2022

jyn514 Sep 7, 2022

jyn514 Sep 7, 2022 •

edited

Loading

jyn514 Sep 7, 2022 •

edited

Loading

This comment has been minimized.

cjgillot commented Sep 14, 2022

bors commented Sep 14, 2022

bors commented Sep 14, 2022

jyn514 commented Sep 14, 2022

rust-timer commented Sep 14, 2022

rust-timer commented Sep 15, 2022

cjgillot commented Sep 15, 2022

bors commented Sep 15, 2022

bors commented Sep 15, 2022

bors commented Sep 15, 2022

rust-timer commented Sep 15, 2022

Further simplify the macros generated by rustc_queries #101173

Further simplify the macros generated by rustc_queries #101173

Conversation

jyn514 commented Aug 29, 2022

Choose a reason for hiding this comment

jyn514 Sep 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyn514 Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

jyn514 Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

cjgillot commented Sep 14, 2022

bors commented Sep 14, 2022

bors commented Sep 14, 2022

jyn514 commented Sep 14, 2022

rust-timer commented Sep 14, 2022

rust-timer commented Sep 15, 2022

Overall result: no relevant changes - no action needed

Footnotes

cjgillot commented Sep 15, 2022

bors commented Sep 15, 2022

bors commented Sep 15, 2022

bors commented Sep 15, 2022

rust-timer commented Sep 15, 2022

Overall result: no relevant changes - no action needed

Footnotes

Further simplify the macros generated by `rustc_queries` #101173

Further simplify the macros generated by `rustc_queries` #101173

jyn514 Sep 1, 2022 •

edited

Loading

jyn514 Sep 7, 2022 •

edited

Loading

jyn514 Sep 7, 2022 •

edited

Loading