On 8/22/2018 3:39 AM, Ævar Arnfjörð Bjarmason wrote:
On Wed, Aug 22, 2018 at 8:20 AM Jeff King <peff@xxxxxxxx> wrote:
On Wed, Aug 22, 2018 at 05:36:26AM +0000, brian m. carlson wrote:
On Tue, Aug 21, 2018 at 11:03:44PM -0400, Jeff King wrote:
I don't know if something like this is an improvement or now, but this
seems to at least compile:
diff --git a/cache.h b/cache.h
index 1398b2a4e4..3207f74771 100644
--- a/cache.h
+++ b/cache.h
@@ -1033,7 +1033,13 @@ extern const struct object_id null_oid;
static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)
{
- return memcmp(sha1, sha2, the_hash_algo->rawsz);
+ switch (the_hash_algo->rawsz) {
+ case 20:
+ case 32:
+ return memcmp(sha1, sha2, the_hash_algo->rawsz);
+ default:
+ assert(0);
+ }
I think that would end up with the same slow code, as gcc would rather
call memcmp than expand out the two sets of asm.
I won't have time to sit down and test this out until tomorrow afternoon
at the earliest. If you want to send in something in the mean time,
even if that limits things to just 20 for now, that's fine.
I don't have a good option. The assert() thing works until I add in the
"32" branch, but that's just punting the issue off until you add support
for the new hash.
Hand-rolling our own asm or C is a portability headache, and we need to
change all of the callsites to use a new hasheq().
Hiding it behind a per-hash function is conceptually cleanest, but not
quite as fast. And it also requires hasheq().
So all of the solutions seem non-trivial. Again, I'm starting to wonder
if it's worth chasing this few percent.
Did you try __builtin_expect? It's a GCC builtin for these sorts of
situations, and sometimes helps:
https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html
I.e. you'd tell GCC we expect to have the 20 there with:
if (__builtin_expect(the_hash_algo->rawsz == 20, 1)) { ... }
The perl codebase has LIKELY() and UNLIKELY() macros for this which if
the feature isn't available fall back on just plain C code:
https://github.com/Perl/perl5/blob/v5.27.7/perl.h#L3335-L3344
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?
