Erase query result types #107937

Zoxc · 2023-02-11T21:04:18Z

This replaces concrete query values V with MaybeUninit<[u8; { size_of::<V>() }]> reducing the code instantiated by queries. This reduces instances of qet_query in rustc_query_impl from 290 to 94. The compile time of rustc_query_impl is reduced by 33%.

This is achieved by introducing an Erased type which does sanity check with cfg(debug_assertions). The query caches gets instatiated with these erased types leaving the code in rustc_query_system unaware of them. rustc_query_system is changed to use instances of QueryConfig so that rustc_query_impl can pass in ErasedQuery which holds a pointer to a virtual table. To allow the virtual table to be stored as a static variable, new Remap and RemapQueryCache traits are introduced which can map types from 'static back to 'tcx.

This regresses performance somewhat. There might be some opportunities for specialization to recover it.

Benchmark	Before	After
Benchmark	Time	Time	%
🟣 clap:check	1.7735s	1.7784s	0.28%
🟣 hyper:check	0.2582s	0.2588s	0.22%
🟣 regex:check	0.9826s	0.9869s	0.44%
🟣 syn:check	1.5866s	1.5943s	0.49%
🟣 syntex_syntax:check	6.1442s	6.1949s	0.82%
Total	10.7452s	10.8133s	0.63%
Summary	1.0000s	1.0045s	0.45%

It's based on #107833 and the performance measurement is relative to it.

cc @oli-obk for an option on how stable feature(generic_const_exprs) is.

r? @cjgillot

# Conflicts: # compiler/rustc_middle/src/ty/query.rs

…tructures

rustbot · 2023-02-11T21:04:27Z

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.

cjgillot · 2023-02-11T21:13:27Z

compiler/rustc_data_structures/src/remap.rs

