[PATCH] sha1_file: avoid comparison if no packed hash matches the first byte

[PATCH] sha1_file: avoid comparison if no packed hash matches the first byte

[René Scharfe]

find_pack_entry_one() uses the fan-out table of pack indexes to find out
which entries match the first byte of the searched hash and does a
binary search on this subset of the main index table.

If there are no matching entries then lo and hi will have the same
value.  The binary search still starts and compares the hash of the
following entry (which has a non-matching first byte, so won't cause any
trouble), or whatever comes after the sorted list of entries.

The probability of that stray comparison matching by mistake is low, but
let's not take any chances and check when entering the binary search
loop if we're actually done already.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 sha1_file.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index b60ae15f70..11ee69a99d 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2799,7 +2799,7 @@ off_t find_pack_entry_one(const unsigned char *sha1,
 		return nth_packed_object_offset(p, pos);
 	}
 
-	do {
+	while (lo < hi) {
 		unsigned mi = (lo + hi) / 2;
 		int cmp = hashcmp(index + mi * stride, sha1);
 
@@ -2812,7 +2812,7 @@ off_t find_pack_entry_one(const unsigned char *sha1,
 			hi = mi;
 		else
 			lo = mi+1;
-	} while (lo < hi);
+	}
 	return 0;
 }
 
-- 
2.14.0

Re: [PATCH] sha1_file: avoid comparison if no packed hash matches the first byte

[Jonathan Nieder]

René Scharfe wrote:

> find_pack_entry_one() uses the fan-out table of pack indexes to find out
> which entries match the first byte of the searched hash and does a
> binary search on this subset of the main index table.
>
> If there are no matching entries then lo and hi will have the same
> value.  The binary search still starts and compares the hash of the
> following entry (which has a non-matching first byte, so won't cause any
> trouble), or whatever comes after the sorted list of entries.
>
> The probability of that stray comparison matching by mistake is low, but
> let's not take any chances and check when entering the binary search
> loop if we're actually done already.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  sha1_file.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Thanks for a clear explanation.

Sanity checking: is this correct in the sha1[0] == 0 case?  In that
case, we have lo = 0, hi = the 0th offset from the fanout table.  The
offsets in the fanout table are defined as "the number of objects in
the corresponding pack, the first byte of whose object name is less
than or equal to N."  So hi == lo would mean there are no objects with
id starting with 0, as hoped.

Or in other words, the [lo, hi) interval we're trying to search is
indeed a half-open interval.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH] sha1_file: avoid comparison if no packed hash matches the first byte

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> find_pack_entry_one() uses the fan-out table of pack indexes to find out
> which entries match the first byte of the searched hash and does a
> binary search on this subset of the main index table.
>
> If there are no matching entries then lo and hi will have the same
> value.  The binary search still starts and compares the hash of the
> following entry (which has a non-matching first byte, so won't cause any
> trouble), or whatever comes after the sorted list of entries.
>
> The probability of that stray comparison matching by mistake is low, but
> let's not take any chances and check when entering the binary search
> loop if we're actually done already.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  sha1_file.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index b60ae15f70..11ee69a99d 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2799,7 +2799,7 @@ off_t find_pack_entry_one(const unsigned char *sha1,
>  		return nth_packed_object_offset(p, pos);
>  	}
>  
> -	do {
> +	while (lo < hi) {
>  		unsigned mi = (lo + hi) / 2;
>  		int cmp = hashcmp(index + mi * stride, sha1);
>  
> @@ -2812,7 +2812,7 @@ off_t find_pack_entry_one(const unsigned char *sha1,
>  			hi = mi;
>  		else
>  			lo = mi+1;
> -	} while (lo < hi);
> +	}
>  	return 0;
>  }

Interesting.  I see that we still have the conditional code to call
out to sha1-lookup.c::sha1_entry_pos().  Do we need a similar change
over there, I wonder?  Alternatively, as we have had the experimental
sha1-lookup.c::sha1_entry_pos() long enough without anybody using it,
perhaps we should write it off as a failed experiment and retire it?

Re: [PATCH] sha1_file: avoid comparison if no packed hash matches the first byte

[Jeff King]

On Tue, Aug 08, 2017 at 03:43:13PM -0700, Junio C Hamano wrote:

