Move query accessor code into functions #107802

Zoxc · 2023-02-08T14:52:05Z

This reduces the amount of code generated by the macros, which I think is the last item on #96524.

It also improves performance.

Benchmark	Before	After
Benchmark	Time	Time	%
🟣 clap:check	1.8271s	1.8163s	-0.59%
🟣 hyper:check	0.2611s	0.2597s	-0.57%
🟣 regex:check	1.0206s	1.0124s	-0.80%
🟣 syn:check	1.6329s	1.6158s	💚 -1.05%
🟣 syntex_syntax:check	6.3299s	6.2816s	-0.76%
Total	11.0717s	10.9858s	-0.78%
Summary	1.0000s	0.9925s	-0.75%

r? @cjgillot

cjgillot · 2023-02-08T17:58:14Z

@bors try @rust-timer queue

bors · 2023-02-08T17:58:23Z

⌛ Trying commit 1fb91b54f117c61f166856ecaf0caca83f84bd03 with merge 203f4baf05d15e06f81a0776f7b923a3f6a84c41...

bors · 2023-02-08T20:49:52Z

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

rust-timer · 2023-02-08T22:51:07Z

Finished benchmarking commit (203f4baf05d15e06f81a0776f7b923a3f6a84c41): 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-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.4%	[0.2%, 0.8%]	8
Regressions ❌ (secondary)	5.1%	[1.2%, 6.2%]	7
Improvements ✅ (primary)	-1.4%	[-1.9%, -0.5%]	3
Improvements ✅ (secondary)	-3.7%	[-4.2%, -3.2%]	6
All ❌✅ (primary)	-0.1%	[-1.9%, 0.8%]	11

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)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
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)	11.3%	[10.3%, 12.3%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

cjgillot · 2023-02-09T17:39:13Z

Both benchmarks and bootstrap timing show a regression. Do you have an idea why? How can this be avoided?

Zoxc · 2023-02-09T18:29:07Z

There's a 0.28% increase in code size which is likely LLVM inlining more and regressing the compile time of the rustc crates. The match-stress benchmark does seem to be quite flaky. I don't think that one is meaningful. It seems to regress when compiling with 1 CGU too.

cjgillot · 2023-02-09T20:01:33Z

compiler/rustc_middle/src/ty/query.rs

+}
+
+#[inline(always)]
+fn query_ensure<'tcx, Cache, K>(


Can this be merged with query_get_at by adding a QueryMode parameter and returning an Option?

This change was a regression:

Benchmark Before After
Time Time %
🟣 clap:check 1.7607s 1.7700s 0.52%
🟣 hyper:check 0.2565s 0.2576s 0.44%
🟣 regex:check 0.9807s 0.9862s 0.56%
🟣 syn:check 1.5808s 1.5889s 0.51%
🟣 syntex_syntax:check 6.1378s 6.1567s 0.31%
Total 10.7165s 10.7594s 0.40%
Summary 1.0000s 1.0047s 0.47%

cjgillot · 2023-02-09T20:04:43Z

compiler/rustc_middle/src/ty/query.rs

+#[inline(always)]
+fn query_get_at<'tcx, Cache, K>(
+    tcx: TyCtxt<'tcx>,
+    execute_query: fn(


The regression probably comes from here: an indirect call (fn pointer) to an indirect call (dyn QueryEngine).

There's only 1 indirect call, same as before.

Actually you're right. LLVM will inline it anyway though as query_get_at is #[inline(always)] so it will discover the function it points to.

cjgillot · 2023-02-09T20:05:32Z

compiler/rustc_middle/src/ty/query.rs

+    Cache::Stored: Copy,
+    Cache: QueryCache,
+{
+    let key = key.into_query_param();


Moving this to the caller will make query_get_at a bit less generic, hopefully improving compile time.

This change was performance neutral and rustc_driver size increased by 0.03%, which is not a good sign for compile time.

Benchmark Before After
Time Time %
🟣 clap:check 1.7610s 1.7607s -0.02%
🟣 hyper:check 0.2569s 0.2570s 0.07%
🟣 regex:check 0.9808s 0.9815s 0.07%
🟣 syn:check 1.5819s 1.5814s -0.03%
🟣 syntex_syntax:check 6.1439s 6.1434s -0.01%
Total 10.7246s 10.7241s -0.00%
Summary 1.0000s 1.0002s 0.02%

I've pushed it so we can see what perf says.

cjgillot · 2023-02-10T21:56:45Z

@bors try @rust-timer queue

bors · 2023-02-10T21:56:54Z

⌛ Trying commit e046b4ca1ef602fd8379bb93a75f7864fb297400 with merge 8bdd2e2179ccbdd8546274274ab39ff27d50aef2...

bors · 2023-02-11T00:37:15Z

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

bors · 2023-02-11T00:37:15Z

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

rust-timer · 2023-02-11T04:04:18Z

Finished benchmarking commit (8bdd2e2179ccbdd8546274274ab39ff27d50aef2): 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-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%, 0.9%]	7
Regressions ❌ (secondary)	5.8%	[5.4%, 6.2%]	6
Improvements ✅ (primary)	-0.4%	[-0.5%, -0.3%]	3
Improvements ✅ (secondary)	-0.5%	[-0.5%, -0.4%]	4
All ❌✅ (primary)	0.3%	[-0.5%, 0.9%]	10

Max RSS (memory usage)

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

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)	8.0%	[3.5%, 11.3%]	9
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Zoxc · 2023-02-11T09:57:39Z

