Run linking and incremental saving / finalizing in parallel

[Zoxc]

This makes saving the dep graph and finalizing the incremental session directory run in parallel with linking.

It adds a task function which allows a closure to run on the Rayon thread pool and be waited on later. This is used in codegen_and_build_linker to start the linker in separate task.

This is blocked on rust-lang/rustc-rayon#12 as otherwise this runs into deadlocks.

[rustbot]

r? @estebank

rustbot has assigned @estebank.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[bjorn3]

Can we keep a timer for the entire time linking takes?

[Zoxc]

We still have link_crate which covers the codegen backend part of linking.

[bjorn3]

Why does this need to run in a separate task? It is immediately awaited, right? The parallelism should happen inside the link method using join, right?

[Zoxc]

It's not immediately awaited. The parallelism happens because we create a task here (think of it as a separate thread).

[bjorn3]

I would have expected the parallelism to come from the join call inside link.

[bjorn3]

Ah, there is a save_dep_graph call inside compiler.enter() that happens after this call. Instead of the join() call and channels would it be possible to move the finalize_session_directory and dep graph dropping to right after the save_dep_graph call? That seems a lot simpler.

[bjorn3]

Something like this. I also fixed a pre-existing incr comp issue where finalize_session_directory is called before the dep graph is actually written, resulting in a broken incr comp cache if you interrupt the build at just the right moment.

diff --git a/compiler/rustc_interface/src/queries.rs b/compiler/rustc_interface/src/queries.rs
index df7778cd512..9376fa9552c 100644
--- a/compiler/rustc_interface/src/queries.rs
+++ b/compiler/rustc_interface/src/queries.rs
@@ -5,11 +5,10 @@
 use rustc_ast as ast;
 use rustc_codegen_ssa::traits::CodegenBackend;
 use rustc_codegen_ssa::CodegenResults;
-use rustc_data_structures::jobserver;
 use rustc_data_structures::steal::Steal;
 use rustc_data_structures::svh::Svh;
 use rustc_data_structures::sync::{
-    join, task, AppendOnlyIndexVec, DynSend, FreezeLock, Lrc, OnceLock, Task, WorkerLocal,
+    task, AppendOnlyIndexVec, DynSend, FreezeLock, Lrc, OnceLock, Task, WorkerLocal,
 };
 use rustc_hir::def::DefKind;
 use rustc_hir::def_id::{StableCrateId, CRATE_DEF_ID, LOCAL_CRATE};
@@ -26,8 +25,7 @@
 use rustc_session::Session;
 use rustc_span::symbol::sym;
 use std::any::Any;
-use std::cell::{RefCell, RefMut};
-use std::sync::mpsc::{sync_channel, Receiver, SyncSender};
+use std::cell::{OnceCell, RefCell, RefMut};
 use std::sync::Arc;
 
 /// Represent the result of a query.
@@ -89,26 +87,23 @@ pub struct Queries<'tcx> {
     arena: WorkerLocal<Arena<'tcx>>,
     hir_arena: WorkerLocal<rustc_hir::Arena<'tcx>>,
 
-    dep_graph_serialized_rx: Steal<Receiver<()>>,
-    dep_graph_serialized_tx: SyncSender<()>,
-
     parse: Query<ast::Crate>,
     // This just points to what's in `gcx_cell`.
     gcx: Query<&'tcx GlobalCtxt<'tcx>>,
+    // Only present when incr. comp. is enabled.
+    crate_hash: OnceCell<Option<Svh>>,
 }
 
 impl<'tcx> Queries<'tcx> {
     pub fn new(compiler: &'tcx Compiler) -> Queries<'tcx> {
-        let (tx, rx) = sync_channel(1);
         Queries {
             compiler,
-            dep_graph_serialized_rx: Steal::new(rx),
-            dep_graph_serialized_tx: tx,
             gcx_cell: OnceLock::new(),
             arena: WorkerLocal::new(|_| Arena::default()),
             hir_arena: WorkerLocal::new(|_| rustc_hir::Arena::default()),
             parse: Default::default(),
             gcx: Default::default(),
+            crate_hash: Default::default(),
         }
     }
 
@@ -245,31 +240,27 @@ pub fn codegen_and_build_linker(&'tcx self) -> Result<Task<Result<()>>> {
             let ongoing_codegen = passes::start_codegen(&*self.compiler.codegen_backend, tcx);
 
             let linker = Linker {
-                dep_graph_serialized_rx: self.dep_graph_serialized_rx.steal(),
                 dep_graph: tcx.dep_graph.clone(),
                 output_filenames: tcx.output_filenames(()).clone(),
-                crate_hash: if tcx.needs_crate_hash() {
-                    Some(tcx.crate_hash(LOCAL_CRATE))
-                } else {
-                    None
-                },
                 ongoing_codegen,
             };
 
             let sess = self.compiler.sess.clone();
             let codegen_backend = self.compiler.codegen_backend.clone();
 
+            // Ensure the crate hash is computed if necessary
+            self.crate_hash
+                .set(if tcx.needs_crate_hash() { Some(tcx.crate_hash(LOCAL_CRATE)) } else { None })
+                .unwrap();
+
             Ok(task(move || linker.link(&sess, &*codegen_backend)))
         })
     }
 }
 
 struct Linker {
-    dep_graph_serialized_rx: Receiver<()>,
     dep_graph: DepGraph,
     output_filenames: Arc<OutputFilenames>,
-    // Only present when incr. comp. is enabled.
-    crate_hash: Option<Svh>,
     ongoing_codegen: Box<dyn Any + DynSend>,
 }
 
