Factor query arena allocation out from query caches #107833

Zoxc · 2023-02-09T06:52:34Z

This moves the logic for arena allocation out from the query caches into conditional code in the query system. The specialized arena caches are removed. A new QuerySystem type is added in rustc_middle which contains the arenas, providers and query caches.

Performance seems to be slightly regressed:

Benchmark	Before	After
Benchmark	Time	Time	%
🟣 clap:check	1.8053s	1.8109s	0.31%
🟣 hyper:check	0.2600s	0.2597s	-0.10%
🟣 regex:check	0.9973s	1.0006s	0.34%
🟣 syn:check	1.6048s	1.6051s	0.02%
🟣 syntex_syntax:check	6.2992s	6.3159s	0.26%
Total	10.9664s	10.9922s	0.23%
Summary	1.0000s	1.0017s	0.17%

Incremental performance is a bit worse:

Benchmark	Before	After
Benchmark	Time	Time	%
🟣 clap:check:initial	2.2103s	2.2247s	0.65%
🟣 hyper:check:initial	0.3335s	0.3349s	0.41%
🟣 regex:check:initial	1.2597s	1.2650s	0.42%
🟣 syn:check:initial	2.0521s	2.0613s	0.45%
🟣 syntex_syntax:check:initial	7.8275s	7.8583s	0.39%
Total	13.6832s	13.7442s	0.45%
Summary	1.0000s	1.0046s	0.46%

It does seem like LLVM optimizers struggle a bit with the current state of the query system.

Based on top of #107782 and #107802.

r? @cjgillot

cjgillot

I'm not convinced by the direction of this last commit.
My proposal would be to:

migrate all queries to arena-allocate manually and drop arena_cache for them,
remove all the arena support.

cjgillot · 2023-02-09T17:47:20Z

compiler/rustc_middle/src/ty/query.rs

@@ -67,6 +71,24 @@ use std::sync::Arc;
 pub(crate) use rustc_query_system::query::QueryJobId;
 use rustc_query_system::query::*;

+pub struct QuerySystem<'tcx> {
+    pub local_providers: Box<Providers>,
+    pub extern_providers: Box<ExternProviders>,


Why are the providers moved here?

It was convenient to access them using a TyCtxt somewhere.

I do want to try moving the rest of the rustc_query_impl::Queries state to rustc_middle though, as that slightly reduces the size of the query hot path.

I'm sorry, I didn't see where you use them. Could you point it?

This data structure has been put into rustc_query_impl in an effort to shrink rustc_middle and its compile time. I'd rather see stuff moved to rustc_query_impl.

To conclude this discussion, could you keep the current setup of fields in GlobalCtxt? To be re-discussed in another PR that needs those fields moved.

Zoxc · 2023-02-09T22:58:02Z

I kind of view this as an incremental step towards removing arena_cache, though it also goes a bit sideways. I think the provider move can be easily avoided, is there anything other problematic?

bors · 2023-02-12T21:40:07Z

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

oli-obk · 2023-02-13T11:27:02Z

compiler/rustc_interface/src/passes.rs

@@ -795,6 +793,8 @@ pub fn create_global_ctxt<'tcx>(
                untracked,
                dep_graph,
                queries.on_disk_cache.as_ref().map(OnDiskCache::as_dyn),


side note: is there a reason we don't make this a method on QueryEngine? seems weird to pass in a reference to the query engine and to a field of it.

oli-obk · 2023-02-13T11:28:23Z

compiler/rustc_interface/src/passes.rs

+                local_providers,
+                extern_providers,


these could be exposed from the QueryEngine, or if that is a performance issue, we could make the current QueryEngine uses be a struct with a trailing dyn QueryEngine field to have quick access to the fields.

cjgillot

I don't see why this PR needs to be based on top of #107782 and #107802.
Can it be landed separately, or separated from the WorkerLocal changes?

cjgillot · 2023-02-13T12:02:59Z

compiler/rustc_query_system/src/query/plumbing.rs

-            (job, result)
+        // Mark as complete before we remove the job from the active state
+        // so no other thread can re-execute this query.
+        cache.complete(key.clone(), result, dep_node_index);


This changes behaviour for the parallel compiler. IIUC, this is a bugfix, so we should probably land this separately.

cjgillot · 2023-02-13T12:05:54Z

compiler/rustc_middle/src/ty/query.rs

