Simplify the `define_query` macro #100943

jyn514 · 2022-08-24T04:39:12Z

This moves a bunch of control flow out of the macro into generic functions, leaving the macro just to call the function with a new generic parameter for each query.

It may be possible to improve compile-times / icache by instantiating the generic functions only with the query key, not the query type itself, but I'm going to leave that for a follow-up PR.

Helps with #96524.

r? @cjgillot

jyn514 · 2022-08-24T04:39:51Z

@bors try @rust-timer queue

rust-timer · 2022-08-24T04:39:53Z

Awaiting bors try build completion.

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

bors · 2022-08-24T04:39:59Z

⌛ Trying commit 61d9559b5b175227be74f1610ee8dbc8778b8038 with merge 2c96ac25c27d7e95c062ec4bf1bd937ad27dd441...

bors · 2022-08-24T05:59:32Z

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

rust-timer · 2022-08-24T05:59:35Z

Queued 2c96ac25c27d7e95c062ec4bf1bd937ad27dd441 with parent 25ea5a3, future comparison URL.

rust-timer · 2022-08-24T07:13:42Z

Finished benchmarking commit (2c96ac25c27d7e95c062ec4bf1bd937ad27dd441): comparison URL.

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

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.0%	[2.1%, 4.6%]	4
Improvements ✅ (primary)	-2.6%	[-2.6%, -2.6%]	1
Improvements ✅ (secondary)	-4.7%	[-4.7%, -4.7%]	1
All ❌✅ (primary)	-2.6%	[-2.6%, -2.6%]	1

Cycles

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

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

jyn514 · 2022-08-24T13:05:45Z

it's hard to tell because this contains other changes, but this PR is actually a regression; #100436 had significant improvements that are gone now. I'll rerun perf once that's merged and we can get a less noisy benchmark.

cjgillot · 2022-08-24T16:59:24Z

compiler/rustc_query_impl/src/plumbing.rs

@@ -300,6 +301,27 @@ pub(crate) fn try_load_from_on_disk_cache<'tcx, K, V>(
    }
 }

+pub(crate) fn force_from_dep_node<'tcx, Q>(
+    tcx: TyCtxt<'tcx>,
+    // dep_node: rustc_query_system::dep_graph::DepNode<CTX::DepKind>,


Stray comment.

This is removed in a later commit; I can try and fix the history if you like.

compiler/rustc_query_impl/src/plumbing.rs

jyn514 · 2022-08-29T15:37:56Z

@bors try @rust-timer queue

rust-timer · 2022-08-29T15:37:58Z

Awaiting bors try build completion.

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

bors · 2022-08-29T15:38:07Z

⌛ Trying commit 5d8015516034c48d59a9c32de04ad3244999ba1c with merge d51cf6c3170ba41ec8eaa42298d63f17bf40dd00...

bors · 2022-08-29T16:05:23Z

💔 Test failed - checks-actions

…ction

- Parameterize DepKindStruct over `'tcx` This allows passing in an invariant function pointer in `query_callback`, rather than having to try and make it work for any lifetime. - Add a new `execute_query` function to `QueryDescription` so we can call `tcx.$name` without needing to be in a macro context

jyn514 · 2022-09-01T23:48:07Z

@bors r=cjgillot

bors · 2022-09-01T23:48:09Z

📌 Commit 4e09a13 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2022-09-02T08:28:11Z

⌛ Testing commit 4e09a13 with merge e21d771...

bors · 2022-09-02T10:55:18Z

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

rust-timer · 2022-09-02T12:10:39Z

Finished benchmarking commit (e21d771): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -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)	0.8%	[0.8%, 0.8%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.8%	[-0.8%, -0.8%]	1
All ❌✅ (primary)	-	-	0

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

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

jyn514 · 2022-09-02T13:04:37Z

Bootstrap times are noisy as usual, but looks like this cut down a second of compile time for rustc_middle :)

…r=cjgillot Make `HandleCycleError` an enum instead of a macro-generated closure Helps with rust-lang#96524. Based on rust-lang#100943 to avoid merge conflicts, so it looks larger than it is (only the last commit is relevant). cc https://rust-lang.zulipchat.com/#narrow/stream/241847-t-compiler.2Fwg-incr-comp/topic/Moving.20.60Value.60.20to.20rustc_query_system.20.2396524 r? `@cjgillot`

