Auto merge of #158059 - xmakro:perf/lexer-ascii-symbolgallery, r=cjgillot

Skip query machinery when promoting disk-cached values



At the end of an incremental session, exec_cache_promotions walks the green nodes and, for queries that were never used this session, loads their value from the previous session's disk cache into memory so it gets written into the new cache. This load currently goes through the full query execution path and shows up in profiles of incremental builds with few or no changes, which is the motivation for this PR.

Instead, this PR only loads the value directly and puts it in the in-memory cache. Perf improvement: -1.3% mean instructions, mostly in incr-unchanged, -0.36% in incr-patched, no regressions, see #158059 (comment).

Behavior change: A missing disk value is now a bug instead of a silent recompute.

Author: bors

compiler/rustc_middle/src/dep_graph/dep_node.rs

@@ -57,7 +57,7 @@ use rustc_hir::definitions::DefPathHash;
 use rustc_macros::{Decodable, Encodable, StableHash};
 use rustc_span::Symbol;
 
-use super::{KeyFingerprintStyle, SerializedDepNodeIndex};
+use super::{DepNodeIndex, KeyFingerprintStyle, SerializedDepNodeIndex};
 use crate::dep_graph::DepNodeKey;
 use crate::mono::MonoItem;
 use crate::ty::{TyCtxt, tls};
@@ -208,8 +208,17 @@ pub struct DepKindVTable<'tcx> {
         fn(tcx: TyCtxt<'tcx>, dep_node: DepNode, prev_index: SerializedDepNodeIndex) -> bool,
     >,
 
-    /// Invoke a query to put the on-disk cached value in memory.
-    pub promote_from_disk_fn: Option<fn(TyCtxt<'tcx>, DepNode)>,
+    /// Load the on-disk cached value of a query into memory. The node is known
+    /// to be green, with `prev_index` its index in the previous session's dep
+    /// graph and `dep_node_index` its index in the current session's dep graph.
+    pub promote_from_disk_fn: Option<
+        fn(
+            tcx: TyCtxt<'tcx>,
+            dep_node: DepNode,
+            prev_index: SerializedDepNodeIndex,
+            dep_node_index: DepNodeIndex,
+        ),
+    >,
 }
 
 /// A "work product" corresponds to a `.o` (or other) file that we

compiler/rustc_middle/src/dep_graph/graph.rs

@@ -1064,12 +1064,12 @@ impl DepGraph {
         let data = self.data.as_ref().unwrap();
         for prev_index in data.colors.values.indices() {
             match data.colors.get(prev_index) {
-                DepNodeColor::Green(_) => {
+                DepNodeColor::Green(dep_node_index) => {
                     let dep_node = data.previous.index_to_node(prev_index);
                     if let Some(promote_fn) =
                         tcx.dep_kind_vtable(dep_node.kind).promote_from_disk_fn
                     {
-                        promote_fn(tcx, *dep_node)
+                        promote_fn(tcx, *dep_node, prev_index, dep_node_index)
                     };
                 }
                 DepNodeColor::Unknown | DepNodeColor::Red => {

compiler/rustc_query_impl/src/dep_kind_vtables.rs

@@ -117,10 +117,12 @@ where
                 crate::execution::force_query_dep_node(tcx, query, dep_node)
             },
         ),
-        promote_from_disk_fn: (can_recover && is_cache_on_disk).then_some(|tcx, dep_node| {
-            let query = Q::query_vtable(tcx);
-            promote_from_disk_inner(tcx, query, dep_node)
-        }),
+        promote_from_disk_fn: (can_recover && is_cache_on_disk).then_some(
+            |tcx, dep_node, prev_index, dep_node_index| {
+                let query = Q::query_vtable(tcx);
+                promote_from_disk_inner(tcx, query, dep_node, prev_index, dep_node_index)
+            },
+        ),
     }
 }
 

compiler/rustc_query_impl/src/execution.rs

@@ -1,6 +1,7 @@
 use std::hash::Hash;
 use std::mem::ManuallyDrop;
 
+use rustc_data_structures::fingerprint::Fingerprint;
 use rustc_data_structures::hash_table::{Entry, HashTable};
 use rustc_data_structures::stack::ensure_sufficient_stack;
 use rustc_data_structures::sync::{DynSend, DynSync};
@@ -484,6 +485,20 @@ fn execute_job_incr<'tcx, C: QueryCache>(
     (result, dep_node_index)
 }
 
+/// Whether a value loaded from the on-disk cache should have its fingerprint
+/// verified with `incremental_verify_ich`. If `-Zincremental-verify-ich` is
+/// specified, re-hash results from the cache and make sure that they have the
+/// expected fingerprint.
+///
+/// If not, we still seek to verify a subset of fingerprints loaded from disk.
+/// Re-hashing results is fairly expensive, so we can't currently afford to
+/// verify every hash. This subset should still give us some coverage of
+/// potential bugs.
+pub(crate) fn should_verify_loaded_value(tcx: TyCtxt<'_>, prev_fingerprint: Fingerprint) -> bool {
+    prev_fingerprint.split().1.as_u64().is_multiple_of(32)
+        || tcx.sess.opts.unstable_opts.incremental_verify_ich
+}
+
 /// Given that the dep node for this query+key is green, obtain a value for it
 /// by loading one from disk if possible, or by invoking its query provider if
 /// necessary.
@@ -518,15 +533,7 @@ fn load_from_disk_or_invoke_provider_green<'tcx, C: QueryCache>(
             }
 
             let prev_fingerprint = dep_graph_data.prev_value_fingerprint_of(prev_index);
-            // If `-Zincremental-verify-ich` is specified, re-hash results from
-            // the cache and make sure that they have the expected fingerprint.
-            //
-            // If not, we still seek to verify a subset of fingerprints loaded
-            // from disk. Re-hashing results is fairly expensive, so we can't
-            // currently afford to verify every hash. This subset should still
-            // give us some coverage of potential bugs.
-            let verify = prev_fingerprint.split().1.as_u64().is_multiple_of(32)
-                || tcx.sess.opts.unstable_opts.incremental_verify_ich;
+            let verify = should_verify_loaded_value(tcx, prev_fingerprint);
 
             (value, verify)
         }

compiler/rustc_query_impl/src/plumbing.rs

@@ -1,22 +1,23 @@
 use std::num::NonZero;
 
 use rustc_data_structures::Limit;
+use rustc_data_structures::stack::ensure_sufficient_stack;
 use rustc_data_structures::unord::UnordMap;
 use rustc_middle::bug;
 #[expect(unused_imports, reason = "used by doc comments")]
 use rustc_middle::dep_graph::DepKindVTable;
-use rustc_middle::dep_graph::{DepNode, DepNodeKey, SerializedDepNodeIndex};
+use rustc_middle::dep_graph::{DepNode, DepNodeIndex, DepNodeKey, SerializedDepNodeIndex};
 use rustc_middle::query::erase::{Erasable, Erased};
 use rustc_middle::query::on_disk_cache::{CacheDecoder, CacheEncoder};
-use rustc_middle::query::{QueryCache, QueryJobId, QueryMode, QueryVTable, erase};
+use rustc_middle::query::{QueryCache, QueryJobId, QueryVTable, erase};
 use rustc_middle::ty::TyCtxt;
 use rustc_middle::ty::tls::{self, ImplicitCtxt};
+use rustc_middle::verify_ich::incremental_verify_ich;
 use rustc_serialize::{Decodable, Encodable};
-use rustc_span::DUMMY_SP;
 use rustc_span::def_id::LOCAL_CRATE;
 
 use crate::error::{QueryOverflow, QueryOverflowNote};
-use crate::execution::all_inactive;
+use crate::execution::{all_inactive, should_verify_loaded_value};
 use crate::job::find_dep_kind_root;
 use crate::query_impl::for_each_query_vtable;
 use crate::{CollectActiveJobsKind, collect_active_query_jobs};
@@ -140,6 +141,8 @@ pub(crate) fn promote_from_disk_inner<'tcx, C: QueryCache>(
     tcx: TyCtxt<'tcx>,
     query: &'tcx QueryVTable<'tcx, C>,
     dep_node: DepNode,
+    prev_index: SerializedDepNodeIndex,
+    dep_node_index: DepNodeIndex,
 ) {
     debug_assert!(tcx.dep_graph.is_green(&dep_node));
 
@@ -156,21 +159,39 @@ pub(crate) fn promote_from_disk_inner<'tcx, C: QueryCache>(
         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);
-        }
+    // If the value is already in memory, then promotion isn't needed.
+    if query.cache.lookup(&key).is_some() {
+        return;
+    }
+
+    // Load the disk-cached value into memory.
+    let dep_graph_data =
+        tcx.dep_graph.data().expect("should always be present in incremental mode");
+
+    let prof_timer = tcx.prof.incr_cache_loading();
+    let value = ensure_sufficient_stack(|| (query.try_load_from_disk_fn)(tcx, prev_index));
+    prof_timer.finish_with_query_invocation_id(dep_node_index.into());
+
+    let Some(value) = value else {
+        // The key is cache-on-disk and its node is green, so a value must be on disk.
+        bug!("failed to load disk-cached value for green node {dep_node:?}");
+    };
+
+    // Verify the fingerprints of the same subset of loaded values as
+    // `load_from_disk_or_invoke_provider_green` does.
+    let prev_fingerprint = dep_graph_data.prev_value_fingerprint_of(prev_index);
+    if should_verify_loaded_value(tcx, prev_fingerprint) {
+        incremental_verify_ich(
+            tcx,
+            dep_graph_data,
+            &value,
+            prev_index,
+            query.hash_value_fn,
+            query.format_value,
+        );
     }
+
+    query.cache.complete(key, value, dep_node_index);
 }
 
 pub(crate) fn try_load_from_disk<'tcx, V>(