Replace `DepKind` by trait objects #78314

cjgillot · 2020-10-24T01:48:31Z

Most of the code depending on DepNodes just matches on their DepKind.
This PR replaces those matches by trait objects, using a unit struct for each DepKind variant.

Everything still has to be declared inside rustc_middle, but the need for the macros is significantly reduced.
I would have preferred defining the trait inside rustc_query_system, but this needs more work.

cc @Julian-Wollersberger

Based on #77830 for the first few commits.

rust-highfive · 2020-10-24T01:48:35Z

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

Julian-Wollersberger

Thanks for cc-ing me. I left some nitpicks.

I can't comment on the larger picture though, because it's still to complicated for me after two weeks working in the query system. (It seems like I have chosen the worst part a beginner can work on. But at least it's a challenge!)

So, I'm curious what you envision the next steps to be here.
It seems orthogonal to what I tried in in #78275.

Julian-Wollersberger · 2020-10-24T12:24:46Z

compiler/rustc_middle/src/dep_graph/dep_node.rs

+
+            $(
+                #[allow(non_camel_case_types)]
+                pub struct $variant;


It would be good to rename $variant to $name here to be consistent with the proc macro and compiler/rustc_middle/src/ty/query/mod.rs
Specifically the inconsistency with the line $(impl dep_kind::$name { bothers me.

Julian-Wollersberger · 2020-10-24T13:01:03Z

compiler/rustc_middle/src/dep_graph/dep_node.rs

+        pub mod dep_kind {
+            $(
+                #[allow(non_camel_case_types)]
+                #[derive(Debug)]
+                pub struct $variant;
+            )*


I'd move this struct to define_dep_kinds.

Julian-Wollersberger · 2020-10-24T13:10:36Z

compiler/rustc_middle/src/dep_graph/mod.rs

@@ -24,14 +25,14 @@ pub type PreviousDepGraph = rustc_query_system::dep_graph::PreviousDepGraph<DepK
 pub type SerializedDepGraph = rustc_query_system::dep_graph::SerializedDepGraph<DepKind>;

 impl rustc_query_system::dep_graph::DepKind for DepKind {


All these different types that are named DepKind get confusing.
Maybe name it DynDepKind?

Julian-Wollersberger · 2020-10-24T13:12:45Z

compiler/rustc_middle/src/dep_graph/dep_node.rs

@@ -103,95 +120,136 @@ macro_rules! contains_eval_always_attr {
    ($($attr:ident $(($($attr_args:tt)*))* ),*) => ({$(is_eval_always_attr!($attr) | )* false});
 }

-macro_rules! define_dep_nodes {
+macro_rules! define_dep_kinds {


I'd move this into a new file dep_kind.rs.

Julian-Wollersberger · 2020-10-24T13:30:47Z

compiler/rustc_middle/src/ty/query/on_disk_cache.rs

@@ -765,7 +753,7 @@ impl<'a, 'tcx> Decodable<CacheDecoder<'a, 'tcx>> for &'tcx [Span] {
 //- ENCODING -------------------------------------------------------------------

 /// This trait is a hack to work around specialization bug #55243.
-trait OpaqueEncoder: Encoder {
+pub trait OpaqueEncoder: Encoder {


Why move the trait to make it private and then make it public again?

This trait is a hack. Ideally, all E: OpaqueEncoder should be E = opaque::Encoder. It needs to be public because it appears in CacheEncoder, which is public because it appears in DepKindTrait. Removing E: OpaqueEncoder from CacheEncoder definition breaks specialization (I don't know why).

Julian-Wollersberger · 2020-10-24T13:41:50Z

compiler/rustc_middle/src/dep_graph/dep_node.rs

+                // FIXME: This match is just a workaround for incremental bugs and should
+                // be removed. https://github.com/rust-lang/rust/issues/62649 is one such
+                // bug that must be fixed before removing this.
+                match self.index() {


All this workaround code is now duplicated for every dep_kind::$variant. That's not good for compile times.
What's the reason for doing this?

I agree this was a bad idea.

Julian-Wollersberger · 2020-10-24T13:54:15Z

compiler/rustc_macros/src/query.rs

@@ -481,51 +459,28 @@ pub fn rustc_queries(input: TokenStream) -> TokenStream {
            if modifiers.eval_always {
                attributes.push(quote! { eval_always });
            };
+            // Pass on the cache modifier
+            if modifiers.cache.is_some() {
+                attributes.push(quote! { cached });


Where is this attribute used? I can't find it.

EDIT: Fount it in encode_query_results. Can you add a comment documenting this dependency?

Julian-Wollersberger · 2020-10-24T14:24:02Z

compiler/rustc_middle/src/dep_graph/dep_node.rs

+            $($variant),*
+        }
+
+        pub static DEP_KINDS: &[DepKind] = &[ $(&dep_kind::$variant),* ];


This is an interesting trick! ^^

Julian-Wollersberger · 2020-10-24T14:27:36Z

compiler/rustc_middle/src/dep_graph/dep_node.rs

@@ -304,6 +526,271 @@ rustc_dep_node_append!([define_dep_nodes!][ <'tcx>
    [] CompileCodegenUnit(Symbol),
 ]);

+impl DepKindTrait for dep_kind::Null {


Can't these traits be implemented by the macro, by adding them to the
rustc_dep_node_append!([define_dep_kinds!][ <'tcx> ]); invokation,
similarly to how it's done for define_dep_nodes?

Those DepKinds are not associated to queries. This requires a special workaround for force_from_dep_node and try_load_from_on_disk_cache.

jonas-schievink · 2020-10-24T18:30:24Z

@bors try @rust-timer queue

rust-timer · 2020-10-24T18:30:25Z

Awaiting bors try build completion

bors · 2020-10-24T18:30:37Z

⌛ Trying commit 41720af with merge ca3c21d7ea44af5f9bbe58698ce3fd2e5cd675bf...

bors · 2020-10-24T19:16:40Z

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

rust-timer · 2020-10-24T19:16:41Z

Queued ca3c21d7ea44af5f9bbe58698ce3fd2e5cd675bf with parent 89fdb30, future comparison URL.

rust-timer · 2020-10-25T00:16:43Z

Finished benchmarking try commit (ca3c21d7ea44af5f9bbe58698ce3fd2e5cd675bf): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

Access query (DepKind) metadata through fields This refactors the access to query definition metadata (attributes such as eval always, anon, has_params) and loading/forcing functions to generate a number of structs, instead of matching on the DepKind enum. This makes access to the fields cheaper to compile. Using a struct means that finding the metadata for a given query is just an offset away; previously the match may have been compiled to a jump table but likely not completely inlined as we expect here. A previous attempt explored a similar strategy, but using trait objects in rust-lang#78314 that proved less effective, likely due to higher overheads due to forcing dynamic calls and poorer cache utilization (all metadata is fairly densely packed with this PR).

cjgillot added 15 commits October 22, 2020 22:35

Remove unused ProfileCategory.

0a4d948

Remove unused category from macros.

de76370

Unify query name and node name.

de7da7f

Retire rustc_dep_node_force.

e853cc0

Retire rustc_dep_node_try_load_from_on_disk_cache.

57ba8ed

Implement DepKind through unit structs.

276d0e3

Same for query loading.

dd9ce3e

Make DepKind into a trait object.

9249a52

Type-erase query stats.

f74b0b9

Monomorphize query serialisation.

f8c6f42

Make OpaqueEncoder private.

5e3cea6

Reduce visibilities.

253da5d

Type-erase query caching.

6d818bd

Remove rustc_cached_queries.

c0ccc7d

Remove one-liners.

908d735

rust-highfive assigned estebank Oct 24, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 24, 2020

Julian-Wollersberger reviewed Oct 24, 2020

View reviewed changes

cjgillot added 3 commits October 24, 2020 17:17

Rename macro parameter.

f06e3d9

Move DepNodeExt outside of the macro.

42baef9

Sanitize DEP_KINDS arrays.

41720af

cjgillot force-pushed the ddk branch from 5d4dfa1 to 41720af Compare October 24, 2020 15:49

jyn514 added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 24, 2020

cjgillot mentioned this pull request Oct 27, 2020

Access query (DepKind) metadata through fields #78452

Merged

cjgillot closed this Nov 1, 2020

cjgillot deleted the ddk branch September 21, 2021 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `DepKind` by trait objects #78314

Replace `DepKind` by trait objects #78314

cjgillot commented Oct 24, 2020

rust-highfive commented Oct 24, 2020

Julian-Wollersberger left a comment

Julian-Wollersberger Oct 24, 2020

Julian-Wollersberger Oct 24, 2020

Julian-Wollersberger Oct 24, 2020

Julian-Wollersberger Oct 24, 2020

Julian-Wollersberger Oct 24, 2020 •

edited

Loading

cjgillot Oct 24, 2020

Julian-Wollersberger Oct 24, 2020

cjgillot Oct 24, 2020

Julian-Wollersberger Oct 24, 2020

Julian-Wollersberger Oct 24, 2020

Julian-Wollersberger Oct 24, 2020

cjgillot Oct 24, 2020

jonas-schievink commented Oct 24, 2020

rust-timer commented Oct 24, 2020

bors commented Oct 24, 2020

bors commented Oct 24, 2020

rust-timer commented Oct 24, 2020

rust-timer commented Oct 25, 2020

		@@ -24,14 +25,14 @@ pub type PreviousDepGraph = rustc_query_system::dep_graph::PreviousDepGraph<DepK
		pub type SerializedDepGraph = rustc_query_system::dep_graph::SerializedDepGraph<DepKind>;

		impl rustc_query_system::dep_graph::DepKind for DepKind {

Replace DepKind by trait objects #78314

Replace DepKind by trait objects #78314

Conversation

cjgillot commented Oct 24, 2020

rust-highfive commented Oct 24, 2020

Julian-Wollersberger 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

Julian-Wollersberger Oct 24, 2020 • edited Loading

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

jonas-schievink commented Oct 24, 2020

rust-timer commented Oct 24, 2020

bors commented Oct 24, 2020

bors commented Oct 24, 2020

rust-timer commented Oct 24, 2020

rust-timer commented Oct 25, 2020

Replace `DepKind` by trait objects #78314

Replace `DepKind` by trait objects #78314

Julian-Wollersberger Oct 24, 2020 •

edited

Loading