From 326be7c91ef1ac1fba450b3fffe832b64d00b287 Mon Sep 17 00:00:00 2001 From: Waleed Khan Date: Thu, 3 Aug 2023 08:13:56 -0700 Subject: [PATCH] working_copy: send updates via `channel` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In preparation of traversing the filesystem in parallel, send updates via `channel`. An alternative is to modify shared mutable state, e.g. put `self.file_states` behind a mutex or use a concurrent hash-map. This risks leaving the `TreeState` in an invalid state if an error occurs, and makes invariants harder to reason about. Using a channel introduces a small performance regression. (I didn't try out the concurrent hash-map approach.) ```sh $ hyperfine --parameter-list hash before,after --setup 'git checkout {hash} && cargo build --profile release-with-debug' --warmup 3 './target/release-with-debug/jj -R ../nixpkgs st' Benchmark 1: ./target/release-with-debug/jj -R ../nixpkgs st (hash = before) Time (mean ± σ): 1.533 s ± 0.013 s [User: 0.587 s, System: 0.926 s] Range (min … max): 1.510 s … 1.559 s 10 runs Benchmark 2: ./target/release-with-debug/jj -R ../nixpkgs st (hash = after) Time (mean ± σ): 1.563 s ± 0.021 s [User: 0.607 s, System: 0.936 s] Range (min … max): 1.518 s … 1.595 s 10 runs Summary ./target/release-with-debug/jj -R ../nixpkgs st (hash = before) ran 1.02 ± 0.02 times faster than ./target/release-with-debug/jj -R ../nixpkgs st (hash = after) ``` --- lib/src/working_copy.rs | 45 ++++++++++++++++++++++++++++++----------- 1 file changed, 33 insertions(+), 12 deletions(-) diff --git a/lib/src/working_copy.rs b/lib/src/working_copy.rs index 33b768a9f..201c6a656 100644 --- a/lib/src/working_copy.rs +++ b/lib/src/working_copy.rs @@ -26,6 +26,7 @@ use std::os::unix::fs::symlink; #[cfg(unix)] use std::os::unix::fs::PermissionsExt; use std::path::{Path, PathBuf}; +use std::sync::mpsc::{channel, Sender}; use std::sync::Arc; use std::time::UNIX_EPOCH; @@ -51,7 +52,6 @@ use crate::op_store::{OperationId, WorkspaceId}; use crate::repo_path::{FsPathParseError, RepoPath, RepoPathComponent, RepoPathJoin}; use crate::store::Store; use crate::tree::{Diff, Tree}; -use crate::tree_builder::TreeBuilder; #[cfg(unix)] type FileExecutableFlag = bool; @@ -655,17 +655,34 @@ impl TreeState { git_ignore: base_ignores, }]; trace_span!("traverse filesystem").in_scope(|| -> Result<(), SnapshotError> { + let (tree_entries_tx, tree_entries_rx) = channel(); + let (file_states_tx, file_states_rx) = channel(); + let (deleted_files_tx, deleted_files_rx) = channel(); while let Some(work_item) = work.pop() { work.extend(self.visit_directory( &matcher, ¤t_tree, - &mut tree_builder, - &mut deleted_files, + tree_entries_tx.clone(), + file_states_tx.clone(), + deleted_files_tx.clone(), work_item, progress, )?); } + drop(tree_entries_tx); + drop(file_states_tx); + drop(deleted_files_tx); + while let Ok((path, tree_value)) = tree_entries_rx.recv() { + tree_builder.set(path, tree_value); + } + while let Ok((path, file_state)) = file_states_rx.recv() { + self.file_states.insert(path, file_state); + } + while let Ok(path) = deleted_files_rx.recv() { + deleted_files.remove(&path); + } + Ok(()) })?; @@ -679,12 +696,14 @@ impl TreeState { Ok(has_changes || fsmonitor_clock_needs_save) } + #[allow(clippy::too_many_arguments)] fn visit_directory( - &mut self, + &self, matcher: &dyn Matcher, current_tree: &Tree, - tree_builder: &mut TreeBuilder, - deleted_files: &mut HashSet, + tree_entries_tx: Sender<(RepoPath, TreeValue)>, + file_states_tx: Sender<(RepoPath, FileState)>, + deleted_files_tx: Sender, work_item: WorkItem, progress: Option<&SnapshotProgress>, ) -> Result, SnapshotError> { @@ -747,7 +766,7 @@ impl TreeState { } }; if let Some(new_file_state) = file_state(&metadata) { - deleted_files.remove(&tracked_path); + deleted_files_tx.send(tracked_path.clone()).ok(); let update = self.get_updated_tree_value( &tracked_path, disk_path, @@ -756,9 +775,11 @@ impl TreeState { &new_file_state, )?; if let Some(tree_value) = update { - tree_builder.set(tracked_path.clone(), tree_value); + tree_entries_tx + .send((tracked_path.clone(), tree_value)) + .ok(); } - self.file_states.insert(tracked_path, new_file_state); + file_states_tx.send((tracked_path, new_file_state)).ok(); } } } else { @@ -785,7 +806,7 @@ impl TreeState { err, })?; if let Some(new_file_state) = file_state(&metadata) { - deleted_files.remove(&path); + deleted_files_tx.send(path.clone()).ok(); let update = self.get_updated_tree_value( &path, entry.path(), @@ -794,9 +815,9 @@ impl TreeState { &new_file_state, )?; if let Some(tree_value) = update { - tree_builder.set(path.clone(), tree_value); + tree_entries_tx.send((path.clone(), tree_value)).ok(); } - self.file_states.insert(path, new_file_state); + file_states_tx.send((path, new_file_state)).ok(); } } }