-
-
Notifications
You must be signed in to change notification settings - Fork 452
gix-blame: Incremental #2457
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
gix-blame: Incremental #2457
Changes from all commits
2e3dbbb
d04cec5
a84beaa
1be2065
939357d
248303e
a85c1fe
5b94604
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,134 @@ | ||
| use std::{ | ||
| env, | ||
| ops::ControlFlow, | ||
| path::{Path, PathBuf}, | ||
| process::Command, | ||
| }; | ||
|
|
||
| use criterion::{criterion_group, criterion_main, Criterion}; | ||
| use gix_blame::{BlameEntry, BlameRanges, BlameSink}; | ||
| use gix_object::bstr::BString; | ||
|
|
||
| const DEFAULT_BENCH_PATH: &str = "gix-blame/src/file/function.rs"; | ||
|
|
||
| struct DiscardSink; | ||
|
|
||
| impl BlameSink for DiscardSink { | ||
| fn push(&mut self, _entry: BlameEntry) -> ControlFlow<()> { | ||
| ControlFlow::Continue(()) | ||
| } | ||
| } | ||
|
|
||
| fn incremental_options() -> gix_blame::Options { | ||
| gix_blame::Options { | ||
| diff_algorithm: gix_diff::blob::Algorithm::Histogram, | ||
| ranges: BlameRanges::default(), | ||
| since: None, | ||
| rewrites: Some(gix_diff::Rewrites::default()), | ||
| debug_track_path: false, | ||
| } | ||
| } | ||
|
|
||
| fn benchmark_incremental_blame(c: &mut Criterion) { | ||
| let repo_path = env::var_os("GIX_BLAME_BENCH_REPO").map_or_else( | ||
| || { | ||
| PathBuf::from(env!("CARGO_MANIFEST_DIR")) | ||
| .parent() | ||
| .expect("gix-blame crate has a parent directory") | ||
| .to_path_buf() | ||
| }, | ||
| PathBuf::from, | ||
| ); | ||
| let source_file_path: BString = env::var("GIX_BLAME_BENCH_PATH") | ||
| .unwrap_or_else(|_| DEFAULT_BENCH_PATH.into()) | ||
| .into_bytes() | ||
| .into(); | ||
| let commit_spec = env::var("GIX_BLAME_BENCH_COMMIT").unwrap_or_else(|_| "HEAD".into()); | ||
| let git_dir = repo_path.join(".git"); | ||
|
|
||
| if !has_commit_graph_cache(&git_dir) { | ||
| write_changed_path_commit_graph(&repo_path).expect("commit-graph should be writable for benchmark repository"); | ||
| } | ||
|
|
||
| let repo = gix::open(&repo_path).expect("repository can be opened"); | ||
| let suspect = repo | ||
| .rev_parse_single(commit_spec.as_str()) | ||
| .expect("commit spec resolves to one object") | ||
| .detach(); | ||
|
|
||
| let mut group = c.benchmark_group("gix-blame::incremental"); | ||
| group.bench_function("without-commit-graph", |b| { | ||
| let mut resource_cache = repo | ||
| .diff_resource_cache_for_tree_diff() | ||
| .expect("tree-diff resource cache can be created"); | ||
|
|
||
| b.iter(|| { | ||
| let mut sink = DiscardSink; | ||
| gix_blame::incremental( | ||
| &repo, | ||
| suspect, | ||
| None, | ||
| &mut resource_cache, | ||
| source_file_path.as_ref(), | ||
| &mut sink, | ||
| incremental_options(), | ||
| ) | ||
| .expect("incremental blame should succeed"); | ||
| }); | ||
| }); | ||
| group.bench_function("with-commit-graph", |b| { | ||
| let mut resource_cache = repo | ||
| .diff_resource_cache_for_tree_diff() | ||
| .expect("tree-diff resource cache can be created"); | ||
| let cache = repo.commit_graph().expect("commit-graph can be loaded from repository"); | ||
| b.iter(|| { | ||
| let mut sink = DiscardSink; | ||
| gix_blame::incremental( | ||
| &repo, | ||
| suspect, | ||
| Some(&cache), | ||
| &mut resource_cache, | ||
| source_file_path.as_ref(), | ||
| &mut sink, | ||
| incremental_options(), | ||
| ) | ||
| .expect("incremental blame should succeed"); | ||
| }); | ||
| }); | ||
| group.finish(); | ||
| } | ||
|
|
||
| fn has_commit_graph_cache(git_dir: &Path) -> bool { | ||
| let info_dir = git_dir.join("objects/info"); | ||
| info_dir.join("commit-graph").is_file() || info_dir.join("commit-graphs").is_dir() | ||
| } | ||
|
Comment on lines
+98
to
+101
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this potentially available as an API on Sebastian Thiel (@Byron) It it is not, should it be?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think so, and judging from how it is used, I think it's fair to just use https://docs.rs/gix/latest/gix/struct.Repository.html#method.commit_graph_if_enabled. While it's doing more than needed here, it's fine in a test. |
||
|
|
||
| fn write_changed_path_commit_graph(worktree_path: &Path) -> std::io::Result<()> { | ||
| let config_status = Command::new("git") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a way to set this value through an API on
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's totally possible, even though I have seen code like this generated a lot as the APIs are cumbersome enough to use at least compared to I'd even be OK with it but would prefer something like |
||
| .args(["config", "commitGraph.changedPathsVersion", "2"]) | ||
| .current_dir(worktree_path) | ||
| .status()?; | ||
| assert!( | ||
| config_status.success(), | ||
| "setting commitGraph.changedPathsVersion should succeed" | ||
| ); | ||
|
|
||
| let write_status = Command::new("git") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think about adding a comment that we have to shell out to |
||
| .args([ | ||
| "commit-graph", | ||
| "write", | ||
| "--no-progress", | ||
| "--reachable", | ||
| "--changed-paths", | ||
| ]) | ||
| .current_dir(worktree_path) | ||
| .status()?; | ||
| assert!( | ||
| write_status.success(), | ||
| "writing changed-path commit-graph should succeed" | ||
| ); | ||
| Ok(()) | ||
| } | ||
|
|
||
| criterion_group!(benches, benchmark_incremental_blame); | ||
| criterion_main!(benches); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sebastian Thiel (@Byron) Is it ok to depend on
gixhere? Since the featureblameis not included, this does not create a circular dependency, but as far as I can tell, currently onlygitoxide-coreandinternal-toolsdepend ongix, so I wanted to flag this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to avoid it as it slows down tests tremendously due to increased build times.
As the
reposeems to be used only to obtain the commit-graph, we should remove this dependency and open it via plumbing directly. Should be easy enough even without re-implementing all Git configuration, so that typical commit-graphs can be opened.It's a benchmark after all.