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.