Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

git_oid_cmp: inline memcmp by hand to optimize #847

Merged
merged 1 commit into from

4 participants

@schu
Collaborator

git.git uses an inlined hashcmp function instead of memcmp, since it
performes much better when comparing hashes (most hashes compared
diverge within the first byte).

Measurements and rationale for the curious reader:
http://thread.gmane.org/gmane.comp.version-control.git/172286

@travisbot

This pull request passes (merged b85363de into 6810ba0).

@schu schu git_oid_cmp: inline memcmp by hand to optimize
git.git uses an inlined hashcmp function instead of memcmp, since it
performes much better when comparing hashes (most hashes compared
diverge within the first byte).

Measurements and rationale for the curious reader:
http://thread.gmane.org/gmane.comp.version-control.git/172286
f6b26e7
@travisbot

This pull request passes (merged f6b26e7 into 6810ba0).

@arrbee
Owner

Wow, I have to say that I was pretty skeptical about this change, but I read through the discussion and tested it out for myself with a couple of different compilers and it seems to actually speed things up a noticeable amount. Looks good!

@arrbee arrbee merged commit 50364dd into libgit2:development
@vmg
Owner

Ewwww, what a terrible patch. I'm glad somebody spotted OID comparisons as a real bottleneck, but the proposed change is disastrous. This is a very braindamaged approach to a very simple optimization problem.

I've rewritten this: pretty much the same code, but with some common sense added on top ("WAIT WHAT IS BRANCHING").

static inline int git_oid_cmp2(const git_oid *a, const git_oid *b)
{
    const uint32_t *sha1 = (uint32_t *)a->id;
    const uint32_t *sha2 = (uint32_t *)b->id;
    int i;

#ifdef __GNUC__
    if (__builtin_expect(*sha1 != *sha2, 1))
#else
    if (*sha1 != *sha2)
#endif
        return *sha1 - *sha2;

    for (i = 1; i < GIT_OID_RAWSZ / sizeof(uint32_t); ++i) {
        if (sha1[i] != sha2[i])
            return sha1[i] - sha2[i];
    }

    return 0;
}

This is (again) 40% faster than this patch, according to this completely unscientific synthetic benchmark. The results certainly agree with the theory, though. Let me run this on core Git and I'll post back.

PS:

@vmg
Owner

80% faster git_oid_eq:

static inline int git_oid_eq(const git_oid *a, const git_oid *b)
{
    const uint32_t *sha1 = (uint32_t *)a->id;
    const uint32_t *sha2 = (uint32_t *)b->id;

    return  !(sha1[0] - sha2[0] ||
            sha1[1] - sha2[1] ||
            sha1[2] - sha2[2] ||
            sha1[3] - sha2[3] ||
            sha1[4] - sha2[4]);
}
@vmg
Owner

Yup, as expected this faster on synthetic benchmarks than on the actual Core Git code... I need to come up with a more scientific way to benchmark Core Git anyway. Measurements there are rather unreliable, and the deviation is too high. Probably disk caching.

@arrbee
Owner

I don't think the existing patch is really "disastrous." The simple loop should make it easier for the compiler to unroll and optimize on its own. (Frankly, I'm somewhat surprised it doesn't just convert the code to a call to memcmp directly.)

Your 32-bit comparison versions will surely be much faster so long as you've got aligned OIDs, but I worry that won't always be true, possibly with very bad effects on non-x86 hardware. I felt like the safe byte-by-byte patch gave a nice little speedup without getting into unpredictable territory.

@vmg
Owner

Yeah, we've been finding this out (check The Library logs): my changes work great under libgit2 (because we use a git_oid struct, which ensures proper alignment), but are proving to be tricky under core Git, which uses a simple byte array.

Disastrous is certainly a grumpy neckbeard overstatement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 29, 2012
  1. @schu

    git_oid_cmp: inline memcmp by hand to optimize

    schu authored
    git.git uses an inlined hashcmp function instead of memcmp, since it
    performes much better when comparing hashes (most hashes compared
    diverge within the first byte).
    
    Measurements and rationale for the curious reader:
    http://thread.gmane.org/gmane.comp.version-control.git/172286
This page is out of date. Refresh to see the latest.
Showing with 26 additions and 12 deletions.
  1. +25 −1 include/git2/oid.h
  2. +0 −5 src/oid.c
  3. +1 −6 src/oidmap.h
View
26 include/git2/oid.h
@@ -136,7 +136,31 @@ GIT_EXTERN(void) git_oid_cpy(git_oid *out, const git_oid *src);
* @param b second oid structure.
* @return <0, 0, >0 if a < b, a == b, a > b.
*/
-GIT_EXTERN(int) git_oid_cmp(const git_oid *a, const git_oid *b);
+GIT_INLINE(int) git_oid_cmp(const git_oid *a, const git_oid *b)
+{
+ const unsigned char *sha1 = a->id;
+ const unsigned char *sha2 = b->id;
+ int i;
+
+ for (i = 0; i < GIT_OID_RAWSZ; i++, sha1++, sha2++) {
+ if (*sha1 != *sha2)
+ return *sha1 - *sha2;
+ }
+
+ return 0;
+}
+
+/**
+ * Compare two oid structures for equality
+ *
+ * @param a first oid structure.
+ * @param b second oid structure.
+ * @return true if equal, false otherwise
+ */
+GIT_INLINE(int) git_oid_equal(const git_oid *a, const git_oid *b)
+{
+ return !git_oid_cmp(a, b);
+}
/**
* Compare the first 'len' hexadecimal characters (packets of 4 bits)
View
5 src/oid.c
@@ -161,11 +161,6 @@ void git_oid_cpy(git_oid *out, const git_oid *src)
memcpy(out->id, src->id, sizeof(out->id));
}
-int git_oid_cmp(const git_oid *a, const git_oid *b)
-{
- return memcmp(a->id, b->id, sizeof(a->id));
-}
-
int git_oid_ncmp(const git_oid *oid_a, const git_oid *oid_b, unsigned int len)
{
const unsigned char *a = oid_a->id;
View
7 src/oidmap.h
@@ -28,13 +28,8 @@ GIT_INLINE(khint_t) hash_git_oid(const git_oid *oid)
return h;
}
-GIT_INLINE(int) hash_git_oid_equal(const git_oid *a, const git_oid *b)
-{
- return (memcmp(a->id, b->id, sizeof(a->id)) == 0);
-}
-
#define GIT__USE_OIDMAP \
- __KHASH_IMPL(oid, static kh_inline, const git_oid *, void *, 1, hash_git_oid, hash_git_oid_equal)
+ __KHASH_IMPL(oid, static kh_inline, const git_oid *, void *, 1, hash_git_oid, git_oid_equal)
#define git_oidmap_alloc() kh_init(oid)
#define git_oidmap_free(h) kh_destroy(oid,h), h = NULL
Something went wrong with that request. Please try again.