Move most of `TyCtxtAt::$name` into a generic `evaluate_query` function #101178

jyn514 · 2022-08-30T02:06:12Z

Helps with #96524. This is based on #100943, so it looks larger than it is.

Move Queries::new out of the define_callbacks macro. This is a very small cleanup, happy to move it to its own PR.
Move most of TyCtxtAt::$name into a generic function. There were a couple different approaches I could have taken here, see https://rust-lang.zulipchat.com/#narrow/stream/241847-t-compiler.2Fwg-incr-comp/topic/making.20.60TyCtxtAt.3A.3A.24query.60.20generic.20.2396524. I chose to use a QueryCache generic type argument and pass in tcx.query_caches.$name instead of doing a much larger refactor to define queries::$name in rustc_middle instead of rustc_query_impl.
Remove OnHit generic parameter to try_get_cached. It was always used with copy after my refactor.

jyn514 · 2022-08-30T02:14:12Z

@bors try @rust-timer queue (will be noisy because the other PRs results are mixed in, but the other PR was mostly neutral)

rust-timer · 2022-08-30T02:14:13Z

Awaiting bors try build completion.

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

bors · 2022-08-30T02:14:20Z

⌛ Trying commit 945c5aa1c444ce371df6331df39b15e0695132bd with merge 3a9c65e699f299095772bc2c587402acc7dfa3cb...

bors · 2022-08-30T02:53:50Z

💔 Test failed - checks-actions

compiler/rustc_middle/src/ty/query.rs

bors · 2022-09-01T11:08:32Z

☔ The latest upstream changes (presumably #100869) made this pull request unmergeable. Please resolve the merge conflicts.

cjgillot · 2022-09-02T15:26:18Z

@bors try @rust-timer queue

rust-timer · 2022-09-02T15:26:22Z

Awaiting bors try build completion.

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

bors · 2022-09-02T15:26:29Z

⌛ Trying commit c1584fa72d4b55f73511e40558fc3b209551563e with merge cc5741e85142c1055aaecb2ce088feb90a8b5ab8...

bors · 2022-09-02T16:50:36Z

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

rust-timer · 2022-09-02T16:50:39Z

Queued cc5741e85142c1055aaecb2ce088feb90a8b5ab8 with parent 9353538, future comparison URL.

rust-timer · 2022-09-02T18:04:58Z

Finished benchmarking commit (cc5741e85142c1055aaecb2ce088feb90a8b5ab8): comparison URL.

Overall result: ❌ regressions - 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.6%	[0.3%, 1.2%]	16
Regressions ❌ (secondary)	1.0%	[0.2%, 2.4%]	25
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.0%	[-1.0%, -1.0%]	1
All ❌✅ (primary)	0.6%	[0.3%, 1.2%]	16

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.2%	[2.0%, 2.4%]	2
Regressions ❌ (secondary)	3.0%	[2.0%, 4.0%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.8%	[-7.5%, -1.6%]	5
All ❌✅ (primary)	2.2%	[2.0%, 2.4%]	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)	-	-	0
Regressions ❌ (secondary)	6.1%	[2.3%, 15.8%]	17
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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

bors · 2022-09-07T13:55:13Z

⌛ Trying commit 3d9e006874c1230a89ea43942f314ce5ba23f8f4 with merge 43ab395f7c492c9ec65610e092ad26cb9879373e...

jyn514 · 2022-09-07T13:56:03Z

It will be a shame to have to add back OnHit but I can do so if it improves perf.

jyn514 · 2022-09-07T13:58:30Z

I can also split d2c53ca to a separate PR to rule out any perf impact, I'll do that this evening.

bors · 2022-09-07T15:17:35Z

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

cjgillot · 2022-09-08T17:56:00Z

@rust-timer build 43ab395f7c492c9ec65610e092ad26cb9879373e

rust-timer · 2022-09-08T17:56:02Z

Queued 43ab395f7c492c9ec65610e092ad26cb9879373e with parent e7c7aa7, future comparison URL.

rust-timer · 2022-09-08T19:43:10Z

Finished benchmarking commit (43ab395f7c492c9ec65610e092ad26cb9879373e): 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%, 1.0%]	19
Regressions ❌ (secondary)	1.0%	[0.3%, 2.4%]	41
Improvements ✅ (primary)	-0.4%	[-0.4%, -0.4%]	1
Improvements ✅ (secondary)	-1.3%	[-1.4%, -1.1%]	3
All ❌✅ (primary)	0.6%	[-0.4%, 1.0%]	20

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%, 5.1%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.5%	[-6.2%, -1.3%]	4
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)	-	-	0
Regressions ❌ (secondary)	6.8%	[4.0%, 14.1%]	14
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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

…ly returning Option I think `copy` may have added some overhead? unclear what's causing the perf impact.

jyn514 · 2022-09-10T01:48:39Z

If this doesn't work, I think I may just close the PR; this seems like unnecessarily much work for how little duplication it removes.

jyn514 · 2022-09-10T01:50:07Z

@bors try @rust-timer queue

rust-timer · 2022-09-10T01:50:08Z

Awaiting bors try build completion.

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

bors · 2022-09-10T01:50:16Z

⌛ Trying commit 3675fdd with merge 9c9dde98c61b16603a24ce49807ef1c314a751d2...

bors · 2022-09-10T03:12:46Z

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

rust-timer · 2022-09-10T03:12:48Z

Queued 9c9dde98c61b16603a24ce49807ef1c314a751d2 with parent 1d37ed6, future comparison URL.

rust-timer · 2022-09-10T05:35:10Z

Finished benchmarking commit (9c9dde98c61b16603a24ce49807ef1c314a751d2): comparison URL.

