Cancel tree diffing early when matching path is found #1747
Conversation
There was a problem hiding this comment.
Thanks a lot, that's great!
Who would have thought that such a small change can have such large impact! It's also something I didn't even think off, so I am really glad you did!
Would it make sense to rename stats.trees_diffed to stats.trees_partially_diffed?
I'd keep the name simple as it's typically Debug printed anyway. But it's probably fair to be specific in its doc-string to indicate the diff is usually partial.
gix-blame/src/file/function.rs
Outdated
| // `recorder` cancels the traversal by returning `Cancel` when a change to `file_path` is | ||
| // found. `gix_diff::tree` converts `Cancel` into `Err(...)` which is why we ignore its return | ||
| // value here. I don’t know whether this has the potential to hide bugs. | ||
| let _ = gix_diff::tree(parent_tree_iter, tree_iter, state, &odb, &mut recorder); |
There was a problem hiding this comment.
Indeed, this is where one would have to ignore the Cancelled variant of the error, but fail on all others.
The reason this is an error is… that it's intended to be used for cancellation, which is when one wants this to error out easily.
Here this isn't too useful though.
There was a problem hiding this comment.
Thanks for the context, I’ve adapted this part.
gix-blame/src/file/function.rs
Outdated
| // TODO | ||
| // The name is preliminary and can potentially include more context. Also, this should probably be | ||
| // moved to its own location. | ||
| struct Recorder { |
There was a problem hiding this comment.
I'd probably call it after what it does, like CancelDiffOncePathTouched (or better :)).
The implementation I'd also put inside the function that uses it.
There was a problem hiding this comment.
I’ve renamed and inlined Recorder.
This PR is ready for review, but it still requires a couple of iterations before it can be considered ready to be merged.
It introduces a custom
Recorderthat is mainly a slimmed down version ofgix_diff::tree::Recorder.Recordershortens diffing by cancelling it as soon as a change tointeresting_pathis found. This is based on the assumption that there will be at most one change per file path.The results seem promising. Compared to the existing implementation, I was able to observe a speedup of about 5 % for
gix blame STABILITY.md.For
gix blame Cargo.lock, the speedup was in the range of 25 % to 30 %.I also ran
gix blameon all files ingitoxideto make sure the changed version’s output exactly matched the current version’s output.Open questions
gix_diff::tree’s return value or is this a bad idea?Recordergo and can we find a better name for this struct?stats.trees_diffedtostats.trees_partially_diffed?