Auto merge of #153387 - Zalathar:call-query, r=nnethercote

Get rid of `QueryVTable::call_query_method_fn`



Calling the query method to promote a value is equivalent to doing a cache lookup and then calling `execute_query_fn`, so we can just do that manually instead.

There are two “functional” differences here: If a cache hit occurs, we don't record the hit for self-profiling, and we don't register a read of the dep node. In the context of promotion, which touches *all* eligible cache entries just before writing the memory-cached values to disk, those two steps should be unnecessary overhead anyway.

r? nnethercote (or compiler)

Author: bors

compiler/rustc_middle/src/query/plumbing.rs

@@ -114,13 +114,6 @@ pub struct QueryVTable<'tcx, C: QueryCache> {
     pub state: QueryState<'tcx, C::Key>,
     pub cache: C,
 
-    /// Function pointer that calls `tcx.$query(key)` for this query and
-    /// discards the returned value.
-    ///
-    /// This is a weird thing to be doing, and probably not what you want.
-    /// It is used for loading query results from disk-cache in some cases.
-    pub call_query_method_fn: fn(tcx: TyCtxt<'tcx>, key: C::Key),
-
     /// Function pointer that actually calls this query's provider.
     /// Also performs some associated secondary tasks; see the macro-defined
     /// implementation in `mod invoke_provider_fn` for more details.

compiler/rustc_query_impl/src/plumbing.rs

@@ -19,14 +19,15 @@ use rustc_middle::query::on_disk_cache::{
 };
 use rustc_middle::query::plumbing::QueryVTable;
 use rustc_middle::query::{
-    QueryCache, QueryJobId, QueryKey, QueryStackDeferred, QueryStackFrame, QueryStackFrameExtra,
-    erase,
+    QueryCache, QueryJobId, QueryKey, QueryMode, QueryStackDeferred, QueryStackFrame,
+    QueryStackFrameExtra, erase,
 };
 use rustc_middle::ty::codec::TyEncoder;
 use rustc_middle::ty::print::with_reduced_queries;
 use rustc_middle::ty::tls::{self, ImplicitCtxt};
 use rustc_middle::ty::{self, TyCtxt};
 use rustc_serialize::{Decodable, Encodable};
+use rustc_span::DUMMY_SP;
 use rustc_span::def_id::LOCAL_CRATE;
 
 use crate::error::{QueryOverflow, QueryOverflowNote};
@@ -215,10 +216,27 @@ pub(crate) fn promote_from_disk_inner<'tcx, Q: GetQueryVTable<'tcx>>(
             dep_node.key_fingerprint
         )
     });
-    if (query.will_cache_on_disk_for_key_fn)(tcx, &key) {
-        // Call `tcx.$query(key)` for its side-effect of loading the disk-cached
-        // value into memory.
-        (query.call_query_method_fn)(tcx, key);
+
+    // If the recovered key isn't eligible for cache-on-disk, then there's no
+    // value on disk to promote.
+    if !(query.will_cache_on_disk_for_key_fn)(tcx, &key) {
+        return;
+    }
+
+    match query.cache.lookup(&key) {
+        // If the value is already in memory, then promotion isn't needed.
+        Some(_) => {}
+
+        // "Execute" the query to load its disk-cached value into memory.
+        //
+        // We know that the key is cache-on-disk and its node is green,
+        // so there _must_ be a value on disk to load.
+        //
+        // FIXME(Zalathar): Is there a reasonable way to skip more of the
+        // query bookkeeping when doing this?
+        None => {
+            (query.execute_query_fn)(tcx, DUMMY_SP, key, QueryMode::Get);
+        }
     }
 }
 
@@ -421,11 +439,6 @@ macro_rules! define_queries {
                     state: Default::default(),
                     cache: Default::default(),
 
-                    call_query_method_fn: |tcx, key| {
-                        // Call the query method for its side-effect of loading a value
-                        // from disk-cache; the caller doesn't need the value.
-                        let _ = tcx.$name(key);
-                    },
                     invoke_provider_fn: self::invoke_provider_fn::__rust_begin_short_backtrace,
 
                     #[cfg($cache_on_disk)]