…cjgillot Make `HandleCycleError` an enum instead of a macro-generated closure Helps with rust-lang#96524. Based on rust-lang#100943 to avoid merge conflicts, so it looks larger than it is (only the last commit is relevant). cc https://rust-lang.zulipchat.com/#narrow/stream/241847-t-compiler.2Fwg-incr-comp/topic/Moving.20.60Value.60.20to.20rustc_query_system.20.2396524 r? `@cjgillot`

Make `HandleCycleError` an enum instead of a macro-generated closure Helps with rust-lang/rust#96524. Based on rust-lang/rust#100943 to avoid merge conflicts, so it looks larger than it is (only the last commit is relevant). cc https://rust-lang.zulipchat.com/#narrow/stream/241847-t-compiler.2Fwg-incr-comp/topic/Moving.20.60Value.60.20to.20rustc_query_system.20.2396524 r? `@cjgillot`

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

rust-highfive assigned cjgillot Aug 24, 2022

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

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

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

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

jyn514 added the perf-regression Performance regression. label Aug 24, 2022

jyn514 mentioned this pull request Aug 24, 2022

Refactor the query system to reduce the amount of macro-expanded functions #96524

Closed

6 tasks

cjgillot reviewed Aug 24, 2022

View reviewed changes

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

jyn514 force-pushed the query-system-2 branch from f271b0a to 11f8d2a Compare August 29, 2022 15:36

jyn514 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 Aug 29, 2022

jyn514 force-pushed the query-system-2 branch from 11f8d2a to 5d80155 Compare August 29, 2022 15:37

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

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

jyn514 added 4 commits September 1, 2022 18:34

Get rid of make_query module

1cf7bcc

Move try_on_disk_cache out of the giant macro

375d780

Move force_with_dep_node outside the giant macro

70f20ac

Get rid of fn recover

8f442e8

jyn514 force-pushed the query-system-2 branch from e49e169 to 3e72aff Compare September 1, 2022 23:47

jyn514 added 3 commits September 1, 2022 18:47

Move almost all of the function in query_callbacks to a generic fun…

83b6dc9

…ction

Simplify try_load_from_on_disk_cache

4fcc745

jyn514 force-pushed the query-system-2 branch from 3e72aff to 4e09a13 Compare September 1, 2022 23:48

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 1, 2022

jyn514 mentioned this pull request Sep 2, 2022

Make HandleCycleError an enum instead of a macro-generated closure #101303

Merged

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

bors merged commit e21d771 into rust-lang:master Sep 2, 2022

rustbot added this to the 1.65.0 milestone Sep 2, 2022

jyn514 deleted the query-system-2 branch September 2, 2022 13:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify the `define_query` macro #100943

Simplify the `define_query` macro #100943

jyn514 commented Aug 24, 2022 •

edited

Loading

jyn514 commented Aug 24, 2022

rust-timer commented Aug 24, 2022

bors commented Aug 24, 2022

bors commented Aug 24, 2022

rust-timer commented Aug 24, 2022

rust-timer commented Aug 24, 2022

jyn514 commented Aug 24, 2022

cjgillot Aug 24, 2022

jyn514 Aug 27, 2022

jyn514 commented Aug 29, 2022

rust-timer commented Aug 29, 2022

bors commented Aug 29, 2022

bors commented Aug 29, 2022

jyn514 commented Sep 1, 2022

bors commented Sep 1, 2022

bors commented Sep 2, 2022

bors commented Sep 2, 2022

rust-timer commented Sep 2, 2022

jyn514 commented Sep 2, 2022

Simplify the define_query macro #100943

Simplify the define_query macro #100943

Conversation

jyn514 commented Aug 24, 2022 • edited Loading

jyn514 commented Aug 24, 2022

rust-timer commented Aug 24, 2022

bors commented Aug 24, 2022

bors commented Aug 24, 2022

rust-timer commented Aug 24, 2022

rust-timer commented Aug 24, 2022

Overall result: ✅ improvements - no action needed

Footnotes

jyn514 commented Aug 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyn514 commented Aug 29, 2022

rust-timer commented Aug 29, 2022

bors commented Aug 29, 2022

bors commented Aug 29, 2022

jyn514 commented Sep 1, 2022

bors commented Sep 1, 2022

bors commented Sep 2, 2022

bors commented Sep 2, 2022

rust-timer commented Sep 2, 2022

Overall result: ❌✅ regressions and improvements - no action needed

Footnotes

jyn514 commented Sep 2, 2022

Simplify the `define_query` macro #100943

Simplify the `define_query` macro #100943

jyn514 commented Aug 24, 2022 •

edited

Loading