Dan McGee [off-list ref] writes:
quoted
dmcgee@galway ~/projects/linux-2.6 (master)
$ time ../git/git-log -- zzzzz_not_exist > /dev/null
real 0m0.945s
user 0m0.857s
sys 0m0.083s
quoted
There is nothing wrong in the patch per-se, but I really wish we didn't
have to do this; it feels like the compiler should be helping us in this
case.
If I resurrect this with an updated commit message reflecting concerns
raised, can it be merged? Given that it is a noticeable performance
boost on real-life repositories and I can show it has little (<1%) to
no impact on most repos, it is a definite win.
I do not see anything wrong in this particular patch per-se, but I really
wish we didn't have to do this.
Please include a few lines of benchmarking result in the updated commit
log message as well if you are rerolling this patch, perhaps like I did
in:
http://thread.gmane.org/gmane.comp.version-control.git/179926/focus=180361
i.e., before and after comparison.
Thanks.
Signed-off-by: Dan McGee <redacted>
---
tree-walk.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/tree-walk.c b/tree-walk.c
index 33f749e..dbcd94a 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -522,7 +522,7 @@ static int match_entry(const struct name_entry *entry, int pathlen,
return 0;
}
-static int match_dir_prefix(const char *base, int baselen,
+static int match_dir_prefix(const char *base,
const char *match, int matchlen)
{
if (strncmp(base, match, matchlen))@@ -579,7 +579,7 @@ int tree_entry_interesting(const struct name_entry *entry,
if (baselen >= matchlen) {
/* If it doesn't match, move along... */
- if (!match_dir_prefix(base_str, baselen, match, matchlen))
+ if (!match_dir_prefix(base_str, match, matchlen))
goto match_wildcards;
if (!ps->recursive || ps->max_depth == -1)--
1.7.6.1
In the case of a wide breadth top-level tree (~2400 entries, all trees
in this case), we can see a noticeable cost in the profiler calling
strncmp() here. Most of the time we are at the base level of the
repository, so base is "" and baselen == 0, which means we will always
test true. Break out this one tiny case so we can short circuit the
strncmp() call.
Test cases are as follows. packages.git is the Arch Linux git-svn clone
of the packages repository which has the characteristics above.
Commands:
[1] packages.git, /usr/bin/time git log >/dev/null
[2] packages.git, /usr/bin/time git log -- autogen/trunk pacman/trunk wget/trunk >/dev/null
[3] linux.git, /usr/bin/time git log >/dev/null
[4] linux.git, /usr/bin/time git log -- drivers/ata drivers/uio tools >/dev/null
Results:
before after %faster
[1] 2.56 2.55 0.4%
[2] 51.82 48.66 6.5%
[3] 5.58 5.61 -0.5%
[4] 1.55 1.51 0.2%
The takeaway here is this doesn't matter in many operations, but it does
for a certain style of repository and operation where it nets a 6.5%
measured improvement. The other changes are likely not significant by
reasonable statistics methods.
Note: the measured improvement when originally submitted was ~11% (43 to
38 secs) for operation [2]. At the time, the repository had 117220
commits; it now has 137537 commits.
Signed-off-by: Dan McGee <redacted>
---
tree-walk.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/tree-walk.c b/tree-walk.c
index dbcd94a..e401f07 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -591,8 +591,8 @@ int tree_entry_interesting(const struct name_entry *entry,
ps->max_depth);
}
- /* Does the base match? */
- if (!strncmp(base_str, match, baselen)) {
+ /* Either there must be no base, or the base must match. */
+ if (baselen == 0 || !strncmp(base_str, match, baselen)) {
if (match_entry(entry, pathlen,
match + baselen, matchlen - baselen,
&never_interesting))--
1.7.6.1