+                    value: query_provided::$name<'tcx>,
+                ) -> query_values::$name<'tcx> {
+                    query_if_arena!([$($modifiers)*]
+                        (&*_tcx.query_system.arenas.$name.alloc(value))


Should we use the global tcx.arena instead?

I've changed this to use the dropless arena for types without destructors at least, which improves performance. We have to add all the query types back to tcx.arena before fully using that.

cjgillot · 2023-02-13T12:07:31Z

compiler/rustc_middle/src/ty/query.rs

-        #[allow(nonstandard_style, unused_lifetimes, unused_parens)]
-        pub mod query_storage {
+        #[allow(nonstandard_style, unused_lifetimes)]
+        pub mod query_provided {


Could you doc-comment on the module that this type is?

cjgillot · 2023-02-13T12:07:50Z

compiler/rustc_middle/src/ty/query.rs

            )*
        }
+        #[allow(nonstandard_style, unused_lifetimes)]
+        pub mod query_provided_to_value {


Could you doc-comment on the module that this does?

cjgillot · 2023-02-13T12:07:59Z

compiler/rustc_middle/src/ty/query.rs


+            $(
+                #[inline]


inline always?

cjgillot · 2023-02-13T12:09:30Z

compiler/rustc_middle/src/ty/query.rs

@@ -67,6 +71,24 @@ use std::sync::Arc;
 pub(crate) use rustc_query_system::query::QueryJobId;
 use rustc_query_system::query::*;

+pub struct QuerySystem<'tcx> {
+    pub local_providers: Box<Providers>,
+    pub extern_providers: Box<ExternProviders>,


To conclude this discussion, could you keep the current setup of fields in GlobalCtxt? To be re-discussed in another PR that needs those fields moved.

cjgillot · 2023-02-13T12:13:23Z

compiler/rustc_query_impl/src/plumbing.rs

+                                qcx,
+                                dep_node
+                            );
+                            value.map(|value| query_provided_to_value::$name(qcx.tcx, value))


Why does this need a call to query_provided_to_value? Could we work with only the query_value here?

arena_cache decodes <V as Deref>::Target so we need to put it on an arena. We can't decode plain V anymore since #73851.

oli-obk

some clones (even if usually no ops/copies) can be avoided now

compiler/rustc_query_system/src/query/caches.rs

Zoxc · 2023-02-14T15:06:47Z

I think I've address all comments now.

The use of the dropless arena was sufficient to offset the regression for non-incremental check:

Benchmark	Before	After
Benchmark	Time	Time	%
🟣 clap:check	1.8226s	1.8204s	-0.12%
🟣 hyper:check	0.2663s	0.2654s	-0.30%
🟣 regex:check	1.0079s	1.0062s	-0.17%
🟣 syn:check	1.6336s	1.6352s	0.10%
🟣 syntex_syntax:check	6.3726s	6.3772s	0.07%
Total	11.1029s	11.1045s	0.01%
Summary	1.0000s	0.9992s	-0.08%

The incremental case still regresses a bit:

Benchmark	Before	After
Benchmark	Time	Time	%
🟣 clap:check:initial	2.2309s	2.2401s	0.41%
🟣 hyper:check:initial	0.3414s	0.3413s	-0.01%
🟣 regex:check:initial	1.2786s	1.2840s	0.43%
🟣 syn:check:initial	2.0676s	2.0772s	0.47%
🟣 syntex_syntax:check:initial	7.9006s	7.9394s	0.49%
Total	13.8191s	13.8821s	0.46%
Summary	1.0000s	1.0036s	0.36%

oli-obk · 2023-02-14T16:11:55Z

@bors try @rust-timer queue

bors · 2023-02-14T16:12:06Z

⌛ Trying commit 257a5a4d57637401596ae8d047b0f576a0dda672 with merge 86f67440d16aa2f389da87483866d18ca64a9c3e...

bors · 2023-02-14T18:59:16Z

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

rust-timer · 2023-02-14T20:20:27Z

Finished benchmarking commit (86f67440d16aa2f389da87483866d18ca64a9c3e): comparison URL.

Overall result: ❌ regressions - 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)	1.3%	[0.3%, 1.8%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
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)	1.1%	[0.4%, 1.8%]	2
Regressions ❌ (secondary)	2.0%	[1.3%, 2.7%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.5%	[-4.5%, -4.5%]	1
All ❌✅ (primary)	1.1%	[0.4%, 1.8%]	2

Cycles

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

cjgillot · 2023-02-14T22:17:15Z

@bors r+

bors · 2023-02-14T22:17:16Z

📌 Commit 257a5a4d57637401596ae8d047b0f576a0dda672 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-02-15T03:23:46Z

⌛ Testing commit 257a5a4d57637401596ae8d047b0f576a0dda672 with merge 76cde7c771243a8bd7e125bc4486fb484327f7a7...

bors · 2023-02-15T04:42:32Z

💔 Test failed - checks-actions

bors · 2023-02-16T11:21:46Z

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

cjgillot · 2023-02-16T18:27:32Z

@bors r+

bors · 2023-02-16T18:27:34Z

📌 Commit caf29b2 has been approved by cjgillot

It is now in the queue for this repository.

cjgillot · 2023-02-16T18:28:16Z

compiler/rustc_query_system/src/query/plumbing.rs

@@ -271,6 +267,7 @@ where
                QueryResult::Poisoned => panic!(),
            }
        };
+        cache.complete(key, result, dep_node_index);


Why isn't this required any more?

Do you mean the extra block that I removed? Not sure when it became redundant.

bors · 2023-02-16T22:10:14Z

⌛ Testing commit caf29b2 with merge 947b696...

bors · 2023-02-17T01:14:32Z

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

