Auto merge of #59722 - Zoxc:query-cache, r=eddyb

Clean up query cache code

There are a couple of queries for which we do not promote saved results, which have been removed from caching here. This was likely the cause of the regression in https://github.com/rust-lang/rust/pull/57293#issuecomment-476421298.

r? @michaelwoerister

src/librustc/dep_graph/graph.rs

@@ -842,31 +842,20 @@ impl DepGraph {
     // This method will only load queries that will end up in the disk cache.
     // Other queries will not be executed.
     pub fn exec_cache_promotions<'tcx>(&self, tcx: TyCtxt<'tcx>) {
-        let green_nodes: Vec<DepNode> = {
         let data = self.data.as_ref().unwrap();
-            data.colors.values.indices().filter_map(|prev_index| {
+        for prev_index in data.colors.values.indices() {
             match data.colors.get(prev_index) {
                 Some(DepNodeColor::Green(_)) => {
                     let dep_node = data.previous.index_to_node(prev_index);
-                        if dep_node.cache_on_disk(tcx) {
+                    dep_node.try_load_from_on_disk_cache(tcx);
-                            Some(dep_node)
-                        } else {
-                            None
-                        }
                 }
                 None |
                 Some(DepNodeColor::Red) => {
                     // We can skip red nodes because a node can only be marked
                     // as red if the query result was recomputed and thus is
                     // already in memory.
-                        None
                 }
             }
-            }).collect()
-        };
-
-        for dep_node in green_nodes {
-            dep_node.load_from_on_disk_cache(tcx);
         }
     }
 

src/librustc/mir/interpret/error.rs

@@ -18,7 +18,7 @@ use errors::DiagnosticBuilder;
 use syntax_pos::{Pos, Span};
 use syntax::symbol::Symbol;
 
-#[derive(Debug, Copy, Clone, PartialEq, Eq, HashStable)]
+#[derive(Debug, Copy, Clone, PartialEq, Eq, HashStable, RustcEncodable, RustcDecodable)]
 pub enum ErrorHandled {
     /// Already reported a lint or an error for this evaluation.
     Reported,

src/librustc/query/mod.rs

@@ -2,7 +2,7 @@ use crate::ty::query::QueryDescription;
 use crate::ty::query::queries;
 use crate::ty::{self, ParamEnvAnd, Ty, TyCtxt};
 use crate::ty::subst::SubstsRef;
-use crate::dep_graph::SerializedDepNodeIndex;
+use crate::dep_graph::{RecoverKey,DepKind, DepNode, SerializedDepNodeIndex};
 use crate::hir::def_id::{CrateNum, DefId, DefIndex};
 use crate::mir;
 use crate::mir::interpret::GlobalId;
@@ -33,13 +33,13 @@ rustc_queries! {
     Other {
         /// Records the type of every item.
         query type_of(key: DefId) -> Ty<'tcx> {
-            cache { key.is_local() }
+            cache_on_disk_if { key.is_local() }
         }
 
         /// Maps from the `DefId` of an item (trait/struct/enum/fn) to its
         /// associated generics.
         query generics_of(key: DefId) -> &'tcx ty::Generics {
-            cache { key.is_local() }
+            cache_on_disk_if { key.is_local() }
             load_cached(tcx, id) {
                 let generics: Option<ty::Generics> = tcx.queries.on_disk_cache
                                                         .try_load_query_result(tcx, id);
@@ -62,7 +62,9 @@ rustc_queries! {
         /// predicate gets in the way of some checks, which are intended
         /// to operate over only the actual where-clauses written by the
         /// user.)
-        query predicates_of(_: DefId) -> &'tcx ty::GenericPredicates<'tcx> {}
+        query predicates_of(key: DefId) -> &'tcx ty::GenericPredicates<'tcx> {
+            cache_on_disk_if { key.is_local() }
+        }
 
         query native_libraries(_: CrateNum) -> Lrc<Vec<NativeLibrary>> {
             desc { "looking up the native libraries of a linked crate" }
@@ -93,7 +95,7 @@ rustc_queries! {
         /// of the MIR qualify_consts pass. The actual meaning of
         /// the value isn't known except to the pass itself.
         query mir_const_qualif(key: DefId) -> (u8, &'tcx BitSet<mir::Local>) {
-            cache { key.is_local() }
+            cache_on_disk_if { key.is_local() }
         }
 
         /// Fetch the MIR for a given `DefId` right after it's built - this includes
@@ -115,7 +117,7 @@ rustc_queries! {
         /// MIR after our optimization passes have run. This is MIR that is ready
         /// for codegen. This is also the only query that can fetch non-local MIR, at present.
         query optimized_mir(key: DefId) -> &'tcx mir::Body<'tcx> {
-            cache { key.is_local() }
+            cache_on_disk_if { key.is_local() }
             load_cached(tcx, id) {
                 let mir: Option<crate::mir::Body<'tcx>> = tcx.queries.on_disk_cache
                                                             .try_load_query_result(tcx, id);
@@ -285,7 +287,9 @@ rustc_queries! {
 
     TypeChecking {
         /// The result of unsafety-checking this `DefId`.
-        query unsafety_check_result(_: DefId) -> mir::UnsafetyCheckResult {}
+        query unsafety_check_result(key: DefId) -> mir::UnsafetyCheckResult {
+            cache_on_disk_if { key.is_local() }
+        }
 
         /// HACK: when evaluated, this reports a "unsafe derive on repr(packed)" error
         query unsafe_derive_on_repr_packed(_: DefId) -> () {}
@@ -348,7 +352,7 @@ rustc_queries! {
         }
 
         query typeck_tables_of(key: DefId) -> &'tcx ty::TypeckTables<'tcx> {
-            cache { key.is_local() }
+            cache_on_disk_if { key.is_local() }
             load_cached(tcx, id) {
                 let typeck_tables: Option<ty::TypeckTables<'tcx>> = tcx
                     .queries.on_disk_cache
@@ -360,7 +364,9 @@ rustc_queries! {
     }
 
     Other {
-        query used_trait_imports(_: DefId) -> &'tcx DefIdSet {}
+        query used_trait_imports(key: DefId) -> &'tcx DefIdSet {
+            cache_on_disk_if { key.is_local() }
+        }
     }
 
     TypeChecking {
@@ -372,11 +378,15 @@ rustc_queries! {
     }
 
     BorrowChecking {
-        query borrowck(_: DefId) -> &'tcx BorrowCheckResult {}
+        query borrowck(key: DefId) -> &'tcx BorrowCheckResult {
+            cache_on_disk_if { key.is_local() }
+        }
 
         /// Borrow-checks the function body. If this is a closure, returns
         /// additional requirements that the closure's creator must verify.
-        query mir_borrowck(_: DefId) -> mir::BorrowCheckResult<'tcx> {}
+        query mir_borrowck(key: DefId) -> mir::BorrowCheckResult<'tcx> {
+            cache_on_disk_if(tcx, _) { key.is_local() && tcx.is_closure(key) }
+        }
     }
 
     TypeChecking {
@@ -412,9 +422,10 @@ rustc_queries! {
                 "const-evaluating `{}`",
                 tcx.def_path_str(key.value.instance.def.def_id())
             }
-            cache { true }
+            cache_on_disk_if(_, opt_result) {
-            load_cached(tcx, id) {
+                // Only store results without errors
-                tcx.queries.on_disk_cache.try_load_query_result(tcx, id).map(Ok)
+                // FIXME: We never store these
+                opt_result.map_or(true, |r| r.is_ok())
             }
         }
 
@@ -427,9 +438,9 @@ rustc_queries! {
                 "const-evaluating + checking `{}`",
                 tcx.def_path_str(key.value.instance.def.def_id())
             }
-            cache { true }
+            cache_on_disk_if(_, opt_result) {
-            load_cached(tcx, id) {
+                // Only store results without errors
-                tcx.queries.on_disk_cache.try_load_query_result(tcx, id).map(Ok)
+                opt_result.map_or(true, |r| r.is_ok())
             }
         }
 
@@ -453,7 +464,9 @@ rustc_queries! {
     }
 
     TypeChecking {
-        query check_match(_: DefId) -> () {}
+        query check_match(key: DefId) -> () {
+            cache_on_disk_if { key.is_local() }
+        }
 
         /// Performs part of the privacy check and computes "access levels".
         query privacy_access_levels(_: CrateNum) -> &'tcx AccessLevels {
@@ -483,7 +496,7 @@ rustc_queries! {
         query symbol_name(key: ty::Instance<'tcx>) -> ty::SymbolName {
             no_force
             desc { "computing the symbol for `{}`", key }
-            cache { true }
+            cache_on_disk_if { true }
         }
 
         query def_kind(_: DefId) -> Option<DefKind> {}
@@ -501,7 +514,9 @@ rustc_queries! {
     }
 
     Codegen {
-        query codegen_fn_attrs(_: DefId) -> CodegenFnAttrs {}
+        query codegen_fn_attrs(_: DefId) -> CodegenFnAttrs {
+            cache_on_disk_if { true }
+        }
     }
 
     Other {
@@ -519,7 +534,7 @@ rustc_queries! {
                 "const checking if rvalue is promotable to static `{}`",
                 tcx.def_path_str(key)
             }
-            cache { true }
+            cache_on_disk_if { true }
         }
         query rvalue_promotable_map(key: DefId) -> &'tcx ItemLocalSet {
             desc { |tcx|
@@ -548,7 +563,7 @@ rustc_queries! {
             key: (ty::ParamEnv<'tcx>, ty::PolyTraitRef<'tcx>)
         ) -> Vtable<'tcx, ()> {
             no_force
-            cache { true }
+            cache_on_disk_if { true }
             desc { |tcx|
                 "checking if `{}` fulfills its obligations",
                 tcx.def_path_str(key.1.def_id())
@@ -560,7 +575,9 @@ rustc_queries! {
         query trait_impls_of(key: DefId) -> &'tcx ty::trait_def::TraitImpls {
             desc { |tcx| "trait impls of `{}`", tcx.def_path_str(key) }
         }
-        query specialization_graph_of(_: DefId) -> &'tcx specialization_graph::Graph {}
+        query specialization_graph_of(_: DefId) -> &'tcx specialization_graph::Graph {
+            cache_on_disk_if { true }
+        }
         query is_object_safe(key: DefId) -> bool {
             desc { |tcx| "determine object safety of trait `{}`", tcx.def_path_str(key) }
         }

src/librustc/ty/query/config.rs

@@ -55,7 +55,7 @@ pub(crate) trait QueryDescription<'tcx>: QueryAccessors<'tcx> {
     fn describe(tcx: TyCtxt<'_>, key: Self::Key) -> Cow<'static, str>;
 
     #[inline]
-    fn cache_on_disk(_: TyCtxt<'tcx>, _: Self::Key) -> bool {
+    fn cache_on_disk(_: TyCtxt<'tcx>, _: Self::Key, _: Option<&Self::Value>) -> bool {
         false
     }
 
@@ -80,33 +80,3 @@ impl<'tcx> QueryDescription<'tcx> for queries::analysis<'tcx> {
         "running analysis passes on this crate".into()
     }
 }
-
-macro_rules! impl_disk_cacheable_query(
-    ($query_name:ident, |$tcx:tt, $key:tt| $cond:expr) => {
-        impl<'tcx> QueryDescription<'tcx> for queries::$query_name<'tcx> {
-            #[inline]
-            fn cache_on_disk($tcx: TyCtxt<'tcx>, $key: Self::Key) -> bool {
-                $cond
-            }
-
-            #[inline]
-            fn try_load_from_disk(tcx: TyCtxt<'tcx>,
-                                      id: SerializedDepNodeIndex)
-                                      -> Option<Self::Value> {
-                tcx.queries.on_disk_cache.try_load_query_result(tcx, id)
-            }
-        }
-    }
-);
-
-impl_disk_cacheable_query!(mir_borrowck, |tcx, def_id| {
-    def_id.is_local() && tcx.is_closure(def_id)
-});
-
-impl_disk_cacheable_query!(unsafety_check_result, |_, def_id| def_id.is_local());
-impl_disk_cacheable_query!(borrowck, |_, def_id| def_id.is_local());
-impl_disk_cacheable_query!(check_match, |_, def_id| def_id.is_local());
-impl_disk_cacheable_query!(predicates_of, |_, def_id| def_id.is_local());
-impl_disk_cacheable_query!(used_trait_imports, |_, def_id| def_id.is_local());
-impl_disk_cacheable_query!(codegen_fn_attrs, |_, _| true);
-impl_disk_cacheable_query!(specialization_graph_of, |_, _| true);

src/librustc/ty/query/on_disk_cache.rs

@@ -221,26 +221,8 @@ impl<'sess> OnDiskCache<'sess> {
                 encode_query_results::<check_match<'_>, _>(tcx, enc, qri)?;
                 encode_query_results::<codegen_fn_attrs<'_>, _>(tcx, enc, qri)?;
                 encode_query_results::<specialization_graph_of<'_>, _>(tcx, enc, qri)?;
-
+                encode_query_results::<const_eval<'_>, _>(tcx, enc, qri)?;
-                // const eval is special, it only encodes successfully evaluated constants
+                // FIXME: Include const_eval_raw?
-                use crate::ty::query::QueryAccessors;
-                let cache = const_eval::query_cache(tcx).borrow();
-                assert!(cache.active.is_empty());
-                for (key, entry) in cache.results.iter() {
-                    use crate::ty::query::config::QueryDescription;
-                    if const_eval::cache_on_disk(tcx, key.clone()) {
-                        if let Ok(ref value) = entry.value {
-                            let dep_node = SerializedDepNodeIndex::new(entry.index.index());
-
-                            // Record position of the cache entry
-                            qri.push((dep_node, AbsoluteBytePos::new(enc.position())));
-
-                            // Encode the type check tables with the SerializedDepNodeIndex
-                            // as tag.
-                            enc.encode_tagged(dep_node, value)?;
-                        }
-                    }
-                }
 
                 Ok(())
             })?;
@@ -1090,7 +1072,7 @@ where
         let map = Q::query_cache(tcx).borrow();
         assert!(map.active.is_empty());
         for (key, entry) in map.results.iter() {
-            if Q::cache_on_disk(tcx, key.clone()) {
+            if Q::cache_on_disk(tcx, key.clone(), Some(&entry.value)) {
                 let dep_node = SerializedDepNodeIndex::new(entry.index.index());
 
                 // Record position of the cache entry

src/librustc/ty/query/plumbing.rs

@@ -444,7 +444,7 @@ impl<'tcx> TyCtxt<'tcx> {
         debug_assert!(self.dep_graph.is_green(dep_node));
 
         // First we try to load the result from the on-disk cache
-        let result = if Q::cache_on_disk(self.global_tcx(), key.clone()) &&
+        let result = if Q::cache_on_disk(self.global_tcx(), key.clone(), None) &&
                         self.sess.opts.debugging_opts.incremental_queries {
             self.sess.profiler(|p| p.incremental_load_result_start(Q::NAME));
             let result = Q::try_load_from_disk(self.global_tcx(), prev_dep_node_index);
@@ -1243,66 +1243,3 @@ pub fn force_from_dep_node<'tcx>(tcx: TyCtxt<'tcx>, dep_node: &DepNode) -> bool
 
     true
 }
-
-
-// FIXME(#45015): Another piece of boilerplate code that could be generated in
-//                a combined define_dep_nodes!()/define_queries!() macro.
-macro_rules! impl_load_from_cache {
-    ($($dep_kind:ident => $query_name:ident,)*) => {
-        impl DepNode {
-            // Check whether the query invocation corresponding to the given
-            // DepNode is eligible for on-disk-caching.
-            pub fn cache_on_disk(&self, tcx: TyCtxt<'_>) -> bool {
-                use crate::ty::query::queries;
-                use crate::ty::query::QueryDescription;
-
-                match self.kind {
-                    $(DepKind::$dep_kind => {
-                        let def_id = self.extract_def_id(tcx).unwrap();
-                        queries::$query_name::cache_on_disk(tcx.global_tcx(), def_id)
-                    })*
-                    _ => false
-                }
-            }
-
-            // This is method will execute the query corresponding to the given
-            // DepNode. It is only expected to work for DepNodes where the
-            // above `cache_on_disk` methods returns true.
-            // Also, as a sanity check, it expects that the corresponding query
-            // invocation has been marked as green already.
-            pub fn load_from_on_disk_cache(&self, tcx: TyCtxt<'_>) {
-                match self.kind {
-                    $(DepKind::$dep_kind => {
-                        debug_assert!(tcx.dep_graph
-                                         .node_color(self)
-                                         .map(|c| c.is_green())
-                                         .unwrap_or(false));
-
-                        let def_id = self.extract_def_id(tcx).unwrap();
-                        let _ = tcx.$query_name(def_id);
-                    })*
-                    _ => {
-                        bug!()
-                    }
-                }
-            }
-        }
-    }
-}
-
-impl_load_from_cache!(
-    typeck_tables_of => typeck_tables_of,
-    optimized_mir => optimized_mir,
-    unsafety_check_result => unsafety_check_result,
-    borrowck => borrowck,
-    mir_borrowck => mir_borrowck,
-    mir_const_qualif => mir_const_qualif,
-    const_is_rvalue_promotable_to_static => const_is_rvalue_promotable_to_static,
-    check_match => check_match,
-    type_of => type_of,
-    generics_of => generics_of,
-    predicates_of => predicates_of,
-    used_trait_imports => used_trait_imports,
-    codegen_fn_attrs => codegen_fn_attrs,
-    specialization_graph_of => specialization_graph_of,
-);

src/librustc_macros/src/lib.rs

@@ -1,6 +1,8 @@
 #![feature(proc_macro_hygiene)]
 #![deny(rust_2018_idioms)]
 
+#![recursion_limit="128"]
+
 extern crate proc_macro;
 
 use synstructure::decl_derive;

src/librustc_macros/src/query.rs

@@ -1,4 +1,5 @@
 use proc_macro::TokenStream;
+use proc_macro2::{TokenTree, Delimiter};
 use syn::{
     Token, Ident, Type, Attribute, ReturnType, Expr, Block, Error,
     braced, parenthesized, parse_macro_input,
@@ -35,7 +36,7 @@ enum QueryModifier {
     Desc(Option<Ident>, Punctuated<Expr, Token![,]>),
 
     /// Cache the query to disk if the `Expr` returns true.
-    Cache(Option<Ident>, Expr),
+    Cache(Option<(IdentOrWild, IdentOrWild)>, Block),
 
     /// Custom code to load the query from disk.
     LoadCached(Ident, Ident, Block),
@@ -77,21 +78,26 @@ impl Parse for QueryModifier {
             };
             let desc = attr_content.parse_terminated(Expr::parse)?;
             Ok(QueryModifier::Desc(tcx, desc))
-        } else if modifier == "cache" {
+        } else if modifier == "cache_on_disk_if" {
             // Parse a cache modifier like:
-            // `cache { |tcx| key.is_local() }`
+            // `cache(tcx, value) { |tcx| key.is_local() }`
-            let attr_content;
+            let has_args = if let TokenTree::Group(group) = input.fork().parse()? {
-            braced!(attr_content in input);
+                group.delimiter() == Delimiter::Parenthesis
-            let tcx = if attr_content.peek(Token![|]) {
+            } else {
-                attr_content.parse::<Token![|]>()?;
+                false
-                let tcx = attr_content.parse()?;
+            };
-                attr_content.parse::<Token![|]>()?;
+            let args = if has_args {
-                Some(tcx)
+                let args;
+                parenthesized!(args in input);
+                let tcx = args.parse()?;
+                args.parse::<Token![,]>()?;
+                let value = args.parse()?;
+                Some((tcx, value))
             } else {
                 None
             };
-            let expr = attr_content.parse()?;
+            let block = input.parse()?;
-            Ok(QueryModifier::Cache(tcx, expr))
+            Ok(QueryModifier::Cache(args, block))
         } else if modifier == "load_cached" {
             // Parse a load_cached modifier like:
             // `load_cached(tcx, id) { tcx.queries.on_disk_cache.try_load_query_result(tcx, id) }`
@@ -203,8 +209,8 @@ struct QueryModifiers {
     /// The description of the query.
     desc: Option<(Option<Ident>, Punctuated<Expr, Token![,]>)>,
 
-    /// Cache the query to disk if the `Expr` returns true.
+    /// Cache the query to disk if the `Block` returns true.
-    cache: Option<(Option<Ident>, Expr)>,
+    cache: Option<(Option<(IdentOrWild, IdentOrWild)>, Block)>,
 
     /// Custom code to load the query from disk.
     load_cached: Option<(Ident, Ident, Block)>,
@@ -247,11 +253,11 @@ fn process_modifiers(query: &mut Query) -> QueryModifiers {
                 }
                 load_cached = Some((tcx, id, block));
             }
-            QueryModifier::Cache(tcx, expr) => {
+            QueryModifier::Cache(args, expr) => {
                 if cache.is_some() {
                     panic!("duplicate modifier `cache` for query `{}`", query.name);
                 }
-                cache = Some((tcx, expr));
+                cache = Some((args, expr));
             }
             QueryModifier::Desc(tcx, list) => {
                 if desc.is_some() {
@@ -321,7 +327,7 @@ fn add_query_description_impl(
     let key = &query.key.0;
 
     // Find out if we should cache the query on disk
-    let cache = modifiers.cache.as_ref().map(|(tcx, expr)| {
+    let cache = modifiers.cache.as_ref().map(|(args, expr)| {
         let try_load_from_disk = if let Some((tcx, id, block)) = modifiers.load_cached.as_ref() {
             // Use custom code to load the query from disk
             quote! {
@@ -346,11 +352,22 @@ fn add_query_description_impl(
             }
         };
 
-        let tcx = tcx.as_ref().map(|t| quote! { #t }).unwrap_or(quote! { _ });
+        let tcx = args.as_ref().map(|t| {
+            let t = &(t.0).0;
+            quote! { #t }
+        }).unwrap_or(quote! { _ });
+        let value = args.as_ref().map(|t| {
+            let t = &(t.1).0;
+            quote! { #t }
+        }).unwrap_or(quote! { _ });
         quote! {
             #[inline]
             #[allow(unused_variables)]
-            fn cache_on_disk(#tcx: TyCtxt<'tcx>, #key: Self::Key) -> bool {
+            fn cache_on_disk(
+                #tcx: TyCtxt<'tcx>,
+                #key: Self::Key,
+                #value: Option<&Self::Value>
+            ) -> bool {
                 #expr
             }
 
@@ -395,6 +412,7 @@ pub fn rustc_queries(input: TokenStream) -> TokenStream {
     let mut query_description_stream = quote! {};
     let mut dep_node_def_stream = quote! {};
     let mut dep_node_force_stream = quote! {};
+    let mut try_load_from_on_disk_cache_stream = quote! {};
     let mut no_force_queries = Vec::new();
 
     for group in groups.0 {
@@ -409,6 +427,22 @@ pub fn rustc_queries(input: TokenStream) -> TokenStream {
                 _ => quote! { #result_full },
             };
 
+            if modifiers.cache.is_some() && !modifiers.no_force {
+                try_load_from_on_disk_cache_stream.extend(quote! {
+                    DepKind::#name => {
+                        debug_assert!(tcx.dep_graph
+                                         .node_color(self)
+                                         .map(|c| c.is_green())
+                                         .unwrap_or(false));
+
+                        let key = RecoverKey::recover(tcx.global_tcx(), self).unwrap();
+                        if queries::#name::cache_on_disk(tcx.global_tcx(), key, None) {
+                            let _ = tcx.#name(key);
+                        }
+                    }
+                });
+            }
+
             let mut attributes = Vec::new();
 
             // Pass on the fatal_cycle modifier
@@ -462,7 +496,11 @@ pub fn rustc_queries(input: TokenStream) -> TokenStream {
                 });
             }
 
-            add_query_description_impl(&query, modifiers, &mut query_description_stream);
+            add_query_description_impl(
+                &query,
+                modifiers,
+                &mut query_description_stream,
+            );
         }
         let name = &group.name;
         query_stream.extend(quote! {
@@ -512,5 +550,19 @@ pub fn rustc_queries(input: TokenStream) -> TokenStream {
             }
         }
         #query_description_stream
+
+        impl DepNode {
+            /// Check whether the query invocation corresponding to the given
+            /// DepNode is eligible for on-disk-caching. If so, this is method
+            /// will execute the query corresponding to the given DepNode.
+            /// Also, as a sanity check, it expects that the corresponding query
+            /// invocation has been marked as green already.
+            pub fn try_load_from_on_disk_cache(&self, tcx: TyCtxt<'_>) {
+                match self.kind {
+                    #try_load_from_on_disk_cache_stream
+                    _ => (),
+                }
+            }
+        }
     })
 }