@@ -0,0 +1,28 @@
+/// Remaps the type with a different lifetime for 'tcx if applicable.
+pub trait Remap {


There is rustc_middle::ty::parametrized::ParameterizedOverTcx that does the same thing.

cjgillot · 2023-02-11T21:20:09Z

compiler/rustc_query_impl/src/lib.rs


 mod on_disk_cache;
 pub use on_disk_cache::OnDiskCache;

 mod profiling_support;
 pub use self::profiling_support::alloc_self_profile_query_strings;

+struct DynamicQuery<C: QueryCache + RemapQueryCache> {


Could you put the introduction of this struct and related changes in a separate commit from the type-erasure change?

Given the feedback by @BoxyUwU I'll split the rustc_query_system and virtual dispatch out to separate PRs.

cjgillot · 2023-02-12T15:15:00Z

compiler/rustc_erase/src/lib.rs

+pub struct Erased<const N: usize> {
+    data: MaybeUninit<[u8; N]>,
+    #[cfg(debug_assertions)]
+    type_id: &'static str,


Should we prefer std::any::TypeId?

That requires 'static.

BoxyUwU · 2023-02-12T20:38:45Z

compiler/rustc_erase/src/lib.rs

@@ -0,0 +1,47 @@
+// This is a separate crate so that we can `allow(incomplete_features)` for just `generic_const_exprs`
+#![feature(generic_const_exprs)]


Please do not use generic_const_exprs, its incredibly incomplete/broken and also its unsound and will have changes made to it that breaks code that works currently in order to get closer to an actually functioning feature. I do not want to have to deal with cfg(bootstrap) pain and working around generic_const_expr bugs from usage of the feature inside of rustc whenever this feature gets worked on ^^'

I guess to explicitly answer

for an option on how stable feature(generic_const_exprs) is.

extremely very not stable 😅

bors · 2023-02-12T21:41:59Z

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

Noratrieb · 2023-02-14T14:04:08Z

compiler/rustc_erase/Cargo.toml

@@ -0,0 +1,6 @@
+[package]
+name = "rustc_erase"


Why is this inside a new crate and not just in rustc_data_structures? Creating a crate for 47 lines seems pointless, even if the code is very unsafe and self-contained

// This is a separate crate so that we can `allow(incomplete_features)` for just `generic_const_exprs`

Ah, didn't see that.

Note that generic_const_exprs will cause ICEs in other crates than the one using the feature if any of it is used in public APIs

…jgillot Make `rustc_query_system` take `QueryConfig` by instance. This allows for easy switching between virtual tables and specialized instances for queries. It also has the benefit of less turbofish. `QueryStorage` has also been merged with `QueryCache`. Split out from rust-lang#107937. r? `@cjgillot`

Make `rustc_query_system` take `QueryConfig` by instance. This allows for easy switching between virtual tables and specialized instances for queries. It also has the benefit of less turbofish. `QueryStorage` has also been merged with `QueryCache`. Split out from rust-lang/rust#107937. r? `@cjgillot`

Use dynamic dispatch for queries This replaces most concrete query values `V` with `MaybeUninit<[u8; { size_of::<V>() }]>` reducing the code instantiated by queries. The compile time of `rustc_query_impl` is reduced by 27%. It is an alternative to rust-lang#107937 which uses unstable const generics while this uses a `EraseType` trait which maps query values to their erased variant. This is achieved by introducing an `Erased` type which does sanity check with `cfg(debug_assertions)`. The query caches gets instantiated with these erased types leaving the code in `rustc_query_system` unaware of them. `rustc_query_system` is changed to use instances of `QueryConfig` so that `rustc_query_impl` can pass in `DynamicConfig` which holds a pointer to a virtual table. <table><tr><td rowspan="2">Benchmark</td><td colspan="1">Before</th><td colspan="2">After</th></tr><tr><td align="right">Time</td><td align="right">Time</td><td align="right">%</th></tr><tr><td>🟣 clap:check</td><td align="right">1.7055s</td><td align="right">1.6949s</td><td align="right"> -0.62%</td></tr><tr><td>🟣 hyper:check</td><td align="right">0.2547s</td><td align="right">0.2528s</td><td align="right"> -0.73%</td></tr><tr><td>🟣 regex:check</td><td align="right">0.9590s</td><td align="right">0.9553s</td><td align="right"> -0.39%</td></tr><tr><td>🟣 syn:check</td><td align="right">1.5457s</td><td align="right">1.5440s</td><td align="right"> -0.11%</td></tr><tr><td>🟣 syntex_syntax:check</td><td align="right">5.9092s</td><td align="right">5.9009s</td><td align="right"> -0.14%</td></tr><tr><td>Total</td><td align="right">10.3741s</td><td align="right">10.3479s</td><td align="right"> -0.25%</td></tr><tr><td>Summary</td><td align="right">1.0000s</td><td align="right">0.9960s</td><td align="right"> -0.40%</td></tr></table> <table><tr><td rowspan="2">Benchmark</td><td colspan="1">Before</th><td colspan="2">After</th></tr><tr><td align="right">Time</td><td align="right">Time</td><td align="right">%</th></tr><tr><td>🟣 clap:check:initial</td><td align="right">2.0605s</td><td align="right">2.0575s</td><td align="right"> -0.15%</td></tr><tr><td>🟣 hyper:check:initial</td><td align="right">0.3218s</td><td align="right">0.3216s</td><td align="right"> -0.07%</td></tr><tr><td>🟣 regex:check:initial</td><td align="right">1.1848s</td><td align="right">1.1839s</td><td align="right"> -0.07%</td></tr><tr><td>🟣 syn:check:initial</td><td align="right">1.9409s</td><td align="right">1.9376s</td><td align="right"> -0.17%</td></tr><tr><td>🟣 syntex_syntax:check:initial</td><td align="right">7.3105s</td><td align="right">7.2928s</td><td align="right"> -0.24%</td></tr><tr><td>Total</td><td align="right">12.8185s</td><td align="right">12.7935s</td><td align="right"> -0.20%</td></tr><tr><td>Summary</td><td align="right">1.0000s</td><td align="right">0.9986s</td><td align="right"> -0.14%</td></tr></table> <table><tr><td rowspan="2">Benchmark</td><td colspan="1">Before</th><td colspan="2">After</th></tr><tr><td align="right">Time</td><td align="right">Time</td><td align="right">%</th></tr><tr><td>🟣 clap:check:unchanged</td><td align="right">0.4606s</td><td align="right">0.4617s</td><td align="right"> 0.24%</td></tr><tr><td>🟣 hyper:check:unchanged</td><td align="right">0.1335s</td><td align="right">0.1336s</td><td align="right"> 0.08%</td></tr><tr><td>🟣 regex:check:unchanged</td><td align="right">0.3324s</td><td align="right">0.3346s</td><td align="right"> 0.65%</td></tr><tr><td>🟣 syn:check:unchanged</td><td align="right">0.6268s</td><td align="right">0.6307s</td><td align="right"> 0.64%</td></tr><tr><td>🟣 syntex_syntax:check:unchanged</td><td align="right">1.8248s</td><td align="right">1.8508s</td><td align="right">💔 1.43%</td></tr><tr><td>Total</td><td align="right">3.3779s</td><td align="right">3.4113s</td><td align="right"> 0.99%</td></tr><tr><td>Summary</td><td align="right">1.0000s</td><td align="right">1.0061s</td><td align="right"> 0.61%</td></tr></table> It's based on rust-lang#108167. r? `@cjgillot`

Use dynamic dispatch for queries This replaces most concrete query values `V` with `MaybeUninit<[u8; { size_of::<V>() }]>` reducing the code instantiated by queries. The compile time of `rustc_query_impl` is reduced by 27%. It is an alternative to rust-lang/rust#107937 which uses unstable const generics while this uses a `EraseType` trait which maps query values to their erased variant. This is achieved by introducing an `Erased` type which does sanity check with `cfg(debug_assertions)`. The query caches gets instantiated with these erased types leaving the code in `rustc_query_system` unaware of them. `rustc_query_system` is changed to use instances of `QueryConfig` so that `rustc_query_impl` can pass in `DynamicConfig` which holds a pointer to a virtual table. <table><tr><td rowspan="2">Benchmark</td><td colspan="1">Before</th><td colspan="2">After</th></tr><tr><td align="right">Time</td><td align="right">Time</td><td align="right">%</th></tr><tr><td>🟣 clap:check</td><td align="right">1.7055s</td><td align="right">1.6949s</td><td align="right"> -0.62%</td></tr><tr><td>🟣 hyper:check</td><td align="right">0.2547s</td><td align="right">0.2528s</td><td align="right"> -0.73%</td></tr><tr><td>🟣 regex:check</td><td align="right">0.9590s</td><td align="right">0.9553s</td><td align="right"> -0.39%</td></tr><tr><td>🟣 syn:check</td><td align="right">1.5457s</td><td align="right">1.5440s</td><td align="right"> -0.11%</td></tr><tr><td>🟣 syntex_syntax:check</td><td align="right">5.9092s</td><td align="right">5.9009s</td><td align="right"> -0.14%</td></tr><tr><td>Total</td><td align="right">10.3741s</td><td align="right">10.3479s</td><td align="right"> -0.25%</td></tr><tr><td>Summary</td><td align="right">1.0000s</td><td align="right">0.9960s</td><td align="right"> -0.40%</td></tr></table> <table><tr><td rowspan="2">Benchmark</td><td colspan="1">Before</th><td colspan="2">After</th></tr><tr><td align="right">Time</td><td align="right">Time</td><td align="right">%</th></tr><tr><td>🟣 clap:check:initial</td><td align="right">2.0605s</td><td align="right">2.0575s</td><td align="right"> -0.15%</td></tr><tr><td>🟣 hyper:check:initial</td><td align="right">0.3218s</td><td align="right">0.3216s</td><td align="right"> -0.07%</td></tr><tr><td>🟣 regex:check:initial</td><td align="right">1.1848s</td><td align="right">1.1839s</td><td align="right"> -0.07%</td></tr><tr><td>🟣 syn:check:initial</td><td align="right">1.9409s</td><td align="right">1.9376s</td><td align="right"> -0.17%</td></tr><tr><td>🟣 syntex_syntax:check:initial</td><td align="right">7.3105s</td><td align="right">7.2928s</td><td align="right"> -0.24%</td></tr><tr><td>Total</td><td align="right">12.8185s</td><td align="right">12.7935s</td><td align="right"> -0.20%</td></tr><tr><td>Summary</td><td align="right">1.0000s</td><td align="right">0.9986s</td><td align="right"> -0.14%</td></tr></table> <table><tr><td rowspan="2">Benchmark</td><td colspan="1">Before</th><td colspan="2">After</th></tr><tr><td align="right">Time</td><td align="right">Time</td><td align="right">%</th></tr><tr><td>🟣 clap:check:unchanged</td><td align="right">0.4606s</td><td align="right">0.4617s</td><td align="right"> 0.24%</td></tr><tr><td>🟣 hyper:check:unchanged</td><td align="right">0.1335s</td><td align="right">0.1336s</td><td align="right"> 0.08%</td></tr><tr><td>🟣 regex:check:unchanged</td><td align="right">0.3324s</td><td align="right">0.3346s</td><td align="right"> 0.65%</td></tr><tr><td>🟣 syn:check:unchanged</td><td align="right">0.6268s</td><td align="right">0.6307s</td><td align="right"> 0.64%</td></tr><tr><td>🟣 syntex_syntax:check:unchanged</td><td align="right">1.8248s</td><td align="right">1.8508s</td><td align="right">💔 1.43%</td></tr><tr><td>Total</td><td align="right">3.3779s</td><td align="right">3.4113s</td><td align="right"> 0.99%</td></tr><tr><td>Summary</td><td align="right">1.0000s</td><td align="right">1.0061s</td><td align="right"> 0.61%</td></tr></table> It's based on rust-lang/rust#108167. r? `@cjgillot`

Zoxc added 4 commits February 9, 2023 07:37

Move query accessor code into functions

77eb2be

# Conflicts: # compiler/rustc_middle/src/ty/query.rs

Move the WorkerLocal type from the rustc-rayon fork into rustc_data_s…

97befd4

…tructures

Factor query arena allocation out from query caches

65f0ce0

Erase query return types

96f07a7

rustbot assigned cjgillot Feb 11, 2023

Fix debug_assertions

e4a6b06

cjgillot reviewed Feb 11, 2023

View reviewed changes

Zoxc added 2 commits February 11, 2023 22:23

Add PhantomData for erased query result types

3d9ce6c

Fix cfg(debug_assertions)

6efc404

Zoxc force-pushed the erase-query-results branch from 61b8bab to 6efc404 Compare February 11, 2023 22:15

cjgillot reviewed Feb 12, 2023

View reviewed changes

BoxyUwU reviewed Feb 12, 2023

View reviewed changes

BoxyUwU mentioned this pull request Feb 14, 2023

rustc should fail CI on allow(incomplete_features) #108045

Open

Noratrieb reviewed Feb 14, 2023

View reviewed changes

Zoxc mentioned this pull request Feb 17, 2023

Make rustc_query_system take QueryConfig by instance. #108167

Merged

cjgillot 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 Feb 18, 2023

Zoxc closed this Feb 24, 2023

Zoxc mentioned this pull request Feb 27, 2023

Use dynamic dispatch for query execution #108526

Closed

Zoxc mentioned this pull request Mar 2, 2023

Use dynamic dispatch for queries #108638

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Erase query result types #107937

Erase query result types #107937

Zoxc commented Feb 11, 2023

rustbot commented Feb 11, 2023

cjgillot Feb 11, 2023

cjgillot Feb 11, 2023

Zoxc Feb 14, 2023

cjgillot Feb 12, 2023

Zoxc Feb 14, 2023

BoxyUwU Feb 12, 2023 •

edited

Loading

BoxyUwU Feb 12, 2023

bors commented Feb 12, 2023

Noratrieb Feb 14, 2023

bjorn3 Feb 14, 2023

Noratrieb Feb 14, 2023

oli-obk Feb 14, 2023

		@@ -0,0 +1,28 @@
		/// Remaps the type with a different lifetime for 'tcx if applicable.
		pub trait Remap {

		@@ -0,0 +1,47 @@
		// This is a separate crate so that we can `allow(incomplete_features)` for just `generic_const_exprs`
		#![feature(generic_const_exprs)]

Erase query result types #107937

Erase query result types #107937

Conversation

Zoxc commented Feb 11, 2023

rustbot commented Feb 11, 2023

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

BoxyUwU Feb 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Feb 12, 2023

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

BoxyUwU Feb 12, 2023 •

edited

Loading