rust-timer · 2023-02-17T03:14:39Z

Finished benchmarking commit (947b696): 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
Regressions ❌ (secondary)	1.0%	[0.2%, 1.9%]	4
Improvements ✅ (primary)	-0.3%	[-0.3%, -0.3%]	1
Improvements ✅ (secondary)	-0.3%	[-0.4%, -0.2%]	3
All ❌✅ (primary)	-0.3%	[-0.3%, -0.3%]	1

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.0%	[1.0%, 1.0%]	1
Regressions ❌ (secondary)	1.9%	[1.8%, 2.0%]	2
Improvements ✅ (primary)	-2.1%	[-2.9%, -1.3%]	2
Improvements ✅ (secondary)	-3.1%	[-4.4%, -1.9%]	5
All ❌✅ (primary)	-1.1%	[-2.9%, 1.0%]	3

Cycles

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

Upstream PRs that require local changes: - Switch to EarlyBinder for type_of query rust-lang/rust#107753 - Factor query arena allocation out from query caches rust-lang/rust#107833 Co-authored-by: Qinheping Hu <qinhh@amazon.com>

rustbot assigned cjgillot Feb 9, 2023

cjgillot reviewed Feb 9, 2023

View reviewed changes

Zoxc mentioned this pull request Feb 11, 2023

Erase query result types #107937

Closed

oli-obk reviewed Feb 13, 2023

View reviewed changes

cjgillot reviewed Feb 13, 2023

View reviewed changes

Zoxc force-pushed the arena-query-clean branch from 65f0ce0 to 2d24908 Compare February 14, 2023 13:43

oli-obk reviewed Feb 14, 2023

View reviewed changes

compiler/rustc_query_system/src/query/caches.rs Outdated Show resolved Hide resolved

compiler/rustc_query_system/src/query/caches.rs Outdated Show resolved Hide resolved

compiler/rustc_query_system/src/query/caches.rs Outdated Show resolved Hide resolved

Zoxc force-pushed the arena-query-clean branch from 2d24908 to afa9de7 Compare February 14, 2023 13:59

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 Feb 14, 2023

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 14, 2023

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 Feb 14, 2023

Zoxc added 5 commits February 16, 2023 14:54

Factor query arena allocation out from query caches

a51a205

Use dropless arena when possible

265e1e9

Move provider fields back to rustc_query_impl

067bf2a

Add some comments and use inline(always) on query_provided_to_value

1ae1eac

Remove parallel compiler fix

caf29b2

Zoxc force-pushed the arena-query-clean branch from 257a5a4 to caf29b2 Compare February 16, 2023 13:58

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 Feb 16, 2023

cjgillot reviewed Feb 16, 2023

View reviewed changes

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 17, 2023

bors merged commit 947b696 into rust-lang:master Feb 17, 2023

rustbot added this to the 1.69.0 milestone Feb 17, 2023

Zoxc deleted the arena-query-clean branch February 17, 2023 01:15

This was referenced Feb 17, 2023

Introduce DynSend and DynSync auto trait for parallel compiler #107586

Merged

Don't allow evaluating queries that were fed in a previous compiler run #108046

Merged

rustbot added the perf-regression Performance regression. label Feb 17, 2023

tautschnig mentioned this pull request Apr 17, 2023

Upgrade Rust toolchain to nightly-2023-02-18 model-checking/kani#2384

Merged

3 tasks

matthiaskrgr mentioned this pull request Aug 15, 2023

ICE: 'is_trivially_sized' applied to unexpected type #114872

Closed

matthiaskrgr mentioned this pull request Nov 18, 2023

ice: non_lifetime_binders: bound types encountered in structurally_relate_tys #118038

Open

matthiaskrgr mentioned this pull request Dec 24, 2023

ICE: predicates_of.rs: no entry found for key #119275

Closed

matthiaskrgr mentioned this pull request Jan 1, 2024

ICE: no entry found for key, expr.rs #119489

Closed

matthiaskrgr mentioned this pull request Feb 22, 2024

ICE: could not resolve DefId #121422

Closed

matthiaskrgr mentioned this pull request Mar 27, 2024

ICE: unexpected infer type #123140

Open

Factor query arena allocation out from query caches #107833

Factor query arena allocation out from query caches #107833

Conversation

Zoxc commented Feb 9, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zoxc commented Feb 9, 2023

bors commented Feb 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk left a comment

Choose a reason for hiding this comment

Zoxc commented Feb 14, 2023

oli-obk commented Feb 14, 2023

This comment has been minimized.

bors commented Feb 14, 2023

bors commented Feb 14, 2023

This comment has been minimized.

rust-timer commented Feb 14, 2023

Overall result: ❌ regressions - no action needed

cjgillot commented Feb 14, 2023

bors commented Feb 14, 2023

bors commented Feb 15, 2023

bors commented Feb 15, 2023

bors commented Feb 16, 2023

cjgillot commented Feb 16, 2023

bors commented Feb 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Feb 16, 2023

bors commented Feb 17, 2023

rust-timer commented Feb 17, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED