Streamline active job collection.

`collect_active_jobs_from_all_queries` takes a `require_complete` bool,
and then some callers `expect` a full map result while others allow a
partial map result. The end result is four possible combinations, but
only three of them are used/make sense.

This commit introduces `CollectActiveJobsKind`, a three-value enum that
describes the three sensible combinations, and rewrites
`collect_active_jobs_from_all_queries` around it. This makes it and its
call sites much clearer, and removes the weird `Option<()>` and
`Result<QueryJobMap, QueryJobMap>` return types.

Other changes of note.
- `active` is removed. The comment about `make_frame` is out of date,
  and `create_deferred_query_stack_frame` *is* safe to call with the
  query state locked.
- When shard locking failure is allowed, collection no longer stops on
  the first failed shard.

Author: nnethercote

compiler/rustc_interface/src/util.rs

@@ -18,7 +18,7 @@ use rustc_data_structures::sync;
 use rustc_metadata::{DylibError, EncodedMetadata, load_symbol_from_dylib};
 use rustc_middle::dep_graph::{WorkProduct, WorkProductId};
 use rustc_middle::ty::{CurrentGcx, TyCtxt};
-use rustc_query_impl::collect_active_jobs_from_all_queries;
+use rustc_query_impl::{CollectActiveJobsKind, collect_active_jobs_from_all_queries};
 use rustc_session::config::{
     Cfg, CrateType, OutFileName, OutputFilenames, OutputTypes, Sysroot, host_tuple,
 };
@@ -253,9 +253,11 @@ internal compiler error: query cycle handler thread panicked, aborting process";
                                     unsafe { &*(session_globals as *const SessionGlobals) },
                                     || {
                                         // Ensure there were no errors collecting all active jobs.
-                                        // We need the complete map to ensure we find a cycle to break.
-                                        collect_active_jobs_from_all_queries(tcx, false).expect(
-                                            "failed to collect active queries in deadlock handler",
+                                        // We need the complete map to ensure we find a cycle to
+                                        // break.
+                                        collect_active_jobs_from_all_queries(
+                                            tcx,
+                                            CollectActiveJobsKind::FullNoContention,
                                         )
                                     },
                                 );

compiler/rustc_query_impl/src/execution.rs

@@ -15,6 +15,7 @@ use rustc_middle::query::{
 use rustc_middle::ty::TyCtxt;
 use rustc_middle::verify_ich::incremental_verify_ich;
 use rustc_span::{DUMMY_SP, Span};
+use tracing::warn;
 
 use crate::collect_active_jobs_from_all_queries;
 use crate::dep_graph::{DepNode, DepNodeIndex};
@@ -30,67 +31,72 @@ pub(crate) fn all_inactive<'tcx, K>(state: &QueryState<'tcx, K>) -> bool {
     state.active.lock_shards().all(|shard| shard.is_empty())
 }
 
+#[derive(Clone, Copy)]
+pub enum CollectActiveJobsKind {
+    /// We need the full query job map, and we are willing to wait to obtain the query state
+    /// shard lock(s).
+    Full,
+
+    /// We need the full query job map, and we shouldn't need to wait to obtain the shard lock(s),
+    /// because we are in a place where nothing else could hole the shard lock(s).
+    FullNoContention,
+
+    /// We can get by without the full query job map, so we won't bother waiting to obtain the
+    /// shard lock(s) if they're not already unlocked.
+    PartialAllowed,
+}
+
 /// Internal plumbing for collecting the set of active jobs for this query.
 ///
 /// Should only be called from `collect_active_jobs_from_all_queries`.
 ///
 /// (We arbitrarily use the word "gather" when collecting the jobs for
 /// each individual query, so that we have distinct function names to
 /// grep for.)
+///
+/// Aborts if jobs can't be gathered as specified by `collect_kind`.
 pub(crate) fn gather_active_jobs<'tcx, C>(
