git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 05/21] hash: convert `oidcmp()` and `oideq()` to compare whole hash

From
Patrick Steinhardt <ps@pks.im>
Date
Jun 11, 2024, 11:57 UTC
Message-ID
<e2a0f2125d4a4af84a71bab9f07050d37c4a40a4.1718106285.git.ps@pks.im>
In-Reply-To
<cover.1718106284.git.ps@pks.im>

With the preceding commit, the hash array of object IDs is now fully zero-padded even when the hash algorithm's output is smaller than the array length. With that, we can now adapt both `oidcmp()` and `oideq()` to unconditionally memcmp(3P) the whole array instead of depending on the hash size.

While it may feel inefficient to compare unused bytes for e.g. SHA-1, in practice the compiler should now be able to produce code that is better optimized both because we have no branch anymore, but also because the size to compare is now known at compile time. Goldbolt spits out the following assembly on an x86_64 platform with GCC 14.1 for the old and new implementations of `oidcmp()`:

    oidcmp_old:
            movsx   rax, DWORD PTR [rdi+32]
            test    eax, eax
            jne     .L2
            mov     rax, QWORD PTR the_repository[rip]
            cmp     QWORD PTR [rax+16], 32
            je      .L6
    .L4:
            mov     edx, 20
            jmp     memcmp
    .L2:
            lea     rdx, [rax+rax*2]
            lea     rax, [rax+rdx*4]
            lea     rax, hash_algos[0+rax*8]
            cmp     QWORD PTR [rax+16], 32
            jne     .L4
    .L6:
            mov     edx, 32
            jmp     memcmp
    oidcmp_new:
            mov     edx, 32
            jmp     memcmp

The new implementation gets ridi of all the branches and effectively only ends setting up `edx` for `memcmp()` and then calling it.

And for `oideq()`:
    oideq_old:
            movsx   rcx, DWORD PTR [rdi+32]
            mov     rax, rdi
            mov     rdx, rsi
            test    ecx, ecx
            jne     .L2
            mov     rcx, QWORD PTR the_repository[rip]
            cmp     QWORD PTR [rcx+16], 32
            mov     rcx, QWORD PTR [rax]
            je      .L12
    .L4:
            mov     rsi, QWORD PTR [rax+8]
            xor     rcx, QWORD PTR [rdx]
            xor     rsi, QWORD PTR [rdx+8]
            or      rcx, rsi
            je      .L13
    .L8:
            mov     eax, 1
            test    eax, eax
            sete    al
            movzx   eax, al
            ret
    .L2:
            lea     rsi, [rcx+rcx*2]
            lea     rcx, [rcx+rsi*4]
            lea     rcx, hash_algos[0+rcx*8]
            cmp     QWORD PTR [rcx+16], 32
            mov     rcx, QWORD PTR [rax]
            jne     .L4
    .L12:
            mov     rsi, QWORD PTR [rax+8]
            xor     rcx, QWORD PTR [rdx]
            xor     rsi, QWORD PTR [rdx+8]
            or      rcx, rsi
            jne     .L8
            mov     rcx, QWORD PTR [rax+16]
            mov     rax, QWORD PTR [rax+24]
            xor     rcx, QWORD PTR [rdx+16]
            xor     rax, QWORD PTR [rdx+24]
            or      rcx, rax
            jne     .L8
            xor     eax, eax
    .L14:
            test    eax, eax
            sete    al
            movzx   eax, al
            ret
    .L13:
            mov     edi, DWORD PTR [rdx+16]
            cmp     DWORD PTR [rax+16], edi
            jne     .L8
            xor     eax, eax
            jmp     .L14
    oideq_new:
            mov     rax, QWORD PTR [rdi]
            mov     rdx, QWORD PTR [rdi+8]
            xor     rax, QWORD PTR [rsi]
            xor     rdx, QWORD PTR [rsi+8]
            or      rax, rdx
            je      .L5
    .L2:
            mov     eax, 1
            xor     eax, 1
            ret
    .L5:
            mov     rax, QWORD PTR [rdi+16]
            mov     rdx, QWORD PTR [rdi+24]
            xor     rax, QWORD PTR [rsi+16]
            xor     rdx, QWORD PTR [rsi+24]
            or      rax, rdx
            jne     .L2
            xor     eax, eax
            xor     eax, 1
            ret