@@ -286,54 +277,34 @@ fn link(self, sess: &Lrc<Session>, codegen_backend: &dyn CodegenBackend) -> Resu
             rustc_incremental::save_work_product_index(sess, &self.dep_graph, work_products)
         });
 
-        let dep_graph_serialized_rx = self.dep_graph_serialized_rx;
+        let prof = sess.prof.clone();
+        prof.generic_activity("drop_dep_graph").run(move || drop(self.dep_graph));
 
-        join(
-            || {
-                if !sess
-                    .opts
-                    .output_types
-                    .keys()
-                    .any(|&i| i == OutputType::Exe || i == OutputType::Metadata)
-                {
-                    return Ok(());
-                }
-
-                if sess.opts.unstable_opts.no_link {
-                    let rlink_file = self.output_filenames.with_extension(config::RLINK_EXT);
-                    CodegenResults::serialize_rlink(
-                        sess,
-                        &rlink_file,
-                        &codegen_results,
-                        &*self.output_filenames,
-                    )
-                    .map_err(|error| {
-                        sess.dcx().emit_fatal(FailedWritingFile { path: &rlink_file, error })
-                    })?;
-                    return Ok(());
-                }
-
-                let _timer = sess.prof.verbose_generic_activity("link_crate");
-                codegen_backend.link(sess, codegen_results, &self.output_filenames)
-            },
-            || {
-                let dep_graph_serialized_rx = dep_graph_serialized_rx;
-
-                // Wait for the dep graph to be serialized before finalizing the session directory.
-                if !dep_graph_serialized_rx.try_recv().is_ok() {
-                    jobserver::release_thread();
-                    dep_graph_serialized_rx.recv().unwrap();
-                    jobserver::acquire_thread();
-                }
+        if !sess
+            .opts
+            .output_types
+            .keys()
+            .any(|&i| i == OutputType::Exe || i == OutputType::Metadata)
+        {
+            return Ok(());
+        }
 
-                sess.prof.generic_activity("drop_dep_graph").run(move || drop(self.dep_graph));
+        if sess.opts.unstable_opts.no_link {
+            let rlink_file = self.output_filenames.with_extension(config::RLINK_EXT);
+            CodegenResults::serialize_rlink(
+                sess,
+                &rlink_file,
+                &codegen_results,
+                &*self.output_filenames,
+            )
+            .map_err(|error| {
+                sess.dcx().emit_fatal(FailedWritingFile { path: &rlink_file, error })
+            })?;
+            return Ok(());
+        }
 
-                // Now that we won't touch anything in the incremental compilation directory
-                // any more, we can finalize it (which involves renaming it)
-                rustc_incremental::finalize_session_directory(sess, self.crate_hash);
-            },
-        )
-        .0
+        let _timer = sess.prof.verbose_generic_activity("link_crate");
+        codegen_backend.link(sess, codegen_results, &self.output_filenames)
     }
 }
 
@@ -362,16 +333,20 @@ pub fn enter<F, T>(&self, f: F) -> T
             self.sess.time("serialize_dep_graph", || gcx.enter(rustc_incremental::save_dep_graph));
         }
 
-        // Finish the dep graph encoding before we signal `dep_graph_serialized`.
+        // The timer's lifetime spans the dropping of `queries`, which contains
+        // the global context.
+        _timer = Some(self.sess.timer("free_global_ctxt"));
         if let Err((path, error)) = queries.finish() {
             self.sess.dcx().emit_fatal(errors::FailedWritingFile { path: &path, error });
         }
 
-        queries.dep_graph_serialized_tx.send(()).ok();
+        // Now that we won't touch anything in the incremental compilation directory
+        // any more, we can finalize it (which involves renaming it)
+        rustc_incremental::finalize_session_directory(
+            &queries.compiler.sess,
+            queries.crate_hash.get().unwrap().clone(),
+        );
 
-        // The timer's lifetime spans the dropping of `queries`, which contains
-        // the global context.
-        _timer = Some(self.sess.timer("free_global_ctxt"));
         ret
     }
 }

[Zoxc]

would it be possible to move the finalize_session_directory and dep graph dropping to right after the save_dep_graph call?

No. finalize_session_directory needs to happen after we save the work product from the codegen backend.

[bjorn3]

Where exactly does the requirement for turning Box<dyn CodegenBackend> into Lrc<dyn CodegenBackend> come from?

[Zoxc]

It's used in the task which calls link.

[bjorn3]

Maybe codegen_backend.link() itself could return a Task instead?

[bors]

☔ The latest upstream changes (presumably #122151) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #139011) made this pull request unmergeable. Please resolve the merge conflicts.