Remove `QueryEngine` trait #109611

Zoxc · 2023-03-25T21:19:32Z

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 Move query accessor code into functions #107802 is included which avoids the double indirection. For Shorten backtraces for queries in ICEs #108938 we can name entry point __rust_end_short_backtrace to avoid some overhead. For Specialize query execution for incremental and non-incremental #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 · 2023-03-25T21:19:40Z

These commits modify the Cargo.lock file. Random changes to Cargo.lock can be introduced when switching branches and rebasing PRs.
This was probably unintentional and should be reverted before this PR is merged.

If this was intentional then you can ignore this comment.

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Noratrieb · 2023-03-25T21:45:39Z

@bors try @rust-timer queue

bors · 2023-03-25T21:45:48Z

⌛ Trying commit 6ff4ec38724688fe2d03e6042a44db705454e11d with merge e917dc60af05173dcd1ddae988d3f4bd9ae00fab...

bors · 2023-03-25T23:42:09Z

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

rust-timer · 2023-03-26T03:30:44Z

Finished benchmarking commit (e917dc60af05173dcd1ddae988d3f4bd9ae00fab): 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)	1.0%	[0.3%, 1.5%]	13
Regressions ❌ (secondary)	0.7%	[0.3%, 2.2%]	19
Improvements ✅ (primary)	-2.7%	[-3.5%, -0.2%]	5
Improvements ✅ (secondary)	-1.3%	[-3.1%, -0.4%]	43
All ❌✅ (primary)	-0.0%	[-3.5%, 1.5%]	18

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.5%	[0.4%, 0.5%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.0%	[-2.2%, -0.4%]	7
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)	2.1%	[1.8%, 2.4%]	3
Regressions ❌ (secondary)	1.4%	[0.7%, 4.5%]	11
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.6%	[-3.4%, -0.4%]	9
All ❌✅ (primary)	2.1%	[1.8%, 2.4%]	3

oli-obk · 2023-03-27T13:44:57Z

compiler/rustc_middle/src/mir/interpret/mod.rs

+            // FIXME: This doesn't encode `did` directly due to some trait specialization error.
+            encode_def_id(did, encoder);


Please include more information about the failure in the comment, as well as the code this should be so it gets addressed in the future

I had a look locally and found that using function syntax instead of method syntax works. I pushed the fix directly to your branch

compiler/rustc_middle/src/query/on_disk_cache.rs

oli-obk · 2023-03-27T13:53:05Z

Please have a look at the perf regressions with cachegrind. They seem ok, but we should at least make an attempt at resolving them.

the change itself lgtm.

Zoxc · 2023-03-28T03:24:11Z

It's not easy to read anything useful from cachegrind with lots of inlining differences and function renaming. I added inlining attributes to compensate for the OnDiskCache location change. It helps for unicode-normalization:check:unchanged locally at least.

oli-obk · 2023-03-28T07:34:36Z

@bors try @rust-timer queue

bors · 2023-03-28T07:34:45Z

⌛ Trying commit b1ae7398814387a28fae32f91a0ee4d82d243eb0 with merge 0f23ca9ea2278f1bd63917535c55886c928388b4...

bors · 2023-03-28T09:33:51Z

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

rust-timer · 2023-03-28T10:49:37Z

Finished benchmarking commit (0f23ca9ea2278f1bd63917535c55886c928388b4): 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.5%	[0.4%, 0.7%]	6
Regressions ❌ (secondary)	0.5%	[0.3%, 0.6%]	6
Improvements ✅ (primary)	-2.8%	[-2.9%, -2.6%]	4
Improvements ✅ (secondary)	-1.5%	[-2.6%, -1.1%]	22
All ❌✅ (primary)	-0.8%	[-2.9%, 0.7%]	10

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.6%	[2.8%, 4.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.4%	[-3.1%, -1.7%]	2
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)	-	-	0
Improvements ✅ (primary)	-2.5%	[-3.5%, -1.6%]	5
Improvements ✅ (secondary)	-3.0%	[-3.5%, -1.9%]	18
All ❌✅ (primary)	-2.5%	[-3.5%, -1.6%]	5

Zoxc · 2023-03-28T11:34:14Z

bitmaps seems to be a wall time improvement locally:

Benchmark	Before	After
Benchmark	Time	Time	%
🟣 bitmaps:check	0.7687s	0.7623s	-0.83%
🟣 bitmaps:check:unchanged	0.3960s	0.3919s	💚 -1.03%
Total	1.1646s	1.1542s	-0.90%
Summary	1.0000s	0.9907s	-0.93%

oli-obk · 2023-04-20T08:04:20Z

r? @cjgillot

bors · 2023-04-21T11:20:53Z

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

bors · 2023-04-21T22:45:48Z

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

bors · 2023-04-24T22:17:37Z

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

bors · 2023-04-26T05:43:59Z

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

cjgillot · 2023-04-29T20:34:26Z

@bors r+

bors · 2023-04-29T20:34:28Z

📌 Commit b694373 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-04-29T21:58:17Z

⌛ Testing commit b694373 with merge f5adff6...

bors · 2023-04-30T00:30:55Z

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

rust-timer · 2023-04-30T02:34:16Z

Finished benchmarking commit (f5adff6): comparison URL.

