Re: [PATCH] git gc: Speed it up by 18% via faster hash comparisons

From: Ingo Molnar <mingo@elte.hu>
Date: Apr 28, 2011, 09:37 UTC
Message-ID: <20110428093703.GB15349@elte.hu>
In-Reply-To: <BANLkTik_2sHZ0OTgQeHpRnpmNsAmT=sAcA@mail.gmail.com>

* Erik Faye-Lund <kusmabite@gmail.com> wrote:

Show 6 quoted lines

> > Secondly, the combined speedup of the cached case with my two patches 
> > appears to be more than 30% on my testbox so it's a very nifty win from two 
> > relatively simple changes.
> 
> That speed-up was on ONE test vector, no? There are a lot of other uses of 
> hash-comparisons in Git, did you measure those?

I picked this hash function because it showed up in the profile (see the profile i posted). There's one other hash that mattered as well in the profile, see the lookup_object() patch i sent yesterday.

Show 12 quoted lines

> > I have added some quick debug code and none of the sha1 pointers (in my 
> > admittedly very limited) testing showed misaligned pointers on 64-bit 
> > systems.
> >
> > On 32-bit systems the pointer might be 32-bit aligned only - the patch 
> > below implements the function 32-bit comparisons.
> 
> That's simply wrong. Unsigned char arrays can and will be unaligned, and this 
> causes exceptions on most architectures (x86 is pretty much the exception 
> here). While some systems for these architectures support unaligned reads 
> from the exception handler, others doesn't. So this patch is pretty much 
> guaranteed to cause a crash in some setups.

If unsigned char arrays are allocated unaligned then that's another bug i suspect that should be fixed. Unaligned access on x86 is not free either - there's cycle penalties.

Alas, i have not seen these sha1 hash buffers being allocated unaligned (in my very limited testing). In which spots are they allocated unaligned?

Thanks,

	Ingo