Overall result: ❌ regressions - 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%, 1.9%]	60
Regressions ❌ (secondary)	1.8%	[0.2%, 9.2%]	70
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.5%	[-0.5%, -0.5%]	1
All ❌✅ (primary)	0.7%	[0.2%, 1.9%]	60

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.0%, 1.5%]	3
Regressions ❌ (secondary)	2.0%	[1.3%, 2.4%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.3%	[1.0%, 1.5%]	3

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.5%	[2.1%, 2.9%]	4
Regressions ❌ (secondary)	5.5%	[2.2%, 15.1%]	21
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.5%	[2.1%, 2.9%]	4

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

…llot Move `Queries::new` out of the macro Split out from rust-lang#101178 to make sure it's not contributing to the perf impact. r? `@cjgillot`

Zoxc · 2023-02-03T09:12:42Z

compiler/rustc_middle/src/ty/query.rs

-#[inline(always)]
-fn copy<T: Copy>(x: &T) -> T {
-    *x
+fn evaluate_query<'tcx, Cache>(


Did you try this with #[inline(always)]? The callers all have that so this should regress without it.

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

rust-highfive assigned cjgillot Aug 30, 2022

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

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

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

This comment has been minimized.

Sign in to view

bors 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 30, 2022

This comment has been minimized.

Sign in to view

cjgillot reviewed Aug 30, 2022

View reviewed changes

compiler/rustc_middle/src/ty/query.rs Outdated Show resolved Hide resolved

compiler/rustc_middle/src/ty/query.rs Outdated Show resolved Hide resolved

jyn514 force-pushed the query-system-4 branch 2 times, most recently from 91a5a06 to c1584fa Compare September 2, 2022 00:28

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 Sep 2, 2022

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

jyn514 force-pushed the query-system-4 branch from c1584fa to 3d9e006 Compare September 7, 2022 02:41

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 Sep 10, 2022

jyn514 mentioned this pull request Sep 10, 2022

Move Queries::new out of the macro #101635

Merged

Move most of TyCtxtAt::$name into a generic function

f8b019f

jyn514 force-pushed the query-system-4 branch from 3d9e006 to 07eadaf Compare September 10, 2022 01:47

Get rid of OnHit by switching on QueryMode instead of unconditional…

3675fdd

…ly returning Option I think `copy` may have added some overhead? unclear what's causing the perf impact.

jyn514 force-pushed the query-system-4 branch from 07eadaf to 3675fdd Compare September 10, 2022 01:48

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

jyn514 closed this Sep 10, 2022

jyn514 deleted the query-system-4 branch September 10, 2022 13:57

TaKO8Ki mentioned this pull request Sep 21, 2022

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

Closed

6 tasks

Zoxc reviewed Feb 3, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move most of `TyCtxtAt::$name` into a generic `evaluate_query` function #101178

Move most of `TyCtxtAt::$name` into a generic `evaluate_query` function #101178

jyn514 commented Aug 30, 2022

jyn514 commented Aug 30, 2022

rust-timer commented Aug 30, 2022

bors commented Aug 30, 2022

This comment has been minimized.

bors commented Aug 30, 2022

This comment has been minimized.

bors commented Sep 1, 2022

cjgillot commented Sep 2, 2022

rust-timer commented Sep 2, 2022

bors commented Sep 2, 2022

bors commented Sep 2, 2022

rust-timer commented Sep 2, 2022

rust-timer commented Sep 2, 2022

bors commented Sep 7, 2022

jyn514 commented Sep 7, 2022

jyn514 commented Sep 7, 2022

bors commented Sep 7, 2022

cjgillot commented Sep 8, 2022

rust-timer commented Sep 8, 2022

rust-timer commented Sep 8, 2022

jyn514 commented Sep 10, 2022

jyn514 commented Sep 10, 2022

rust-timer commented Sep 10, 2022

bors commented Sep 10, 2022

bors commented Sep 10, 2022

rust-timer commented Sep 10, 2022

rust-timer commented Sep 10, 2022

Zoxc Feb 3, 2023

Move most of TyCtxtAt::$name into a generic evaluate_query function #101178

Move most of TyCtxtAt::$name into a generic evaluate_query function #101178

Conversation

jyn514 commented Aug 30, 2022

jyn514 commented Aug 30, 2022

rust-timer commented Aug 30, 2022

bors commented Aug 30, 2022

This comment has been minimized.

bors commented Aug 30, 2022

This comment has been minimized.

bors commented Sep 1, 2022

cjgillot commented Sep 2, 2022

rust-timer commented Sep 2, 2022

bors commented Sep 2, 2022

bors commented Sep 2, 2022

rust-timer commented Sep 2, 2022

rust-timer commented Sep 2, 2022

Overall result: ❌ regressions - ACTION NEEDED

Footnotes

bors commented Sep 7, 2022

jyn514 commented Sep 7, 2022

jyn514 commented Sep 7, 2022

bors commented Sep 7, 2022

cjgillot commented Sep 8, 2022

rust-timer commented Sep 8, 2022

rust-timer commented Sep 8, 2022

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Footnotes

jyn514 commented Sep 10, 2022

jyn514 commented Sep 10, 2022

rust-timer commented Sep 10, 2022

bors commented Sep 10, 2022

bors commented Sep 10, 2022

rust-timer commented Sep 10, 2022

rust-timer commented Sep 10, 2022

Overall result: ❌ regressions - ACTION NEEDED

Footnotes

Choose a reason for hiding this comment

Move most of `TyCtxtAt::$name` into a generic `evaluate_query` function #101178

Move most of `TyCtxtAt::$name` into a generic `evaluate_query` function #101178