From 4a8d250f2c015748715c541a9381fabcf3ec6ff1 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Tue, 3 Sep 2024 20:13:46 -0700 Subject: [PATCH] store: make `write_file()` async --- cli/src/commands/fix.rs | 6 ++++-- cli/src/merge_tools/builtin.rs | 5 ++++- cli/src/merge_tools/external.rs | 3 ++- lib/src/conflicts.rs | 8 +++++--- lib/src/default_index/revset_engine.rs | 6 ++++-- lib/src/local_working_copy.rs | 19 ++++++++----------- lib/src/merged_tree.rs | 17 +++++++++-------- lib/src/store.rs | 4 ++-- lib/src/tree.rs | 12 ++++++++---- lib/testutils/src/lib.rs | 5 ++++- 10 files changed, 50 insertions(+), 35 deletions(-) diff --git a/cli/src/commands/fix.rs b/cli/src/commands/fix.rs index 20c33a871..718d10e85 100644 --- a/cli/src/commands/fix.rs +++ b/cli/src/commands/fix.rs @@ -344,8 +344,10 @@ fn fix_file_ids<'a>( } }); if new_content != old_content { - let new_file_id = - store.write_file(&tool_input.repo_path, &mut new_content.as_slice())?; + // TODO: send futures back over channel + let new_file_id = store + .write_file(&tool_input.repo_path, &mut new_content.as_slice()) + .block_on()?; updates_tx.send((tool_input, new_file_id)).unwrap(); } } diff --git a/cli/src/merge_tools/builtin.rs b/cli/src/merge_tools/builtin.rs index 68354cb1a..1238f3bc8 100644 --- a/cli/src/merge_tools/builtin.rs +++ b/cli/src/merge_tools/builtin.rs @@ -453,6 +453,7 @@ pub fn apply_diff_builtin( files.len(), "result had a different number of files" ); + // TODO: Write files concurrently for (path, file) in changed_files.into_iter().zip(files) { let (selected, _unselected) = file.get_selected_contents(); match selected { @@ -496,7 +497,9 @@ pub fn apply_diff_builtin( tree_builder.set_or_remove(path, value); } scm_record::SelectedContents::Present { contents } => { - let file_id = store.write_file(&path, &mut contents.as_bytes())?; + let file_id = store + .write_file(&path, &mut contents.as_bytes()) + .block_on()?; tree_builder.set_or_remove( path, Merge::normal(TreeValue::File { diff --git a/cli/src/merge_tools/external.rs b/cli/src/merge_tools/external.rs index 1800d692f..a01187b18 100644 --- a/cli/src/merge_tools/external.rs +++ b/cli/src/merge_tools/external.rs @@ -231,7 +231,8 @@ pub fn run_mergetool_external( } else { let new_file_id = tree .store() - .write_file(repo_path, &mut output_file_contents.as_slice())?; + .write_file(repo_path, &mut output_file_contents.as_slice()) + .block_on()?; Merge::normal(new_file_id) }; let new_tree_value = match new_file_ids.into_resolved() { diff --git a/lib/src/conflicts.rs b/lib/src/conflicts.rs index 54fb6b13c..805097185 100644 --- a/lib/src/conflicts.rs +++ b/lib/src/conflicts.rs @@ -25,6 +25,7 @@ use futures::Stream; use futures::StreamExt; use futures::TryStreamExt; use itertools::Itertools; +use pollster::FutureExt; use regex::bytes::Regex; use regex::bytes::RegexBuilder; @@ -513,7 +514,7 @@ pub async fn update_from_content( }; }; // Either there are no markers or they don't have the expected arity - let file_id = store.write_file(path, &mut &content[..])?; + let file_id = store.write_file(path, &mut &content[..]).await?; return Ok(Merge::normal(file_id)); }; @@ -535,17 +536,18 @@ pub async fn update_from_content( if zip(contents.iter(), used_file_ids.iter()) .any(|(content, file_id)| file_id.is_none() && !content.is_empty()) { - let file_id = store.write_file(path, &mut &content[..])?; + let file_id = store.write_file(path, &mut &content[..]).await?; return Ok(Merge::normal(file_id)); } // Now write the new files contents we found by parsing the file with conflict // markers. + // TODO: Write these concurrently let new_file_ids: Vec> = zip(contents.iter(), used_file_ids.iter()) .map(|(content, file_id)| -> BackendResult> { match file_id { Some(_) => { - let file_id = store.write_file(path, &mut content.as_slice())?; + let file_id = store.write_file(path, &mut content.as_slice()).block_on()?; Ok(Some(file_id)) } None => { diff --git a/lib/src/default_index/revset_engine.rs b/lib/src/default_index/revset_engine.rs index c1c466224..b8473c468 100644 --- a/lib/src/default_index/revset_engine.rs +++ b/lib/src/default_index/revset_engine.rs @@ -1170,9 +1170,10 @@ fn has_diff_from_parent( // TODO: handle copy tracking let mut tree_diff = from_tree.diff_stream(&to_tree, matcher); async { + // TODO: Resolve values concurrently while let Some(entry) = tree_diff.next().await { let (from_value, to_value) = entry.values?; - let from_value = resolve_file_values(store, &entry.path, from_value)?; + let from_value = resolve_file_values(store, &entry.path, from_value).await?; if from_value == to_value { continue; } @@ -1197,9 +1198,10 @@ fn matches_diff_from_parent( // TODO: handle copy tracking let mut tree_diff = from_tree.diff_stream(&to_tree, files_matcher); async { + // TODO: Resolve values concurrently while let Some(entry) = tree_diff.next().await { let (left_value, right_value) = entry.values?; - let left_value = resolve_file_values(store, &entry.path, left_value)?; + let left_value = resolve_file_values(store, &entry.path, left_value).await?; if left_value == right_value { continue; } diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index a11cb632a..c3c734722 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -606,7 +606,7 @@ impl TreeState { return Err(TreeStateError::ReadTreeState { path: tree_state_path, source: err, - }) + }); } Ok(file) => file, }; @@ -709,7 +709,7 @@ impl TreeState { self.store.get_root_tree(&self.tree_id) } - fn write_file_to_store( + async fn write_file_to_store( &self, path: &RepoPath, disk_path: &Path, @@ -718,7 +718,7 @@ impl TreeState { message: format!("Failed to open file {}", disk_path.display()), err: err.into(), })?; - Ok(self.store.write_file(path, &mut file)?) + Ok(self.store.write_file(path, &mut file).await?) } fn write_symlink_to_store( @@ -1152,12 +1152,9 @@ impl TreeState { new_file_state.file_type.clone() }; let new_tree_values = match new_file_type { - FileType::Normal { executable } => self.write_path_to_store( - repo_path, - &disk_path, - ¤t_tree_values, - executable, - )?, + FileType::Normal { executable } => self + .write_path_to_store(repo_path, &disk_path, ¤t_tree_values, executable) + .block_on()?, FileType::Symlink => { let id = self.write_symlink_to_store(repo_path, &disk_path)?; Merge::normal(TreeValue::Symlink(id)) @@ -1172,7 +1169,7 @@ impl TreeState { } } - fn write_path_to_store( + async fn write_path_to_store( &self, repo_path: &RepoPath, disk_path: &Path, @@ -1184,7 +1181,7 @@ impl TreeState { if let Some(current_tree_value) = current_tree_values.as_resolved() { #[cfg(unix)] let _ = current_tree_value; // use the variable - let id = self.write_file_to_store(repo_path, disk_path)?; + let id = self.write_file_to_store(repo_path, disk_path).await?; // On Windows, we preserve the executable bit from the current tree. #[cfg(windows)] let executable = { diff --git a/lib/src/merged_tree.rs b/lib/src/merged_tree.rs index a5eb4d901..81090aedb 100644 --- a/lib/src/merged_tree.rs +++ b/lib/src/merged_tree.rs @@ -451,9 +451,10 @@ fn merge_trees(merge: &Merge) -> BackendResult> { // any conflicts. let mut new_tree = backend::Tree::default(); let mut conflicts = vec![]; + // TODO: Merge values concurrently for (basename, path_merge) in all_merged_tree_entries(merge) { let path = dir.join(basename); - let path_merge = merge_tree_values(store, &path, &path_merge)?; + let path_merge = merge_tree_values(store, &path, &path_merge).block_on()?; match path_merge.into_resolved() { Ok(value) => { new_tree.set_or_remove(basename, value); @@ -487,10 +488,10 @@ fn merge_trees(merge: &Merge) -> BackendResult> { /// Ok(Merge::normal(value)) if the conflict was resolved, and /// Ok(Merge::absent()) if the path should be removed. Returns the /// conflict unmodified if it cannot be resolved automatically. -fn merge_tree_values( +async fn merge_tree_values( store: &Arc, path: &RepoPath, - values: &MergedTreeVal, + values: &MergedTreeVal<'_>, ) -> BackendResult { if let Some(resolved) = values.resolve_trivial() { return Ok(Merge::resolved(resolved.cloned())); @@ -504,7 +505,7 @@ fn merge_tree_values( Ok(merged_tree .map(|tree| (tree.id() != empty_tree_id).then(|| TreeValue::Tree(tree.id().clone())))) } else { - let maybe_resolved = try_resolve_file_values(store, path, values)?; + let maybe_resolved = try_resolve_file_values(store, path, values).await?; Ok(maybe_resolved.unwrap_or_else(|| values.cloned())) } } @@ -512,7 +513,7 @@ fn merge_tree_values( /// Tries to resolve file conflicts by merging the file contents. Treats missing /// files as empty. If the file conflict cannot be resolved, returns the passed /// `values` unmodified. -pub fn resolve_file_values( +pub async fn resolve_file_values( store: &Arc, path: &RepoPath, values: MergedTreeValue, @@ -521,11 +522,11 @@ pub fn resolve_file_values( return Ok(Merge::resolved(resolved.clone())); } - let maybe_resolved = try_resolve_file_values(store, path, &values)?; + let maybe_resolved = try_resolve_file_values(store, path, &values).await?; Ok(maybe_resolved.unwrap_or(values)) } -fn try_resolve_file_values>( +async fn try_resolve_file_values>( store: &Arc, path: &RepoPath, values: &Merge>, @@ -537,7 +538,7 @@ fn try_resolve_file_values>( .simplify(); // No fast path for simplified.is_resolved(). If it could be resolved, it would // have been caught by values.resolve_trivial() above. - if let Some(resolved) = try_resolve_file_conflict(store, path, &simplified)? { + if let Some(resolved) = try_resolve_file_conflict(store, path, &simplified).await? { Ok(Some(Merge::normal(resolved))) } else { // Failed to merge the files, or the paths are not files diff --git a/lib/src/store.rs b/lib/src/store.rs index 8f62b01ad..8b52f99a4 100644 --- a/lib/src/store.rs +++ b/lib/src/store.rs @@ -242,12 +242,12 @@ impl Store { self.backend.read_file(path, id).await } - pub fn write_file( + pub async fn write_file( &self, path: &RepoPath, contents: &mut (dyn Read + Send), ) -> BackendResult { - self.backend.write_file(path, contents).block_on() + self.backend.write_file(path, contents).await } pub fn read_symlink(&self, path: &RepoPath, id: &SymlinkId) -> BackendResult { diff --git a/lib/src/tree.rs b/lib/src/tree.rs index 0c59e18a5..091c9ef1c 100644 --- a/lib/src/tree.rs +++ b/lib/src/tree.rs @@ -397,7 +397,8 @@ fn merge_tree_value( Err(conflict) => { let conflict_borrowed = conflict.map(|value| value.as_ref()); if let Some(tree_value) = - try_resolve_file_conflict(store, &filename, &conflict_borrowed)? + try_resolve_file_conflict(store, &filename, &conflict_borrowed) + .block_on()? { Some(tree_value) } else { @@ -414,10 +415,10 @@ fn merge_tree_value( /// /// The input `conflict` is supposed to be simplified. It shouldn't contain /// non-file values that cancel each other. -pub fn try_resolve_file_conflict( +pub async fn try_resolve_file_conflict( store: &Store, filename: &RepoPath, - conflict: &MergedTreeVal, + conflict: &MergedTreeVal<'_>, ) -> BackendResult> { // If there are any non-file or any missing parts in the conflict, we can't // merge it. We check early so we don't waste time reading file contents if @@ -456,6 +457,7 @@ pub fn try_resolve_file_conflict( // cannot let file_id_conflict = file_id_conflict.simplify(); + // TODO: Read the files concurrently let contents: Merge> = file_id_conflict.try_map(|&file_id| -> BackendResult> { let mut content = vec![]; @@ -472,7 +474,9 @@ pub fn try_resolve_file_conflict( let merge_result = files::merge(&contents); match merge_result { MergeResult::Resolved(merged_content) => { - let id = store.write_file(filename, &mut merged_content.as_slice())?; + let id = store + .write_file(filename, &mut merged_content.as_slice()) + .await?; Ok(Some(TreeValue::File { id, executable })) } MergeResult::Conflict(_) => Ok(None), diff --git a/lib/testutils/src/lib.rs b/lib/testutils/src/lib.rs index ec3a291b6..3b3410b0a 100644 --- a/lib/testutils/src/lib.rs +++ b/lib/testutils/src/lib.rs @@ -298,7 +298,10 @@ pub fn read_file(store: &Store, path: &RepoPath, id: &FileId) -> Vec { } pub fn write_file(store: &Store, path: &RepoPath, contents: &str) -> FileId { - store.write_file(path, &mut contents.as_bytes()).unwrap() + store + .write_file(path, &mut contents.as_bytes()) + .block_on() + .unwrap() } pub fn write_normal_file(