-    query: &'tcx QueryVTable<'tcx, C>,
     tcx: TyCtxt<'tcx>,
-    require_complete: bool,
-    job_map_out: &mut QueryJobMap<'tcx>, // Out-param; job info is gathered into this map
-) -> Option<()>
-where
+    query: &'tcx QueryVTable<'tcx, C>,
+    collect_kind: CollectActiveJobsKind,
+    job_map: &mut QueryJobMap<'tcx>,
+) where
     C: QueryCache<Key: QueryKey + DynSend + DynSync>,
     QueryVTable<'tcx, C>: DynSync,
 {
-    let mut active = Vec::new();
-
-    // Helper to gather active jobs from a single shard.
     let mut gather_shard_jobs = |shard: &HashTable<(C::Key, ActiveKeyStatus<'tcx>)>| {
-        for (k, v) in shard.iter() {
-            if let ActiveKeyStatus::Started(ref job) = *v {
-                active.push((*k, job.clone()));
+        for (key, status) in shard.iter() {
+            if let ActiveKeyStatus::Started(job) = status {
+                // This function is safe to call with the shard locked because it is very simple.
+                let frame = crate::plumbing::create_deferred_query_stack_frame(tcx, query, *key);
+                job_map.insert(job.id, QueryJobInfo { frame, job: job.clone() });
             }
         }
     };
 
-    // Lock shards and gather jobs from each shard.
-    if require_complete {
-        for shard in query.state.active.lock_shards() {
-            gather_shard_jobs(&shard);
+    match collect_kind {
+        CollectActiveJobsKind::Full => {
+            for shard in query.state.active.lock_shards() {
+                gather_shard_jobs(&shard);
+            }
         }
-    } else {
-        // We use try_lock_shards here since we are called from the
-        // deadlock handler, and this shouldn't be locked.
-        for shard in query.state.active.try_lock_shards() {
-            // This can be called during unwinding, and the function has a `try_`-prefix, so
-            // don't `unwrap()` here, just manually check for `None` and do best-effort error
-            // reporting.
-            match shard {
-                None => {
-                    tracing::warn!(
-                        "Failed to collect active jobs for query with name `{}`!",
-                        query.name
-                    );
-                    return None;
+        CollectActiveJobsKind::FullNoContention => {
+            for shard in query.state.active.try_lock_shards() {
+                match shard {
+                    Some(shard) => gather_shard_jobs(&shard),
+                    None => panic!("Failed to collect active jobs for query `{}`!", query.name),
+                }
+            }
+        }
+        CollectActiveJobsKind::PartialAllowed => {
+            for shard in query.state.active.try_lock_shards() {
+                match shard {
+                    Some(shard) => gather_shard_jobs(&shard),
+                    None => warn!("Failed to collect active jobs for query `{}`!", query.name),
                 }
-                Some(shard) => gather_shard_jobs(&shard),
             }
         }
     }
-
-    // Call `make_frame` while we're not holding a `state.active` lock as `make_frame` may call
-    // queries leading to a deadlock.
-    for (key, job) in active {
-        let frame = crate::plumbing::create_deferred_query_stack_frame(tcx, query, key);
-        job_map_out.insert(job.id, QueryJobInfo { frame, job });
-    }
-
-    Some(())
 }
 
 #[cold]
@@ -205,11 +211,10 @@ fn cycle_error<'tcx, C: QueryCache>(
     try_execute: QueryJobId,
     span: Span,
 ) -> (C::Value, Option<DepNodeIndex>) {
-    // Ensure there was no errors collecting all active jobs.
+    // Ensure there were no errors collecting all active jobs.
     // We need the complete map to ensure we find a cycle to break.
-    let job_map = collect_active_jobs_from_all_queries(tcx, false)
-        .ok()
-        .expect("failed to collect active queries");
+    let job_map =
+        collect_active_jobs_from_all_queries(tcx, CollectActiveJobsKind::FullNoContention);
 
     let error = find_cycle_in_stack(try_execute, job_map, &current_query_job(), span);
     (mk_cycle(query, tcx, error.lift()), None)

compiler/rustc_query_impl/src/job.rs

@@ -14,7 +14,7 @@ use rustc_middle::ty::TyCtxt;
 use rustc_session::Session;
 use rustc_span::{DUMMY_SP, Span};
 
-use crate::collect_active_jobs_from_all_queries;
+use crate::{CollectActiveJobsKind, collect_active_jobs_from_all_queries};
 
 /// Map from query job IDs to job information collected by
 /// `collect_active_jobs_from_all_queries`.
@@ -385,8 +385,7 @@ pub fn print_query_stack<'tcx>(
     let mut count_total = 0;
 
     // Make use of a partial query job map if we fail to take locks collecting active queries.
-    let job_map: QueryJobMap<'_> = collect_active_jobs_from_all_queries(tcx, false)
-        .unwrap_or_else(|partial_job_map| partial_job_map);
+    let job_map = collect_active_jobs_from_all_queries(tcx, CollectActiveJobsKind::PartialAllowed);
 
     if let Some(ref mut file) = file {
         let _ = writeln!(file, "\n\nquery stack during panic:");

compiler/rustc_query_impl/src/lib.rs

@@ -18,6 +18,7 @@ use rustc_middle::ty::TyCtxt;
 use rustc_span::Span;
 
 pub use crate::dep_kind_vtables::make_dep_kind_vtables;
+pub use crate::execution::CollectActiveJobsKind;
 pub use crate::job::{QueryJobMap, break_query_cycles, print_query_stack};
 use crate::profiling_support::QueryKeyStringCache;
 

compiler/rustc_query_impl/src/plumbing.rs

@@ -33,11 +33,10 @@ use rustc_span::def_id::LOCAL_CRATE;
 use crate::error::{QueryOverflow, QueryOverflowNote};
 use crate::execution::{all_inactive, force_query};
 use crate::job::find_dep_kind_root;
-use crate::{GetQueryVTable, collect_active_jobs_from_all_queries};
+use crate::{CollectActiveJobsKind, GetQueryVTable, collect_active_jobs_from_all_queries};
 
 fn depth_limit_error<'tcx>(tcx: TyCtxt<'tcx>, job: QueryJobId) {
-    let job_map =
-        collect_active_jobs_from_all_queries(tcx, true).expect("failed to collect active queries");
+    let job_map = collect_active_jobs_from_all_queries(tcx, CollectActiveJobsKind::Full);
     let (info, depth) = find_dep_kind_root(job, job_map);
 
     let suggested_limit = match tcx.recursion_limit() {
@@ -522,24 +521,20 @@ macro_rules! define_queries {
         /// complete map is needed and no deadlock is possible at this call site.
         pub fn collect_active_jobs_from_all_queries<'tcx>(
             tcx: TyCtxt<'tcx>,
-            require_complete: bool,
-        ) -> Result<QueryJobMap<'tcx>, QueryJobMap<'tcx>> {
-            let mut job_map_out = QueryJobMap::default();
-            let mut complete = true;
+            collect_kind: CollectActiveJobsKind,
+        ) -> QueryJobMap<'tcx> {
+            let mut job_map = QueryJobMap::default();
 
             $(
-                let res = crate::execution::gather_active_jobs(
-                    &tcx.query_system.query_vtables.$name,
+                crate::execution::gather_active_jobs(
                     tcx,
-                    require_complete,
-                    &mut job_map_out,
+                    &tcx.query_system.query_vtables.$name,
+                    collect_kind,
+                    &mut job_map,
                 );
-                if res.is_none() {
-                    complete = false;
-                }
             )*
 
-            if complete { Ok(job_map_out) } else { Err(job_map_out) }
+            job_map
         }
 
         /// All self-profiling events generated by the query engine use