Interestingly, the compiler decides to split the comparisons into two so that it first compares the lower half of the object ID for equality and then the upper half. If the first check shows a difference, then we wouldn't even end up comparing the second half.

In both cases, the new generated code is significantly shorter and has way less branches. While I didn't benchmark the change, I'd be surprised if the new code was slower.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 hash-ll.h | 10 ++++++++++
 hash.h    | 20 --------------------
 2 files changed, 10 insertions(+), 20 deletions(-)
diff --git a/hash-ll.h b/hash-ll.h
index b72f84f4ae..b04fe12aef 100644
--- a/hash-ll.h
+++ b/hash-ll.h
@@ -278,6 +278,16 @@ static inline void hashclr(unsigned char *hash, const struct git_hash_algo *algo
 	memset(hash, 0, algop->rawsz);
 }
 
+static inline int oidcmp(const struct object_id *oid1, const struct object_id *oid2)
+{
+	return memcmp(oid1->hash, oid2->hash, GIT_MAX_RAWSZ);
+}
+
+static inline int oideq(const struct object_id *oid1, const struct object_id *oid2)
+{
+	return !memcmp(oid1->hash, oid2->hash, GIT_MAX_RAWSZ);
+}
+
 static inline void oidcpy(struct object_id *dst, const struct object_id *src)
 {
 	memcpy(dst->hash, src->hash, GIT_MAX_RAWSZ);
diff --git a/hash.h b/hash.h
index e43e3d8b5a..ddc2e5ca47 100644
--- a/hash.h
+++ b/hash.h
@@ -6,26 +6,6 @@
 
 #define the_hash_algo the_repository->hash_algo
 
-static inline int oidcmp(const struct object_id *oid1, const struct object_id *oid2)
-{
-	const struct git_hash_algo *algop;
-	if (!oid1->algo)
-		algop = the_hash_algo;
-	else
-		algop = &hash_algos[oid1->algo];
-	return hashcmp(oid1->hash, oid2->hash, algop);
-}
-
-static inline int oideq(const struct object_id *oid1, const struct object_id *oid2)
-{
-	const struct git_hash_algo *algop;
-	if (!oid1->algo)
-		algop = the_hash_algo;
-	else
-		algop = &hash_algos[oid1->algo];
-	return hasheq(oid1->hash, oid2->hash, algop);
-}
-
 static inline int is_null_oid(const struct object_id *oid)
 {
 	return oideq(oid, null_oid());
-- 
2.45.2.436.gcd77e87115.dirty
Previous: Patrick SteinhardtNext: Patrick Steinhardt
Message 6 of 94 in “Introduce `USE_THE_REPOSITORY_VARIABLE` macro”
  1. 00/21 Introduce `USE_THE_REPOSITORY_VARIABLE` macroPatrick Steinhardt, Jun 11, 2024
  2. 01/21 hash: drop (mostly) unused `is_empty_{blob,tree}_sha1()` functionsPatrick Steinhardt, Jun 11, 2024
  3. 02/21 hash: require hash algorithm in `hasheq()`, `hashcmp()` and `hashclr()`Patrick Steinhardt, Jun 11, 2024
  4. 03/21 hash: require hash algorithm in `oidread()` and `oidclr()`Patrick Steinhardt, Jun 11, 2024
  5. 04/21 global: ensure that object IDs are always paddedPatrick Steinhardt, Jun 11, 2024
  6. 05/21 hash: convert `oidcmp()` and `oideq()` to compare whole hashPatrick Steinhardt, Jun 11, 2024
  7. 06/21 hash: make `is_null_oid()` independent of `the_repository`Patrick Steinhardt, Jun 11, 2024
  8. 07/21 hash: require hash algorithm in `is_empty_{blob,tree}_oid()`Patrick Steinhardt, Jun 11, 2024
  9. 08/21 hash: require hash algorithm in `empty_tree_oid_hex()`Patrick Steinhardt, Jun 11, 2024
  10. 09/21 global: introduce `USE_THE_REPOSITORY_VARIABLE` macroPatrick Steinhardt, Jun 11, 2024
  11. 10/21 refs: avoid include cycle with "repository.h"Patrick Steinhardt, Jun 11, 2024
  12. 11/21 hash-ll: merge with "hash.h"Patrick Steinhardt, Jun 11, 2024
  13. 12/21 http-fetch: don't crash when parsing packfile without a repoPatrick Steinhardt, Jun 11, 2024
  14. 13/21 oidset: pass hash algorithm when parsing filePatrick Steinhardt, Jun 11, 2024
  15. 14/21 protocol-caps: use hash algorithm from passed-in repositoryPatrick Steinhardt, Jun 11, 2024
  16. 15/21 replace-object: use hash algorithm from passed-in repositoryPatrick Steinhardt, Jun 11, 2024
  17. 16/21 compat/fsmonitor: remove dependency on `the_repository` in Darwin IPCPatrick Steinhardt, Jun 11, 2024
  18. brian m. carlsonJun 11, 2024
  19. Patrick SteinhardtJun 12, 2024
  20. 17/21 t/helper: use correct object hash in partial-clone helperPatrick Steinhardt, Jun 11, 2024
  21. 18/21 t/helper: fix segfault in "oid-array" command without repositoryPatrick Steinhardt, Jun 11, 2024
  22. Ghanshyam ThakkarJun 11, 2024
  23. 19/21 t/helper: remove dependency on `the_repository` in "oidtree"Patrick Steinhardt, Jun 11, 2024
  24. Ghanshyam ThakkarJun 11, 2024
  25. Patrick SteinhardtJun 12, 2024
  26. brian m. carlsonJun 11, 2024
  27. Patrick SteinhardtJun 12, 2024
  28. 20/21 t/helper: remove dependency on `the_repository` in "proc-receive"Patrick Steinhardt, Jun 11, 2024
  29. 21/21 hex: guard declarations with `USE_THE_REPOSITORY_VARIABLE`Patrick Steinhardt, Jun 11, 2024
  30. brian m. carlsonJun 11, 2024
  31. Patrick SteinhardtJun 12, 2024
  32. Ghanshyam ThakkarJun 12, 2024
  33. 00/20 Introduce `USE_THE_REPOSITORY_VARIABLE` macroPatrick Steinhardt, Jun 13, 2024
  34. 01/20 hash: drop (mostly) unused `is_empty_{blob,tree}_sha1()` functionsPatrick Steinhardt, Jun 13, 2024
  35. 02/20 hash: require hash algorithm in `hasheq()`, `hashcmp()` and `hashclr()`Patrick Steinhardt, Jun 13, 2024
  36. 03/20 hash: require hash algorithm in `oidread()` and `oidclr()`Patrick Steinhardt, Jun 13, 2024
  37. 04/20 global: ensure that object IDs are always paddedPatrick Steinhardt, Jun 13, 2024
  38. 05/20 hash: convert `oidcmp()` and `oideq()` to compare whole hashPatrick Steinhardt, Jun 13, 2024
  39. 06/20 hash: make `is_null_oid()` independent of `the_repository`Patrick Steinhardt, Jun 13, 2024
  40. 07/20 hash: require hash algorithm in `is_empty_{blob,tree}_oid()`Patrick Steinhardt, Jun 13, 2024
  41. 08/20 hash: require hash algorithm in `empty_tree_oid_hex()`Patrick Steinhardt, Jun 13, 2024
  42. Phillip WoodJun 13, 2024
  43. Junio C HamanoJun 13, 2024
  44. Patrick SteinhardtJun 14, 2024
  45. 09/20 global: introduce `USE_THE_REPOSITORY_VARIABLE` macroPatrick Steinhardt, Jun 13, 2024
  46. 10/20 refs: avoid include cycle with "repository.h"Patrick Steinhardt, Jun 13, 2024
  47. 11/20 hash-ll: merge with "hash.h"Patrick Steinhardt, Jun 13, 2024
  48. 12/20 http-fetch: don't crash when parsing packfile without a repoPatrick Steinhardt, Jun 13, 2024
  49. 13/20 oidset: pass hash algorithm when parsing filePatrick Steinhardt, Jun 13, 2024
  50. 14/20 protocol-caps: use hash algorithm from passed-in repositoryPatrick Steinhardt, Jun 13, 2024
  51. 15/20 replace-object: use hash algorithm from passed-in repositoryPatrick Steinhardt, Jun 13, 2024
  52. 16/20 compat/fsmonitor: fix socket path in networked SHA256 reposPatrick Steinhardt, Jun 13, 2024
  53. 17/20 t/helper: use correct object hash in partial-clone helperPatrick Steinhardt, Jun 13, 2024
  54. 18/20 t/helper: fix segfault in "oid-array" command without repositoryPatrick Steinhardt, Jun 13, 2024
  55. 19/20 t/helper: remove dependency on `the_repository` in "proc-receive"Patrick Steinhardt, Jun 13, 2024
  56. 20/20 hex: guard declarations with `USE_THE_REPOSITORY_VARIABLE`Patrick Steinhardt, Jun 13, 2024
  57. Junio C HamanoJun 13, 2024
  58. Junio C HamanoJun 13, 2024
  59. Ramsay JonesJun 13, 2024
  60. Junio C HamanoJun 13, 2024
  61. Ramsay JonesJun 13, 2024
  62. Junio C HamanoJun 14, 2024
  63. Patrick SteinhardtJun 14, 2024
  64. Junio C HamanoJun 14, 2024
  65. 00/20 Introduce `USE_THE_REPOSITORY_VARIABLE` macroPatrick Steinhardt, Jun 14, 2024
  66. 01/20 hash: drop (mostly) unused `is_empty_{blob,tree}_sha1()` functionsPatrick Steinhardt, Jun 14, 2024
  67. 02/20 hash: require hash algorithm in `hasheq()`, `hashcmp()` and `hashclr()`Patrick Steinhardt, Jun 14, 2024
  68. 03/20 hash: require hash algorithm in `oidread()` and `oidclr()`Patrick Steinhardt, Jun 14, 2024
  69. 04/20 global: ensure that object IDs are always paddedPatrick Steinhardt, Jun 14, 2024
  70. 05/20 hash: convert `oidcmp()` and `oideq()` to compare whole hashPatrick Steinhardt, Jun 14, 2024
  71. Karthik NayakJun 17, 2024
  72. 06/20 hash: make `is_null_oid()` independent of `the_repository`Patrick Steinhardt, Jun 14, 2024
  73. 07/20 hash: require hash algorithm in `is_empty_{blob,tree}_oid()`Patrick Steinhardt, Jun 14, 2024
  74. 08/20 hash: require hash algorithm in `empty_tree_oid_hex()`Patrick Steinhardt, Jun 14, 2024
  75. 09/20 global: introduce `USE_THE_REPOSITORY_VARIABLE` macroPatrick Steinhardt, Jun 14, 2024
  76. Karthik NayakJun 17, 2024
  77. Patrick SteinhardtJun 18, 2024
  78. Karthik NayakJun 18, 2024
  79. Junio C HamanoJun 18, 2024
  80. Junio C HamanoJun 18, 2024
  81. Karthik NayakJun 18, 2024
  82. 10/20 refs: avoid include cycle with "repository.h"Patrick Steinhardt, Jun 14, 2024
  83. 11/20 hash-ll: merge with "hash.h"Patrick Steinhardt, Jun 14, 2024
  84. 12/20 http-fetch: don't crash when parsing packfile without a repoPatrick Steinhardt, Jun 14, 2024
  85. 13/20 oidset: pass hash algorithm when parsing filePatrick Steinhardt, Jun 14, 2024
  86. 14/20 protocol-caps: use hash algorithm from passed-in repositoryPatrick Steinhardt, Jun 14, 2024
  87. 15/20 replace-object: use hash algorithm from passed-in repositoryPatrick Steinhardt, Jun 14, 2024
  88. 16/20 compat/fsmonitor: fix socket path in networked SHA256 reposPatrick Steinhardt, Jun 14, 2024
  89. 17/20 t/helper: use correct object hash in partial-clone helperPatrick Steinhardt, Jun 14, 2024
  90. 18/20 t/helper: fix segfault in "oid-array" command without repositoryPatrick Steinhardt, Jun 14, 2024
  91. 19/20 t/helper: remove dependency on `the_repository` in "proc-receive"Patrick Steinhardt, Jun 14, 2024
  92. 20/20 hex: guard declarations with `USE_THE_REPOSITORY_VARIABLE`Patrick Steinhardt, Jun 14, 2024
  93. Phillip WoodJun 18, 2024
  94. Karthik NayakJun 18, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.