Re: [Qemu-devel] [PATCH 06/10] include: add xxhash.h

[Emilio G. Cota]

On Wed, Apr 06, 2016 at 12:39:42 +0100, Alex Bennée wrote:
> 
> Emilio G. Cota <address@hidden> writes:
> 
> > xxhash is a fast, high-quality hashing function. The appended
> > brings in the 32-bit version of it, with the small modification that
> > it assumes the data to be hashed is made of 32-bit chunks; this increases
> > speed slightly for the use-case we care about, i.e. tb-hash.
> >
> > The original algorithm, as well as a 64-bit implementation, can be found at:
> >   https://github.com/Cyan4973/xxHash
> >
> > Signed-off-by: Emilio G. Cota <address@hidden>
> > ---
> >  include/qemu/xxhash.h | 106 
> > ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 106 insertions(+)
> >  create mode 100644 include/qemu/xxhash.h
> >
> > diff --git a/include/qemu/xxhash.h b/include/qemu/xxhash.h
> > new file mode 100644
> > index 0000000..a13a665
> > --- /dev/null
> > +++ b/include/qemu/xxhash.h
> > @@ -0,0 +1,106 @@
> <snip>
> > +
> > +/* u32 hash of @n contiguous chunks of u32's */
> > +static inline uint32_t qemu_xxh32(const uint32_t *p, size_t n, uint32_t 
> > seed)
> > +{
> 
> What is the point of seed here? I looked on the original site to see if
> there was any guidance on tuning seed but couldn't find anything. I
> appreciate the compiler can inline the constant away but perhaps we
> should #define it and drop the parameter if we are not intending to
> modify it?

The seed value would only matter if we needed consistent hashes (we don't).

Any seed value should give similar performance, given xxhash's quality.

I'll set a define for this seed if it is used in more than one place.

> Also it might be helpful to wrap the call to avoid getting the
> boilerplate sizing wrong:
> 
>   #define qemu_xxh32(s) qemu_xxh32_impl((const uint32_t *)s, 
> sizeof(*s)/sizeof(uint32_t), 1)
> 
> Then calls become a little simpler for the user:
> 
>   return qemu_xxh32(&k);
> 
> Do we need to include a compile time check for structures that don't
> neatly divide into uint32_t chunks?

I'll give the original byte-sized function a shot and come back to this
if necessary.

Thanks,

                E.