This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11615

rhg: faster hg cat when many files are requested
ClosedPublic

Authored by aalekseyev on Oct 5 2021, 10:16 AM.

Details

Reviewers
martinvonz
Group Reviewers
hg-reviewers
Commits
rHG6b5773f89183: rhg: faster hg cat when many files are requested
Summary

With this patch I'm seeing a ~39ms improvement (220ms -> 181ms) when
running [hg cat] on ~220 files in a ~260k-file repo.

The timing for [hg cat] on an individual file becomes slightly worse
(losing 5ms: 145ms -> 150ms).
A follow-up commit is intended to improve that.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

aalekseyev created this revision.Oct 5 2021, 10:16 AM
aalekseyev edited the summary of this revision. (Show Details)Oct 5 2021, 10:44 AM
martinvonz requested changes to this revision.Oct 8 2021, 5:21 PM
martinvonz added a subscriber: martinvonz.
martinvonz added inline comments.
rust/hg-core/src/operations/cat.rs
71

nit: This is not actually the order they were specified, right? I think it makes the order be sorted. So you could do missing.sort_unstable() instead, I suppose. Or maybe they're already sorted and you don't need to do anything here? (I suspect the code initially didn't have the sorting on line 47.)

This revision now requires changes to proceed.Oct 8 2021, 5:21 PM
aalekseyev added inline comments.Oct 11 2021, 6:39 AM
rust/hg-core/src/operations/cat.rs
71

Wow, I made two mistakes here:

  • Assumed that the existing behavior of rhg cat should be preserved, whereas in fact it differs from python hg, so should probably be changed. The python hg actually reports these in a sorted order.
  • Forgot that I sorted files already

The mistakes "cancel out" in some sense, so you're right that this should just be simplified. I'm pushing a change.

aalekseyev updated this revision to Diff 30699.Oct 11 2021, 6:50 AM
aalekseyev added a comment.Oct 14 2021, 5:29 AM

Even though my measurement above talks about concatenation of 200 files, a real production use case we have is concatenating around 10000 files.
The time of that operation is improved from ~3.5 seconds to ~0.4 seconds by this change.

aalekseyev marked an inline comment as done.Oct 14 2021, 5:30 AM
aalekseyev added a commit: rHG6b5773f89183: rhg: faster hg cat when many files are requested.Oct 14 2021, 11:16 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG6b5773f89183: rhg: faster hg cat when many files are requested (authored by aalekseyev). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 30806

rust/hg-core/src/operations/cat.rs

// list_tracked_files.rs // list_tracked_files.rs
// //
// Copyright 2020 Antoine Cezar <antoine.cezar@octobus.net> // Copyright 2020 Antoine Cezar <antoine.cezar@octobus.net>
// //
// This software may be used and distributed according to the terms of the // This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version. // GNU General Public License version 2 or any later version.
use crate::repo::Repo; use crate::repo::Repo;
use crate::revlog::revlog::RevlogError; use crate::revlog::revlog::RevlogError;
use crate::revlog::Node; use crate::revlog::Node;
use crate::utils::hg_path::HgPathBuf; use crate::utils::hg_path::HgPathBuf;
use itertools::EitherOrBoth::{Both, Left, Right};
use itertools::Itertools;
pub struct CatOutput { pub struct CatOutput {
/// Whether any file in the manifest matched the paths given as CLI /// Whether any file in the manifest matched the paths given as CLI
/// arguments /// arguments
pub found_any: bool, pub found_any: bool,
/// The contents of matching files, in manifest order /// The contents of matching files, in manifest order
pub concatenated: Vec<u8>, pub concatenated: Vec<u8>,
/// Which of the CLI arguments did not match any manifest file /// Which of the CLI arguments did not match any manifest file
pub missing: Vec<HgPathBuf>, pub missing: Vec<HgPathBuf>,
/// The node ID that the given revset was resolved to /// The node ID that the given revset was resolved to
pub node: Node, pub node: Node,
} }
/// Output the given revision of files /// Output the given revision of files
/// ///
/// * `root`: Repository root /// * `root`: Repository root
/// * `rev`: The revision to cat the files from. /// * `rev`: The revision to cat the files from.
/// * `files`: The files to output. /// * `files`: The files to output.
pub fn cat<'a>( pub fn cat<'a>(
repo: &Repo, repo: &Repo,
revset: &str, revset: &str,
files: &'a [HgPathBuf], mut files: Vec<HgPathBuf>,
) -> Result<CatOutput, RevlogError> { ) -> Result<CatOutput, RevlogError> {
let rev = crate::revset::resolve_single(revset, repo)?; let rev = crate::revset::resolve_single(revset, repo)?;
let manifest = repo.manifest_for_rev(rev)?; let manifest = repo.manifest_for_rev(rev)?;
let node = *repo let node = *repo
.changelog()? .changelog()?
.node_from_rev(rev) .node_from_rev(rev)
.expect("should succeed when repo.manifest did"); .expect("should succeed when repo.manifest did");
let mut bytes = vec![]; let mut bytes = vec![];
let mut matched = vec![false; files.len()];
let mut found_any = false; let mut found_any = false;
files.sort_unstable();
let mut missing = vec![];
for (manifest_file, node_bytes) in manifest.files_with_nodes() { for entry in manifest
for (cat_file, is_matched) in files.iter().zip(&mut matched) { .files_with_nodes()
if cat_file.as_bytes() == manifest_file.as_bytes() { .merge_join_by(files.iter(), |(manifest_file, _), file| {
*is_matched = true; manifest_file.cmp(&file.as_ref())
})
{
match entry {
Left(_) => (),
Right(path) => missing.push(path),
Both((manifest_file, node_bytes), _) => {
found_any = true; found_any = true;
let file_log = repo.filelog(manifest_file)?; let file_log = repo.filelog(manifest_file)?;
let file_node = Node::from_hex_for_repo(node_bytes)?; let file_node = Node::from_hex_for_repo(node_bytes)?;
let entry = file_log.data_for_node(file_node)?; let entry = file_log.data_for_node(file_node)?;
bytes.extend(entry.data()?) bytes.extend(entry.data()?)
} }
} }
} }
let missing: Vec<_> = files let missing: Vec<HgPathBuf> = missing
.iter() .iter()
martinvonzUnsubmitted
Done

nit: This is not actually the order they were specified, right? I think it makes the order be sorted. So you could do missing.sort_unstable() instead, I suppose. Or maybe they're already sorted and you don't need to do anything here? (I suspect the code initially didn't have the sorting on line 47.)

martinvonz: nit: This is not actually the order they were specified, right? I think it makes the order be…
aalekseyevAuthorUnsubmitted
Done

Wow, I made two mistakes here:

  • Assumed that the existing behavior of rhg cat should be preserved, whereas in fact it differs from python hg, so should probably be changed. The python hg actually reports these in a sorted order.
  • Forgot that I sorted files already

The mistakes "cancel out" in some sense, so you're right that this should just be simplified. I'm pushing a change.

aalekseyev: Wow, I made two mistakes here: - Assumed that the existing behavior of `rhg cat` should be…
.zip(&matched) .map(|file| (*(file.as_ref())).to_owned())
.filter(|pair| !*pair.1)
.map(|pair| pair.0.clone())
.collect(); .collect();
Ok(CatOutput { Ok(CatOutput {
found_any, found_any,
concatenated: bytes, concatenated: bytes,
missing, missing,
node, node,
}) })
} }

rust/rhg/src/commands/cat.rs

} }
// TODO probably move this to a util function like `repo.default_rev` or // TODO probably move this to a util function like `repo.default_rev` or
// something when it's used somewhere else // something when it's used somewhere else
let rev = match rev { let rev = match rev {
Some(r) => r.to_string(), Some(r) => r.to_string(),
None => format!("{:x}", repo.dirstate_parents()?.p1), None => format!("{:x}", repo.dirstate_parents()?.p1),
}; };
let output = cat(&repo, &rev, &files).map_err(|e| (e, rev.as_str()))?; let output = cat(&repo, &rev, files).map_err(|e| (e, rev.as_str()))?;
invocation.ui.write_stdout(&output.concatenated)?; invocation.ui.write_stdout(&output.concatenated)?;
if !output.missing.is_empty() { if !output.missing.is_empty() {
let short = format!("{:x}", output.node.short()).into_bytes(); let short = format!("{:x}", output.node.short()).into_bytes();
for path in &output.missing { for path in &output.missing {
invocation.ui.write_stderr(&format_bytes!( invocation.ui.write_stderr(&format_bytes!(
b"{}: no such file in rev {}\n", b"{}: no such file in rev {}\n",
path.as_bytes(), path.as_bytes(),
short short