I changed the code to use self.at again as it was a performance win:

Benchmark	Before	After
Benchmark	Time	Time	%
🟣 clap:check	1.7640s	1.7568s	-0.41%
🟣 hyper:check	0.2574s	0.2562s	-0.44%
🟣 regex:check	0.9856s	0.9826s	-0.31%
🟣 syn:check	1.5816s	1.5752s	-0.41%
🟣 syntex_syntax:check	6.1438s	6.1225s	-0.35%
Total	10.7325s	10.6933s	-0.36%
Summary	1.0000s	0.9962s	-0.38%

The overall change to this PR seems to improve bitmaps locally:

Benchmark	Before	After
Benchmark	Time	Time	%
🟣 bitmaps:check	0.8432s	0.8145s	💚 -3.40%
Total	0.8432s	0.8145s	💚 -3.40%
Summary	1.0000s	0.9660s	💚 -3.40%

bors · 2023-03-12T17:38:39Z

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

Remove `QueryEngine` trait This removes the `QueryEngine` trait and `Queries` from `rustc_query_impl` and replaced them with function pointers and fields in `QuerySystem`. As a side effect `OnDiskCache` is moved back into `rustc_middle` and the `OnDiskCache` trait is also removed. This has a couple of benefits. - `TyCtxt` is used in the query system instead of the removed `QueryCtxt` which is larger. - Function pointers are more flexible to work with. A variant of rust-lang#107802 is included which avoids the double indirection. For rust-lang#108938 we can name entry point `__rust_end_short_backtrace` to avoid some overhead. For rust-lang#108062 it avoids the duplicate `QueryEngine` structs. - `QueryContext` now implements `DepContext` which avoids many `dep_context()` calls in `rustc_query_system`. - The `rustc_driver` size is reduced by 0.33%, hopefully that means some bootstrap improvements. - This avoids the unsafe code around the `QueryEngine` trait. r? `@cjgillot`

Zoxc · 2023-04-30T07:46:24Z

This was mostly merged as part of #109611. The remaining attributes could use a perf run.

cjgillot · 2023-04-30T09:00:06Z

@bors try @rust-timer queue

bors · 2023-04-30T09:00:14Z

⌛ Trying commit b6720ad with merge 2aa83d2eb45fdf777462706ce8ac5e6e0407f4a4...

bors · 2023-04-30T11:15:16Z

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

bors · 2023-04-30T11:15:16Z

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

rust-timer · 2023-04-30T13:10:17Z

Finished benchmarking commit (2aa83d2eb45fdf777462706ce8ac5e6e0407f4a4): 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-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.5%	[0.5%, 0.5%]	1
Improvements ✅ (primary)	-0.5%	[-2.8%, -0.2%]	31
Improvements ✅ (secondary)	-1.4%	[-7.3%, -0.3%]	36
All ❌✅ (primary)	-0.5%	[-2.8%, -0.2%]	31

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.4%	[0.5%, 3.0%]	8
Regressions ❌ (secondary)	2.5%	[1.8%, 3.8%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.4%	[0.5%, 3.0%]	8

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)	1.9%	[1.8%, 2.1%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.9%	[-3.1%, -2.6%]	2
All ❌✅ (primary)	1.9%	[1.8%, 2.1%]	3

cjgillot · 2023-04-30T13:24:30Z

r=me with PR title updated

Zoxc · 2023-04-30T14:12:28Z

This no longer seems to be an improvement locally and the bootstrap cost is quite high, so I think I'll close this for now.

Benchmark	Before	After		Before	After
Benchmark	Time	Time	%	Memory	Memory	%
🟣 clap:check	1.6276s	1.6328s	0.32%	86.36 MiB	86.57 MiB	0.24%
🟣 hyper:check	0.2446s	0.2444s	-0.08%	63.39 MiB	63.70 MiB	0.49%
🟣 regex:check	0.9089s	0.9124s	0.38%	100.40 MiB	100.73 MiB	0.32%
🟣 syn:check	1.4859s	1.4850s	-0.06%	122.92 MiB	123.23 MiB	0.26%
🟣 syntex_syntax:check	5.7065s	5.7227s	0.28%	329.77 MiB	329.48 MiB	-0.09%
Total	9.9734s	9.9971s	0.24%	702.84 MiB	703.71 MiB	0.12%
Summary	1.0000s	1.0017s	0.17%	1 byte	1.00 bytes	0.24%

Remove `QueryEngine` trait This removes the `QueryEngine` trait and `Queries` from `rustc_query_impl` and replaced them with function pointers and fields in `QuerySystem`. As a side effect `OnDiskCache` is moved back into `rustc_middle` and the `OnDiskCache` trait is also removed. This has a couple of benefits. - `TyCtxt` is used in the query system instead of the removed `QueryCtxt` which is larger. - Function pointers are more flexible to work with. A variant of rust-lang/rust#107802 is included which avoids the double indirection. For rust-lang/rust#108938 we can name entry point `__rust_end_short_backtrace` to avoid some overhead. For rust-lang/rust#108062 it avoids the duplicate `QueryEngine` structs. - `QueryContext` now implements `DepContext` which avoids many `dep_context()` calls in `rustc_query_system`. - The `rustc_driver` size is reduced by 0.33%, hopefully that means some bootstrap improvements. - This avoids the unsafe code around the `QueryEngine` trait. r? `@cjgillot`

rustbot assigned cjgillot Feb 8, 2023