Re: [PATCH v2 04/13] cache: make hashcmp and hasheq work with larger hashes

From: Duy Nguyen <pclouds@gmail.com>
Date: Oct 16, 2018, 15:44 UTC
Message-ID: <CACsJy8Cyep229CH8OfcVRqxC4soRiiCDhE_FMPhiEzVf-Z9v+Q@mail.gmail.com>
In-Reply-To: <20181015021900.1030041-5-sandals@crustytoothpaste.net>

On Mon, Oct 15, 2018 at 4:21 AM brian m. carlson <sandals@crustytoothpaste.net> wrote:

Show 21 quoted lines

> diff --git a/cache.h b/cache.h
> index a13d14ce0a..0b88c3a344 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1024,16 +1024,12 @@ extern const struct object_id null_oid;
>  static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)
>  {
>         /*
> -        * This is a temporary optimization hack. By asserting the size here,
> -        * we let the compiler know that it's always going to be 20, which lets
> -        * it turn this fixed-size memcmp into a few inline instructions.
> -        *
> -        * This will need to be extended or ripped out when we learn about
> -        * hashes of different sizes.
> +        * Teach the compiler that there are only two possibilities of hash size
> +        * here, so that it can optimize for this case as much as possible.
>          */
> -       if (the_hash_algo->rawsz != 20)
> -               BUG("hash size not yet supported by hashcmp");
> -       return memcmp(sha1, sha2, the_hash_algo->rawsz);
> +       if (the_hash_algo->rawsz == GIT_MAX_RAWSZ)

It's tangent. But performance is probably another good reason to detach the_hash_algo from the_repository so we have one less dereference to do. (the other good reason is these hash operations should work in "no-repo" commands as well, where the_repository does not really make sense).

Show 20 quoted lines

> +               return memcmp(sha1, sha2, GIT_MAX_RAWSZ);
> +       return memcmp(sha1, sha2, GIT_SHA1_RAWSZ);
>  }
>
>  static inline int oidcmp(const struct object_id *oid1, const struct object_id *oid2)
> @@ -1043,7 +1039,13 @@ static inline int oidcmp(const struct object_id *oid1, const struct object_id *o
>
>  static inline int hasheq(const unsigned char *sha1, const unsigned char *sha2)
>  {
> -       return !hashcmp(sha1, sha2);
> +       /*
> +        * We write this here instead of deferring to hashcmp so that the
> +        * compiler can properly inline it and avoid calling memcmp.
> +        */
> +       if (the_hash_algo->rawsz == GIT_MAX_RAWSZ)
> +               return !memcmp(sha1, sha2, GIT_MAX_RAWSZ);
> +       return !memcmp(sha1, sha2, GIT_SHA1_RAWSZ);
>  }
>
>  static inline int oideq(const struct object_id *oid1, const struct object_id *oid2)

-- 
Duy