refactor: avoid the GitHelper and instead keep data on the stack #126
Conversation
There was a problem hiding this comment.
I also left some comments to better explain what's happening here.
Something I'd love to ask is if you could make all changes you need directly in this PR (gh pr checkout 126 && changes && git commit -m "better" && git push) - this is usually the most efficient way to get things done, both for you and for me.
Thanks for your consideration
| .strip_prefix(&workdir) | ||
| .expect("git repository encloses iteration"); | ||
| let platform = excludes | ||
| .at_path(rela_path, Some(file_type.is_dir())) |
There was a problem hiding this comment.
This is why this is fast - after all the initialization, only the necessary work is performed here. All the pre-amble is hidden in git2, but that does only mean it does a lot of work for each call. Think about the relative-path requirement, it's the same for git2 and it will canonicalize everything because it has to, which is really expensive as well.
That's also the reason gix won't offer such a nice and convenient API like git2, it's simply too wasteful (or very stateful internally, which comes with its own problems).
There was a problem hiding this comment.
Agree.
Out of the thread, in another project I participate we discover an issue that PROST's performance is even "worse" than go-protobuf, because it makes less assumption/requirement on ownership/lifetime so it has to copy the buffer internally.
| if file_type.is_symlink() { | ||
| debug!("skip symlink: {:?}", path); | ||
| } else if file_type.is_dir() { | ||
| if git_helper.ignored(path, true)? { | ||
| if platform.is_excluded() { |
There was a problem hiding this comment.
This call also performs work, as it now has to search through the initialized stack of .gitignore patterns to find a match.
There was a problem hiding this comment.
I don't get it clear. Isn't this change part of the cascading of
let platform = excludes.at_path(rela_path, Some(file_type.is_dir()))above? In the previous code, it wrote:
let platform = attrs
.at_path(at_path, Some(is_dir))
.context(GixCheckExcludeOpSnafu)?;
Ok(platform.is_excluded())The construction of attrs is different, but the rest is the same.
There was a problem hiding this comment.
Yes, what I tried to say is that the is_excluded() information isn't cached, but it is computed during the call. It was probably unnecessary to point that out (but I found it interesting enough).
Of course. I like this way; although I was told not to do this in another project >_<. It's 11pm here now. Perhaps I will give this a look tomorrow. |
Yeah, you never know. Fortunately authors can untick a checkbox to prevent it, so if it's enabled I usually do it. |
…andoru#123) This enables far-better performance.
tisonkun
changed the title
Signed-off-by: tison <wander4096@gmail.com>
I don't have time to build a bench suite yet. The observed slow process is when I check against a large repo (>~10 thousands files tested), whether it can finish < 1 second. |
I am definitely curious to learn some of the performance numbers, if you get to compare the two on a large repo one day. Besides that, the PR seems ready once your patch is applied. |
Signed-off-by: tison <wander4096@gmail.com>
|
Thanks for your contribution @Byron! I enjoy to collaborate with you. |
|
Likewise! Fast, efficient, constructive and friendly - my pleasure :). |
This enables far-better performance.
Fixes #123
Review Notes
I didn't actually test performance, and don't know how much tests would catch.