> > @@ -2812,7 +2812,7 @@ off_t find_pack_entry_one(const unsigned char *sha1,
> >  			hi = mi;
> >  		else
> >  			lo = mi+1;
> > -	} while (lo < hi);
> > +	}
> >  	return 0;
> >  }
> 
> Interesting.  I see that we still have the conditional code to call
> out to sha1-lookup.c::sha1_entry_pos().  Do we need a similar change
> over there, I wonder?  Alternatively, as we have had the experimental
> sha1-lookup.c::sha1_entry_pos() long enough without anybody using it,
> perhaps we should write it off as a failed experiment and retire it?

There is also sha1_pos(), which seems to have the same problem (and is
used in several places).

-Peff

Re: [PATCH] sha1_file: avoid comparison if no packed hash matches the first byte

[Jeff King]

On Tue, Aug 08, 2017 at 06:52:31PM -0400, Jeff King wrote:

> > Interesting.  I see that we still have the conditional code to call
> > out to sha1-lookup.c::sha1_entry_pos().  Do we need a similar change
> > over there, I wonder?  Alternatively, as we have had the experimental
> > sha1-lookup.c::sha1_entry_pos() long enough without anybody using it,
> > perhaps we should write it off as a failed experiment and retire it?
> 
> There is also sha1_pos(), which seems to have the same problem (and is
> used in several places).

Actually, I take it back. The problem happens when we enter the loop
with no entries to look at. But both sha1_pos() and sha1_entry_pos()
return early before hitting their do-while loops in that case.

-Peff

Re: [PATCH] sha1_file: avoid comparison if no packed hash matches the first byte

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Aug 08, 2017 at 06:52:31PM -0400, Jeff King wrote:
>
>> > Interesting.  I see that we still have the conditional code to call
>> > out to sha1-lookup.c::sha1_entry_pos().  Do we need a similar change
>> > over there, I wonder?  Alternatively, as we have had the experimental
>> > sha1-lookup.c::sha1_entry_pos() long enough without anybody using it,
>> > perhaps we should write it off as a failed experiment and retire it?
>> 
>> There is also sha1_pos(), which seems to have the same problem (and is
>> used in several places).
>
> Actually, I take it back. The problem happens when we enter the loop
> with no entries to look at. But both sha1_pos() and sha1_entry_pos()
> return early before hitting their do-while loops in that case.

Ah, I was not looking at that part of the code.  Thanks.

I still wonder if we want to retire that conditional invocation of
sha1_entry_pos(), though.

Re: [PATCH] sha1_file: avoid comparison if no packed hash matches the first byte

[Jeff King]

On Tue, Aug 08, 2017 at 10:36:33PM -0700, Junio C Hamano wrote:

> > Actually, I take it back. The problem happens when we enter the loop
> > with no entries to look at. But both sha1_pos() and sha1_entry_pos()
> > return early before hitting their do-while loops in that case.
> 
> Ah, I was not looking at that part of the code.  Thanks.
> 
> I still wonder if we want to retire that conditional invocation of
> sha1_entry_pos(), though.

I think so. Digging in the list for it, almost every mention is either
somebody asking "should we scrap this?" or somebody showing benchmarks
in which it is slower than the normal lookup (and then somebody asking
"should we scrap this" :) ).

I just re-ran a simple benchmark and it is indeed slower. I also came
across the hashcmp open-code-versus-memcmp discussion, which shows that
the memcmp in recent glibc is much faster. That has been around long
enough that it's probably worth switching to.

-Peff

Re: [PATCH] sha1_file: avoid comparison if no packed hash matches the first byte

[Jeff King]

On Wed, Aug 09, 2017 at 05:20:05AM -0400, Jeff King wrote:

> > I still wonder if we want to retire that conditional invocation of
> > sha1_entry_pos(), though.
> 
> I think so. Digging in the list for it, almost every mention is either
> somebody asking "should we scrap this?" or somebody showing benchmarks
> in which it is slower than the normal lookup (and then somebody asking
> "should we scrap this" :) ).
> 
> I just re-ran a simple benchmark and it is indeed slower. I also came
> across the hashcmp open-code-versus-memcmp discussion, which shows that
> the memcmp in recent glibc is much faster. That has been around long
> enough that it's probably worth switching to.

