Re: [PATCH 01/41] cache: add a function to read an object ID from a buffer

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: May 1, 2018, 23:58 UTC
Message-ID: <20180501235837.GG13217@genre.crustytoothpaste.net>
In-Reply-To: <20180501093603.GA15820@duynguyen.home>

On Tue, May 01, 2018 at 11:36:03AM +0200, Duy Nguyen wrote:

Show 16 quoted lines

> On Mon, Apr 23, 2018 at 11:39:11PM +0000, brian m. carlson wrote:
> > diff --git a/cache.h b/cache.h
> > index bbaf5c349a..4bca177cf3 100644
> > --- a/cache.h
> > +++ b/cache.h
> > @@ -1008,6 +1008,11 @@ static inline void oidclr(struct object_id *oid)
> >  	memset(oid->hash, 0, GIT_MAX_RAWSZ);
> >  }
> >  
> > +static inline void oidread(struct object_id *oid, const unsigned char *hash)
> > +{
> > +	memcpy(oid->hash, hash, the_hash_algo->rawsz);
> 
> If performance is a concern, should we go with GIT_MAX_RAWSZ instead
> of the_hash_algo->rawsz which gives the compiler some more to bypass
> actual memcpy function and generate copy code directly?

I don't think we can do that. If we have both NewHash and SHA-1 compiled in and are using SHA-1, GIT_MAX_RAWSZ will be 32, but we may only have 20 bytes that are valid to read.

> If it is not a performance problem, should we avoid inline and move
> the implementation somewhere?

I would like to make it as fast as possible if we can, especially since hashcpy is inline. If you have concerns about performance, I can add a patch in a future series that does some sort of macro if we're using gcc that does something like the following:

  ({
    int rawsz = the_hash_algo->rawsz;
    if (rawsz != GIT_SHA1_RAWSZ && rawsz != GIT_MAX_RAWSZ)
      __builtin_trap(); /* never reached */
    rawsz;
   })

And then use that instead of the_hash_algo->rawsz in performance sensitive paths. That would mean the compiler would know that it was only one of those two values and it could optimize better.

-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204