Re: [ANNOUNCE] Git v2.19.0-rc0

On 8/22/2018 12:26 PM, Jeff King wrote:

On Wed, Aug 22, 2018 at 06:14:24PM +0200, Duy Nguyen wrote:

quoted

On Wed, Aug 22, 2018 at 6:08 PM Duy Nguyen [off-list ref] wrote:

quoted

On Wed, Aug 22, 2018 at 6:03 PM Jeff King [off-list ref] wrote:

quoted

On Wed, Aug 22, 2018 at 07:14:42AM -0400, Derrick Stolee wrote:

quoted

The other thing I was going to recommend (and I'll try to test this out
myself later) is to see if 'the_hash_algo->rawsz' is being treated as a
volatile variable, since it is being referenced through a pointer. Perhaps
storing the value locally and then casing on it would help?

I tried various sprinkling of "const" around the declarations to make it
clear that the values wouldn't change once we saw them. But I couldn't
detect any difference. At most I think that would let us hoist the "if"
out of the loop, but gcc still seems unwilling to expand the memcmp when
there are other branches.

I think if that's the thing we want to have happen, we really do need to
just write it out on that branch rather than saying "memcmp".

This reminds me of an old discussion about memcpy() vs doing explicit
compare loop with lots of performance measurements..

Ah found it. Not sure if it is still relevant in light of multiple hash support

https://public-inbox.org/git/20110427225114.GA16765@elte.hu/

Yes, that was what I meant. We actually did switch to that hand-rolled
loop, but later we went back to memcmp in 0b006014c8 (hashcmp: use
memcmp instead of open-coded loop, 2017-08-09).

Looking at that commit, I'm surprised the old logic was just a for loop, instead of a word-based approach, such as the following:

diff --git a/cache.h b/cache.h
index b1fd3d58ab..5e5819ad49 100644
--- a/cache.h
+++ b/cache.h

@@ -1021,9 +1021,41 @@ extern int find_unique_abbrev_r(char *hex, const 

struct object_id *oid, int len)
  extern const unsigned char null_sha1[GIT_MAX_RAWSZ];
  extern const struct object_id null_oid;

+static inline int word_cmp_32(uint32_t a, uint32_t b)
+{
+       return memcmp(&a, &b, sizeof(uint32_t));
+}
+
+static inline int word_cmp_64(uint64_t a, uint64_t b)
+{
+       return memcmp(&a, &b, sizeof(uint64_t));
+}
+
+struct object_id_20 {
+       uint64_t data0;
+       uint64_t data1;
+       uint32_t data2;
+};
+
  static inline int hashcmp(const unsigned char *sha1, const unsigned 
char *sha2)
  {
-       return memcmp(sha1, sha2, the_hash_algo->rawsz);
+       if (the_hash_algo->rawsz == 20) {
+               struct object_id_20 *obj1 = (struct object_id_20 *)sha1;
+               struct object_id_20 *obj2 = (struct object_id_20 *)sha2;
+
+               if (obj1->data0 == obj2->data0) {
+                       if (obj1->data1 == obj2->data1) {
+                               if (obj1->data2 == obj2->data2) {
+                                       return 0;
+                               }
+                               return word_cmp_32(obj1->data2, 
obj2->data2);
+                       }
+                       return word_cmp_64(obj1->data1, obj2->data1);
+               }
+               return word_cmp_64(obj1->data0, obj2->data0);
+       }
+
+       assert(0);
  }

  static inline int oidcmp(const struct object_id *oid1, const struct 
object_id *oid2)