Get rid of `rustc_query_description!` #102895

Noratrieb · 2022-10-10T18:26:40Z

I am not entirely sure whether this is an improvement and would like to get your feedback on it.

Helps with #96524.

Queries can provide an arbitrary expression for their description and their caching behavior. Before, these expressions where stored in a rustc_query_description macro emitted by the rustc_queries macro, and then used in rustc_query_impl to fill out the methods for the QueryDescription trait.

Instead, we now emit two new modules from rustc_queries containing the functions with the expressions. rustc_query_impl calls these functions now instead of invoking the macro.

Since we are now defining some of the functions in rustc_middle::query, we now need all the imports for the key types mthere as well.

r? @cjgillot

jyn514 · 2022-10-10T18:51:23Z

@bors try @rust-timer queue

rust-timer · 2022-10-10T18:51:25Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-10-10T18:51:32Z

⌛ Trying commit 6042ea72275fb466660bbdde8777dc7d6540e84b with merge 755919a27fb7a6a1ee85bc2810a3ec36c41099c6...

cjgillot

Thanks @Nilstrieb. I think this goes in the right direction.

compiler/rustc_query_impl/src/plumbing.rs

cjgillot · 2022-10-10T19:27:57Z

compiler/rustc_query_impl/src/plumbing.rs

+            }
+
+            #[inline]
+            fn cache_on_disk(tcx: TyCtxt<'tcx>, key: &Self::Key) -> bool {


cache_on_disk is typically very small and never used in rustc_query_system. I'd like to remove it from the trait too, if possible. It's used in try_load_from_on_disk_cache, so it's a bit more complex as a refactor.

Doing this in a not-cursed way (I have several cursed ideas involving conjuring up generic fn defs using transmute(())) requires a few more refactors than I'd like to do here, so I'd prefer doing that in a follow-up PR.

compiler/rustc_macros/src/query.rs

bors · 2022-10-10T20:41:50Z

☀️ Try build successful - checks-actions
Build commit: 755919a27fb7a6a1ee85bc2810a3ec36c41099c6 (755919a27fb7a6a1ee85bc2810a3ec36c41099c6)

rust-timer · 2022-10-10T20:41:52Z

Queued 755919a27fb7a6a1ee85bc2810a3ec36c41099c6 with parent 0265a3e, future comparison URL.

rust-timer · 2022-10-10T21:59:54Z

Finished benchmarking commit (755919a27fb7a6a1ee85bc2810a3ec36c41099c6): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	0.7%	[0.2%, 0.9%]	7
Regressions ❌ (secondary)	1.2%	[1.0%, 1.3%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.8%	[-1.1%, -0.4%]	4
All ❌✅ (primary)	0.7%	[0.2%, 0.9%]	7

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

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)	2.2%	[2.2%, 2.2%]	1
Regressions ❌ (secondary)	3.7%	[3.4%, 4.0%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.0%	[-3.0%, -3.0%]	1
All ❌✅ (primary)	2.2%	[2.2%, 2.2%]	1

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

Noratrieb · 2022-10-13T20:02:28Z

I implemented most of the fixes, but I haven't investigated the perf regressions for diesel. Maybe another perf run will make them magically go away after my fixes? 🙂

jyn514 · 2022-10-13T20:05:03Z

@bors try @rust-timer queue

rust-timer · 2022-10-13T20:05:05Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-10-13T20:05:13Z

⌛ Trying commit 4ae32c8cca22265c5c3ea8c2b751a251f53f97ed with merge cce71dc4adbb716bd87c9a5208f331d6cbfc7382...

bors · 2022-10-13T22:24:59Z

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

rust-timer · 2022-10-13T22:25:01Z

Queued cce71dc4adbb716bd87c9a5208f331d6cbfc7382 with parent 6b3ede3, future comparison URL.

rust-timer · 2022-10-13T23:45:05Z

Finished benchmarking commit (cce71dc4adbb716bd87c9a5208f331d6cbfc7382): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.1%	[1.1%, 1.1%]	3
Improvements ✅ (primary)	-0.3%	[-0.4%, -0.2%]	14
Improvements ✅ (secondary)	-0.7%	[-1.2%, -0.3%]	8
All ❌✅ (primary)	-0.3%	[-0.4%, -0.2%]	14

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

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)	2.6%	[1.5%, 4.1%]	4
Regressions ❌ (secondary)	2.7%	[1.8%, 3.2%]	13
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.6%	[1.5%, 4.1%]	4

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

Noratrieb · 2022-10-14T06:08:17Z

And they did magically go away and some even turned into improvements, very nice.

cjgillot · 2022-10-14T11:15:03Z

Great. We will see about the cache_on_disk situation later.
@bors r+

bors · 2022-10-14T11:15:04Z

📌 Commit 4ae32c8cca22265c5c3ea8c2b751a251f53f97ed has been approved by cjgillot

It is now in the queue for this repository.

bors · 2022-10-14T19:45:22Z

⌛ Testing commit 4ae32c8cca22265c5c3ea8c2b751a251f53f97ed with merge f18001a215707d9635b0865ec00b420dc96c0d7a...

bors · 2022-10-14T20:15:43Z

💔 Test failed - checks-actions

Queries can provide an arbitrary expression for their description and their caching behavior. Before, these expressions where stored in a `rustc_query_description` macro emitted by the `rustc_queries` macro, and then used in `rustc_query_impl` to fill out the methods for the `QueryDescription` trait. Instead, we now emit two new modules from `rustc_queries` containing the functions with the expressions. `rustc_query_impl` calls these functions now instead of invoking the macro. Since we are now defining some of the functions in `rustc_middle::query`, we now need all the imports for the key types there as well.

It was called directly already, but now it's even more useless since it just forwards to the free function. Call it directly.

Noratrieb · 2022-10-14T20:55:39Z

I rebased the commits and tried running the rustdoc-gui tests locally (which for some reason fail here even though I touched nothing relevant to it??). Some tests failed locally but after a few retries they all passed. Are the rustdoc-gui tests that flaky? Anyways, this seems to be a spurious error and not the fault of this PR

jyn514 · 2022-10-15T01:30:43Z

@bors retry

Dylan-DPC · 2022-10-15T09:05:58Z

@bors r=cjgillot

bors · 2022-10-15T09:06:00Z

📌 Commit 24ce4cf has been approved by cjgillot

It is now in the queue for this repository.

bors · 2022-10-15T13:30:18Z

⌛ Testing commit 24ce4cf with merge b8c35ca...

bors · 2022-10-15T16:15:43Z

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

rust-timer · 2022-10-15T17:35:05Z

Finished benchmarking commit (b8c35ca): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	0.3%	[0.2%, 0.5%]	9
Regressions ❌ (secondary)	0.6%	[0.3%, 0.8%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.7%	[-1.2%, -0.2%]	6
All ❌✅ (primary)	0.3%	[0.2%, 0.5%]	9

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)	2.0%	[2.0%, 2.0%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.9%	[-3.7%, -2.1%]	9
All ❌✅ (primary)	2.0%	[2.0%, 2.0%]	1

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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.7%	[-3.4%, -2.1%]	13
All ❌✅ (primary)	-	-	0

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

rust-highfive assigned cjgillot Oct 10, 2022

rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 10, 2022

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

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 10, 2022

cjgillot reviewed Oct 10, 2022

View reviewed changes

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Oct 10, 2022

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 13, 2022

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 13, 2022

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 Oct 14, 2022

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 14, 2022

Remove unsued variable in query macro

1566273

Noratrieb force-pushed the query-cleanups branch from 4ae32c8 to 1624187 Compare October 14, 2022 20:34

Noratrieb added 2 commits October 14, 2022 22:35

Remove the describe method from the QueryDescription trait

24ce4cf

It was called directly already, but now it's even more useless since it just forwards to the free function. Call it directly.

Noratrieb force-pushed the query-cleanups branch from 1624187 to 24ce4cf Compare October 14, 2022 20:36

This comment has been minimized.

Sign in to view

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 Oct 15, 2022

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

bors merged commit b8c35ca into rust-lang:master Oct 15, 2022

rustbot added this to the 1.66.0 milestone Oct 15, 2022

bors mentioned this pull request Oct 15, 2022

Allow to feed a value in another query's cache and remove WithOptConstParam #96840

Merged

Noratrieb deleted the query-cleanups branch October 15, 2022 16:54

Noratrieb mentioned this pull request Oct 23, 2022

MBE (macro_rules!) pattern-matching is unnecessarily O(n) even in simple cases. #68836

Open

This was referenced Jan 7, 2024

ICE with dyn* and method that takes self by value #119694

Open

ICE with dyn* when trait is implemented for union #119695

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get rid of `rustc_query_description!` #102895

Get rid of `rustc_query_description!` #102895

Noratrieb commented Oct 10, 2022 •

edited

Loading

jyn514 commented Oct 10, 2022

rust-timer commented Oct 10, 2022

bors commented Oct 10, 2022

cjgillot left a comment

cjgillot Oct 10, 2022

Noratrieb Oct 13, 2022

bors commented Oct 10, 2022

rust-timer commented Oct 10, 2022

rust-timer commented Oct 10, 2022

Noratrieb commented Oct 13, 2022

jyn514 commented Oct 13, 2022

rust-timer commented Oct 13, 2022

bors commented Oct 13, 2022

bors commented Oct 13, 2022

rust-timer commented Oct 13, 2022

rust-timer commented Oct 13, 2022

Noratrieb commented Oct 14, 2022

cjgillot commented Oct 14, 2022

bors commented Oct 14, 2022

bors commented Oct 14, 2022

bors commented Oct 14, 2022

Noratrieb commented Oct 14, 2022 •

edited

Loading

This comment has been minimized.

jyn514 commented Oct 15, 2022

Dylan-DPC commented Oct 15, 2022

bors commented Oct 15, 2022

bors commented Oct 15, 2022

bors commented Oct 15, 2022

rust-timer commented Oct 15, 2022

Get rid of rustc_query_description! #102895

Get rid of rustc_query_description! #102895

Conversation

Noratrieb commented Oct 10, 2022 • edited Loading

jyn514 commented Oct 10, 2022

rust-timer commented Oct 10, 2022

bors commented Oct 10, 2022

cjgillot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Oct 10, 2022

rust-timer commented Oct 10, 2022

rust-timer commented Oct 10, 2022

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Footnotes

Noratrieb commented Oct 13, 2022

jyn514 commented Oct 13, 2022

rust-timer commented Oct 13, 2022

bors commented Oct 13, 2022

bors commented Oct 13, 2022

rust-timer commented Oct 13, 2022

rust-timer commented Oct 13, 2022

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Footnotes

Noratrieb commented Oct 14, 2022

cjgillot commented Oct 14, 2022

bors commented Oct 14, 2022

bors commented Oct 14, 2022

bors commented Oct 14, 2022

Noratrieb commented Oct 14, 2022 • edited Loading

This comment has been minimized.

jyn514 commented Oct 15, 2022

Dylan-DPC commented Oct 15, 2022

bors commented Oct 15, 2022

bors commented Oct 15, 2022

bors commented Oct 15, 2022

rust-timer commented Oct 15, 2022

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Footnotes

Get rid of `rustc_query_description!` #102895

Get rid of `rustc_query_description!` #102895

Noratrieb commented Oct 10, 2022 •

edited

Loading

Noratrieb commented Oct 14, 2022 •

edited

Loading