So here are two patches (on top of René's since there is otherwise a
minor textual conflict).

  [1/2]: sha1_file: drop experimental GIT_USE_LOOKUP search
  [2/2]: hashcmp: use memcmp instead of open-coded loop

 cache.h                           |   9 +-
 sha1-lookup.c                     | 216 --------------------------------------
 sha1_file.c                       |  11 --
 t/t5308-pack-detect-duplicates.sh |  11 +-
 t/test-lib.sh                     |   1 -
 5 files changed, 2 insertions(+), 246 deletions(-)

-Peff

[PATCH 1/2] sha1_file: drop experimental GIT_USE_LOOKUP search

[Jeff King]

Long ago in 628522ec14 (sha1-lookup: more memory efficient
search in sorted list of SHA-1, 2007-12-29) we added
sha1_entry_pos(), a binary search that uses the uniform
distribution of sha1s to scale the selection of mid-points.
As this was a performance experiment, we tied it to the
GIT_USE_LOOKUP environment variable and never enabled it by
default.

This code was successful in reducing the number of steps in
each search. But the overhead of the scaling ends up making
it slower when the cache is warm. Here are best-of-five
timings for running rev-list on linux.git, which will have
to look up every object:

  $ time git rev-list --objects --all >/dev/null
  real	0m35.357s
  user	0m35.016s
  sys	0m0.340s

  $ time GIT_USE_LOOKUP=1 git rev-list --objects --all >/dev/null
  real	0m37.364s
  user	0m37.045s
  sys	0m0.316s

The USE_LOOKUP version might have more benefit on a cold
cache, as the time to fault in each page would dominate. But
that would be for a single lookup. In practice, most
operations tend to look up many objects, and the whole pack
.idx will end up warm.

It's possible that the code could be better optimized to
compete with a naive binary search for the warm-cache case,
and we could have the best of both worlds. But over the
years nobody has done so, and this is largely dead code that
is rarely run outside of the test suite. Let's drop it in
the name of simplicity.

This lets us remove sha1_entry_pos() entirely, as the .idx
lookup code was the only caller.  Note that sha1-lookup.c
still contains sha1_pos(), which differs from
sha1_entry_pos() in two ways:

  - it has a different interface; it uses a function pointer
    to access sha1 entries rather than a size/offset pair
    describing the table's memory layout

  - it only scales the initial selection of "mi", rather
    than each iteration of the search

We can't get rid of this function, as it's called from
several places. It may be that we could replace it with a
simple binary search, but that's out of scope for this patch
(and would need benchmarking).

Signed-off-by: Jeff King <peff@peff.net>
---
 sha1-lookup.c                     | 216 --------------------------------------
 sha1_file.c                       |  11 --
 t/t5308-pack-detect-duplicates.sh |  11 +-
 t/test-lib.sh                     |   1 -
 4 files changed, 1 insertion(+), 238 deletions(-)

diff --git a/sha1-lookup.c b/sha1-lookup.c
index 5f069214d9..2552b7902c 100644
--- a/sha1-lookup.c
+++ b/sha1-lookup.c
@@ -99,219 +99,3 @@ int sha1_pos(const unsigned char *sha1, void *table, size_t nr,
 	} while (lo < hi);
 	return -lo-1;
 }