Overall result: ✅ 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.3%	[0.3%, 0.3%]	3
Improvements ✅ (primary)	-0.6%	[-0.8%, -0.2%]	84
Improvements ✅ (secondary)	-0.8%	[-1.5%, -0.4%]	32
All ❌✅ (primary)	-0.6%	[-0.8%, -0.2%]	84

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%	[0.7%, 4.8%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.7%	[-3.7%, -3.7%]	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)	-	-	0
Improvements ✅ (primary)	-2.4%	[-3.4%, -1.6%]	6
Improvements ✅ (secondary)	-3.0%	[-3.5%, -2.1%]	15
All ❌✅ (primary)	-2.4%	[-3.4%, -1.6%]	6

rustbot assigned cjgillot Mar 25, 2023

Zoxc force-pushed the query-engine-rem branch from 2cf3dfa to 6ff4ec3 Compare March 25, 2023 21:34

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 25, 2023

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Mar 26, 2023

oli-obk reviewed Mar 27, 2023

View reviewed changes

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

Zoxc force-pushed the query-engine-rem branch from 6ff4ec3 to b1ae739 Compare March 28, 2023 03:20

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 28, 2023

This comment has been minimized.

Sign in to view

rustbot assigned cjgillot and unassigned oli-obk Apr 20, 2023

Zoxc force-pushed the query-engine-rem branch from 9f86f2e to 0cddc65 Compare April 21, 2023 12:04

Zoxc force-pushed the query-engine-rem branch from 0cddc65 to 063601a Compare April 22, 2023 00:32

Zoxc force-pushed the query-engine-rem branch from 063601a to a89309f Compare April 25, 2023 01:40

Zoxc added 4 commits April 26, 2023 07:46

Move on_disk_cache.rs

897a146

Remove QueryEngine trait

66d8543

Add query accessor functions

4440e81

Inline tweaks

b694373

Zoxc force-pushed the query-engine-rem branch from a89309f to b694373 Compare April 26, 2023 05:57

chenyukang mentioned this pull request Apr 29, 2023

Shorten backtraces for queries in ICEs #108938

Merged

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 Apr 29, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 30, 2023

bors merged commit f5adff6 into rust-lang:master Apr 30, 2023

rustbot added this to the 1.71.0 milestone Apr 30, 2023

bors mentioned this pull request Apr 30, 2023

Avoid code generation for ThinVec<Diagnostic>'s destructor in the query system #108359

Merged

rustbot removed the perf-regression Performance regression. label Apr 30, 2023

Zoxc deleted the query-engine-rem branch April 30, 2023 06:04

Zoxc mentioned this pull request Apr 30, 2023

Move query accessor code into functions #107802

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `QueryEngine` trait #109611

Remove `QueryEngine` trait #109611

Zoxc commented Mar 25, 2023 •

edited

Loading

rustbot commented Mar 25, 2023

Noratrieb commented Mar 25, 2023

This comment has been minimized.

bors commented Mar 25, 2023

bors commented Mar 25, 2023

This comment has been minimized.

rust-timer commented Mar 26, 2023

oli-obk Mar 27, 2023

oli-obk Mar 28, 2023

oli-obk commented Mar 27, 2023

Zoxc commented Mar 28, 2023

oli-obk commented Mar 28, 2023

This comment has been minimized.

bors commented Mar 28, 2023

bors commented Mar 28, 2023

This comment has been minimized.

rust-timer commented Mar 28, 2023

Zoxc commented Mar 28, 2023

oli-obk commented Apr 20, 2023

bors commented Apr 21, 2023

bors commented Apr 21, 2023

bors commented Apr 24, 2023

bors commented Apr 26, 2023

cjgillot commented Apr 29, 2023

bors commented Apr 29, 2023

bors commented Apr 29, 2023

bors commented Apr 30, 2023

rust-timer commented Apr 30, 2023

		// FIXME: This doesn't encode `did` directly due to some trait specialization error.
		encode_def_id(did, encoder);

Remove QueryEngine trait #109611

Remove QueryEngine trait #109611

Conversation

Zoxc commented Mar 25, 2023 • edited Loading

rustbot commented Mar 25, 2023

Noratrieb commented Mar 25, 2023

This comment has been minimized.

bors commented Mar 25, 2023

bors commented Mar 25, 2023

This comment has been minimized.

rust-timer commented Mar 26, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Mar 27, 2023

Zoxc commented Mar 28, 2023

oli-obk commented Mar 28, 2023

This comment has been minimized.

bors commented Mar 28, 2023

bors commented Mar 28, 2023

This comment has been minimized.

rust-timer commented Mar 28, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Zoxc commented Mar 28, 2023

oli-obk commented Apr 20, 2023

bors commented Apr 21, 2023

bors commented Apr 21, 2023

bors commented Apr 24, 2023

bors commented Apr 26, 2023

cjgillot commented Apr 29, 2023

bors commented Apr 29, 2023

bors commented Apr 29, 2023

bors commented Apr 30, 2023

rust-timer commented Apr 30, 2023

Overall result: ✅ improvements - no action needed

Remove `QueryEngine` trait #109611

Remove `QueryEngine` trait #109611

Zoxc commented Mar 25, 2023 •

edited

Loading