From: Ingo Molnar <mingo@elte.hu>
Date: Thu, 28 Apr 2011 09:11:10 GMT
Subject: Re: [PATCH] git gc: Speed it up by 18% via faster hash comparisons
Message-ID: <20110428091110.GA14431@elte.hu>
In-Reply-To: <BANLkTim7bbFiSsj3PRr-_yM5gh1txYQR5w@mail.gmail.com>


* Dmitry Potapov <dpotapov@gmail.com> wrote:

> 2011/4/28 Ingo Molnar <mingo@elte.hu>:
> > +static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)
> >  {
> > -       return !memcmp(sha1, null_sha1, 20);
> > +       int i;
> > +
> > +       for (i = 0; i < 20; i++, sha1++, sha2++) {
> > +               if (*sha1 != *sha2) {
> 
> At the very least, you may want to put 'likely' in this 'if'
> condition, otherwise the compiler may optimize this loop in
> the same way as with memcmp. So, it may work well now, but
> it may not work much slower with future versions or different
> level of optimization. (AFAIK, -O3 is far more aggressive in
> optimizing of loops).

the main difference is between the string assembly instructions and the loop. 
Modern CPUs will hardly notice this loop being emitted with slight variations 
by the compiler. So i do not share this concern.

> Another thing is that so far this optimization was only with
> GCC, and we do not know whether it helps or harms to compilers.
> So, maybe placing this code under 'ifdef __GNUC__' makes more
> sense than pushing this change on other compilers too.

Well, given that 90%+ of Git development is done with GCC (and probably the 
user share is similarly high as well):

 aldebaran:~/git> git log --pretty=oneline -i --grep gcc | wc -l
 127
 aldebaran:~/git> git log --pretty=oneline -i --grep llvm | wc -l
 1

and given that these two patches speed up 'git gc' by 30%+, i doubt we'd want 
to litter core Git code with #ifdef __GNUC__ uglinesses.

Thanks,

	Ingo