-
-/*
- * Conventional binary search loop looks like this:
- *
- *	unsigned lo, hi;
- *      do {
- *              unsigned mi = (lo + hi) / 2;
- *              int cmp = "entry pointed at by mi" minus "target";
- *              if (!cmp)
- *                      return (mi is the wanted one)
- *              if (cmp > 0)
- *                      hi = mi; "mi is larger than target"
- *              else
- *                      lo = mi+1; "mi is smaller than target"
- *      } while (lo < hi);
- *
- * The invariants are:
- *
- * - When entering the loop, lo points at a slot that is never
- *   above the target (it could be at the target), hi points at a
- *   slot that is guaranteed to be above the target (it can never
- *   be at the target).
- *
- * - We find a point 'mi' between lo and hi (mi could be the same
- *   as lo, but never can be as same as hi), and check if it hits
- *   the target.  There are three cases:
- *
- *    - if it is a hit, we are happy.
- *
- *    - if it is strictly higher than the target, we set it to hi,
- *      and repeat the search.
- *
- *    - if it is strictly lower than the target, we update lo to
- *      one slot after it, because we allow lo to be at the target.
- *
- *   If the loop exits, there is no matching entry.
- *
- * When choosing 'mi', we do not have to take the "middle" but
- * anywhere in between lo and hi, as long as lo <= mi < hi is
- * satisfied.  When we somehow know that the distance between the
- * target and lo is much shorter than the target and hi, we could
- * pick mi that is much closer to lo than the midway.
- *
- * Now, we can take advantage of the fact that SHA-1 is a good hash
- * function, and as long as there are enough entries in the table, we
- * can expect uniform distribution.  An entry that begins with for
- * example "deadbeef..." is much likely to appear much later than in
- * the midway of the table.  It can reasonably be expected to be near
- * 87% (222/256) from the top of the table.
- *
- * However, we do not want to pick "mi" too precisely.  If the entry at
- * the 87% in the above example turns out to be higher than the target
- * we are looking for, we would end up narrowing the search space down
- * only by 13%, instead of 50% we would get if we did a simple binary
- * search.  So we would want to hedge our bets by being less aggressive.
- *
- * The table at "table" holds at least "nr" entries of "elem_size"
- * bytes each.  Each entry has the SHA-1 key at "key_offset".  The
- * table is sorted by the SHA-1 key of the entries.  The caller wants
- * to find the entry with "key", and knows that the entry at "lo" is
- * not higher than the entry it is looking for, and that the entry at
- * "hi" is higher than the entry it is looking for.
- */
-int sha1_entry_pos(const void *table,
-		   size_t elem_size,
-		   size_t key_offset,
-		   unsigned lo, unsigned hi, unsigned nr,
-		   const unsigned char *key)
-{
-	const unsigned char *base = table;
-	const unsigned char *hi_key, *lo_key;
-	unsigned ofs_0;
-	static int debug_lookup = -1;
-
-	if (debug_lookup < 0)
-		debug_lookup = !!getenv("GIT_DEBUG_LOOKUP");
-
-	if (!nr || lo >= hi)
-		return -1;
-
-	if (nr == hi)
-		hi_key = NULL;
-	else
-		hi_key = base + elem_size * hi + key_offset;
-	lo_key = base + elem_size * lo + key_offset;
-
-	ofs_0 = 0;
-	do {
-		int cmp;
-		unsigned ofs, mi, range;
-		unsigned lov, hiv, kyv;
-		const unsigned char *mi_key;
-
-		range = hi - lo;
-		if (hi_key) {
-			for (ofs = ofs_0; ofs < 20; ofs++)
-				if (lo_key[ofs] != hi_key[ofs])
-					break;
-			ofs_0 = ofs;
-			/*
-			 * byte 0 thru (ofs-1) are the same between
-			 * lo and hi; ofs is the first byte that is
-			 * different.
-			 *
-			 * If ofs==20, then no bytes are different,
-			 * meaning we have entries with duplicate
-			 * keys. We know that we are in a solid run
-			 * of this entry (because the entries are
-			 * sorted, and our lo and hi are the same,
-			 * there can be nothing but this single key
-			 * in between). So we can stop the search.
-			 * Either one of these entries is it (and
-			 * we do not care which), or we do not have
-			 * it.
-			 *
-			 * Furthermore, we know that one of our
-			 * endpoints must be the edge of the run of
-			 * duplicates. For example, given this
-			 * sequence:
-			 *
-			 *     idx 0 1 2 3 4 5
-			 *     key A C C C C D
-			 *
-			 * If we are searching for "B", we might
-			 * hit the duplicate run at lo=1, hi=3
-			 * (e.g., by first mi=3, then mi=0). But we
-			 * can never have lo > 1, because B < C.
-			 * That is, if our key is less than the
-			 * run, we know that "lo" is the edge, but
-			 * we can say nothing of "hi". Similarly,
-			 * if our key is greater than the run, we
-			 * know that "hi" is the edge, but we can
-			 * say nothing of "lo".
-			 *
-			 * Therefore if we do not find it, we also
-			 * know where it would go if it did exist:
-			 * just on the far side of the edge that we
-			 * know about.
-			 */
-			if (ofs == 20) {
-				mi = lo;
-				mi_key = base + elem_size * mi + key_offset;
-				cmp = memcmp(mi_key, key, 20);
-				if (!cmp)
-					return mi;
-				if (cmp < 0)
-					return -1 - hi;
-				else
-					return -1 - lo;
-			}
-
-			hiv = hi_key[ofs_0];
-			if (ofs_0 < 19)
-				hiv = (hiv << 8) | hi_key[ofs_0+1];
-		} else {
-			hiv = 256;
-			if (ofs_0 < 19)
-				hiv <<= 8;
-		}
-		lov = lo_key[ofs_0];
-		kyv = key[ofs_0];
-		if (ofs_0 < 19) {
-			lov = (lov << 8) | lo_key[ofs_0+1];
-			kyv = (kyv << 8) | key[ofs_0+1];
-		}
-		assert(lov < hiv);
-
-		if (kyv < lov)
-			return -1 - lo;
-		if (hiv < kyv)
-			return -1 - hi;
-
-		/*
-		 * Even if we know the target is much closer to 'hi'
-		 * than 'lo', if we pick too precisely and overshoot
-		 * (e.g. when we know 'mi' is closer to 'hi' than to
-		 * 'lo', pick 'mi' that is higher than the target), we
-		 * end up narrowing the search space by a smaller
-		 * amount (i.e. the distance between 'mi' and 'hi')
-		 * than what we would have (i.e. about half of 'lo'
-		 * and 'hi').  Hedge our bets to pick 'mi' less
-		 * aggressively, i.e. make 'mi' a bit closer to the
-		 * middle than we would otherwise pick.
-		 */
-		kyv = (kyv * 6 + lov + hiv) / 8;
-		if (lov < hiv - 1) {
-			if (kyv == lov)
-				kyv++;
-			else if (kyv == hiv)
-				kyv--;
-		}
-		mi = (range - 1) * (kyv - lov) / (hiv - lov) + lo;
-
-		if (debug_lookup) {
-			printf("lo %u hi %u rg %u mi %u ", lo, hi, range, mi);
-			printf("ofs %u lov %x, hiv %x, kyv %x\n",
-			       ofs_0, lov, hiv, kyv);
-		}
-		if (!(lo <= mi && mi < hi))
-			die("assertion failure lo %u mi %u hi %u %s",
-			    lo, mi, hi, sha1_to_hex(key));
-
-		mi_key = base + elem_size * mi + key_offset;
-		cmp = memcmp(mi_key + ofs_0, key + ofs_0, 20 - ofs_0);
-		if (!cmp)
-			return mi;
-		if (cmp > 0) {
-			hi = mi;
-			hi_key = mi_key;
-		} else {
-			lo = mi + 1;
-			lo_key = mi_key + elem_size;
-		}
-	} while (lo < hi);
-	return -lo-1;
-}
diff --git a/sha1_file.c b/sha1_file.c
index 11ee69a99d..607b34ea53 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2759,7 +2759,6 @@ off_t find_pack_entry_one(const unsigned char *sha1,
 	const uint32_t *level1_ofs = p->index_data;
 	const unsigned char *index = p->index_data;
 	unsigned hi, lo, stride;
-	static int use_lookup = -1;
 	static int debug_lookup = -1;
 
 	if (debug_lookup < 0)
@@ -2789,16 +2788,6 @@ off_t find_pack_entry_one(const unsigned char *sha1,
 		printf("%02x%02x%02x... lo %u hi %u nr %"PRIu32"\n",
 		       sha1[0], sha1[1], sha1[2], lo, hi, p->num_objects);
 
-	if (use_lookup < 0)
-		use_lookup = !!getenv("GIT_USE_LOOKUP");
-	if (use_lookup) {
-		int pos = sha1_entry_pos(index, stride, 0,
-					 lo, hi, p->num_objects, sha1);
-		if (pos < 0)
-			return 0;
-		return nth_packed_object_offset(p, pos);
-	}
-
 	while (lo < hi) {
 		unsigned mi = (lo + hi) / 2;
 		int cmp = hashcmp(index + mi * stride, sha1);
diff --git a/t/t5308-pack-detect-duplicates.sh b/t/t5308-pack-detect-duplicates.sh
index 9c5a8766ab..156ae9e9d3 100755
--- a/t/t5308-pack-detect-duplicates.sh
+++ b/t/t5308-pack-detect-duplicates.sh
@@ -56,20 +56,11 @@ test_expect_success 'create batch-check test vectors' '
 	EOF
 '
 
-test_expect_success 'lookup in duplicated pack (binary search)' '
+test_expect_success 'lookup in duplicated pack' '
 	git cat-file --batch-check <input >actual &&
 	test_cmp expect actual
 '
 
-test_expect_success 'lookup in duplicated pack (GIT_USE_LOOKUP)' '
-	(
-		GIT_USE_LOOKUP=1 &&
-		export GIT_USE_LOOKUP &&
-		git cat-file --batch-check <input >actual
-	) &&
-	test_cmp expect actual
-'
-
 test_expect_success 'index-pack can reject packs with duplicates' '
 	clear_packs &&
 	create_pack dups.pack 2 &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1b6e53f78a..5e3ca30ab3 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -99,7 +99,6 @@ unset VISUAL EMAIL LANGUAGE COLUMNS $("$PERL_PATH" -e '
 	my $ok = join("|", qw(
 		TRACE
 		DEBUG
-		USE_LOOKUP
 		TEST
 		.*_TEST
 		PROVE
-- 
2.14.0.609.gd2d1f7ddf

Re: [PATCH 1/2] sha1_file: drop experimental GIT_USE_LOOKUP search

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This lets us remove sha1_entry_pos() entirely, as the .idx
> lookup code was the only caller.  Note that sha1-lookup.c
> still contains sha1_pos(), which differs from
> sha1_entry_pos() in two ways:
>
>   - it has a different interface; it uses a function pointer
>     to access sha1 entries rather than a size/offset pair
>     describing the table's memory layout
>
>   - it only scales the initial selection of "mi", rather
>     than each iteration of the search
>
> We can't get rid of this function, as it's called from
> several places. It may be that we could replace it with a
> simple binary search, but that's out of scope for this patch
> (and would need benchmarking).

Thanks for reducing the count of binary search functions by one.

I think the "just one round of newton-raphson" in sha1_pos() comes
from [*1*]; I agree that it needs benchmarking before tweaking it.

We may want to tell libgit2 folks about this change, though [*2*].
I think they too are carrying dead code that is only used under CPP
macro GIT_USE_LOOKUP, which they do not seem to define.


[Reference]

*1* https://public-inbox.org/git/7v38vso8kz.fsf@alter.siamese.dyndns.org/
*2* https://github.com/libgit2/libgit2/commit/dd453c4dbf9a1fa38530b1f51e079852736b8f66

Re: [PATCH 1/2] sha1_file: drop experimental GIT_USE_LOOKUP search

[Jeff King]

On Wed, Aug 09, 2017 at 11:12:19AM -0700, Junio C Hamano wrote:

> Thanks for reducing the count of binary search functions by one.
> 
> I think the "just one round of newton-raphson" in sha1_pos() comes
> from [*1*]; I agree that it needs benchmarking before tweaking it.

Actually, it's weirder than that. You mentioned it in that thread, but
the code dates back much further. It was moved to the file in 96beef8,
but that was just a copy from patch-ids.c. The original seems to be from
5d23e133d2 (Refactor patch-id filtering out of git-cherry and
git-format-patch., 2007-04-09), which predates the sha1_entry_pos()
experiment.

I always thought those two sha1-lookup functions came in the opposite
order, but I guess was wrong. Or possibly you are a time traveler.

> We may want to tell libgit2 folks about this change, though [*2*].
> I think they too are carrying dead code that is only used under CPP
> macro GIT_USE_LOOKUP, which they do not seem to define.

Good thinking. It looks like they could use all three of the patches
under discussion. I opened some PRs:

  https://github.com/libgit2/libgit2/pull/4326
  https://github.com/libgit2/libgit2/pull/4327
  https://github.com/libgit2/libgit2/pull/4328

-Peff

[PATCH 2/2] hashcmp: use memcmp instead of open-coded loop

[Jeff King]

In 1a812f3a70 (hashcmp(): inline memcmp() by hand to
optimize, 2011-04-28), it was reported that an open-coded
loop outperformed memcmp() for comparing sha1s.

Discussion[1] a few years later in 2013 showed that this
depends on your libc's version of memcmp(). In particular,
glibc 2.13 optimized their memcmp around 2011. Here are
current timings with glibc 2.24 (best-of-five, on
linux.git):

  [before this patch, open-coded]
  $ time git rev-list --objects --all
  real	0m35.357s
  user	0m35.016s
  sys	0m0.340s

  [after this patch, memcmp]
  real	0m32.930s
  user	0m32.630s
  sys	0m0.300s

Now that we've had 6 years for that version of glibc to
make its way onto people's machines, it's worth revisiting
our benchmarks and switching to memcmp().

It may be that there are other non-glibc systems where
memcmp() isn't as well optimized. But since our single data
point in favor of open-coding was on a now-ancient glibc, we
should probably assume the system memcmp is good unless
proven otherwise. We may end up with a SLOW_MEMCMP Makefile
knob, but we can hold off on that until we actually find
such a system in practice.

[1] https://public-inbox.org/git/20130318073229.GA5551@sigill.intra.peff.net/

Signed-off-by: Jeff King <peff@peff.net>
---
I also wondered if using memcmp() could be a hint to the compiler to use
an intrinsic or some other trick, especially because the "len" here is a
constant. But in a toy function compiled with "gcc -S", it looks like we
do keep the call to memcmp (so the speedup really is glibc, and not some
compiler magic).

 cache.h | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/cache.h b/cache.h
index 71fe092644..46af4ecddb 100644
--- a/cache.h
+++ b/cache.h
@@ -939,14 +939,7 @@ extern const struct object_id null_oid;
 
 static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)
 {
-	int i;
-
-	for (i = 0; i < GIT_SHA1_RAWSZ; i++, sha1++, sha2++) {
-		if (*sha1 != *sha2)
-			return *sha1 - *sha2;
-	}
-
-	return 0;
+	return memcmp(sha1, sha2, GIT_SHA1_RAWSZ);
 }
 
 static inline int oidcmp(const struct object_id *oid1, const struct object_id *oid2)
-- 
2.14.0.609.gd2d1f7ddf

Re: [PATCH 2/2] hashcmp: use memcmp instead of open-coded loop

[René Scharfe]

Am 09.08.2017 um 12:16 schrieb Jeff King:
> In 1a812f3a70 (hashcmp(): inline memcmp() by hand to
> optimize, 2011-04-28), it was reported that an open-coded
> loop outperformed memcmp() for comparing sha1s.
> 
> Discussion[1] a few years later in 2013 showed that this
> depends on your libc's version of memcmp(). In particular,
> glibc 2.13 optimized their memcmp around 2011. Here are
> current timings with glibc 2.24 (best-of-five, on
> linux.git):
> 
>    [before this patch, open-coded]
>    $ time git rev-list --objects --all
>    real	0m35.357s
>    user	0m35.016s
>    sys	0m0.340s
> 
>    [after this patch, memcmp]
>    real	0m32.930s
>    user	0m32.630s
>    sys	0m0.300s

Nice.  And here's the size of the git executable in my build:

         unstripped stripped
  before    8048176  2082416
  after     8006064  2037360

> I also wondered if using memcmp() could be a hint to the compiler to use
> an intrinsic or some other trick, especially because the "len" here is a
> constant. But in a toy function compiled with "gcc -S", it looks like we
> do keep the call to memcmp (so the speedup really is glibc, and not some
> compiler magic).

GCC 7 inlines memcmp() if we only need a binary result:

	https://godbolt.org/g/iZ11Ne

René

Re: [PATCH 2/2] hashcmp: use memcmp instead of open-coded loop

[Jeff King]

On Wed, Aug 09, 2017 at 04:55:43PM +0200, René Scharfe wrote:

> > I also wondered if using memcmp() could be a hint to the compiler to use
> > an intrinsic or some other trick, especially because the "len" here is a
> > constant. But in a toy function compiled with "gcc -S", it looks like we
> > do keep the call to memcmp (so the speedup really is glibc, and not some
> > compiler magic).
> 
> GCC 7 inlines memcmp() if we only need a binary result:
> 
> 	https://godbolt.org/g/iZ11Ne

Cute.  It turns it into a series of 8-byte xors. The original open-coded
loop doesn't end up nearly as optimized with gcc-7.

I suspect many calls in practice are of the binary-result type. So some
of the speedup I saw may have been from compiler improvements and not
libc improvements. Still, I think the general argument is the same,
replacing "if your libc memcmp is fast" with "if your libc/compiler
makes memcmp fast".

-Peff