On Mon, 2012-01-09 at 13:27 -0500, Simo Sorce wrote:
On Mon, 2012-01-09 at 12:58 -0500, Stephen Gallagher wrote:
> On Tue, 2012-01-03 at 18:00 -0500, Simo Sorce wrote:
> > >From [PATCH 0/0] A shared memory cache to perform better:
> >
> > 0/3: New utility functions/headers
> > These are also quite straightforward but they are useless without the
> > later patches
>
> Nack.
>
> Your implementation doesn't match the original:
>
>
> From MurmurHash3_x86_32():
> h1 = h1*5+0xe6546b64;
>
> From this patch:
> h1 = h1 + 5 + 0xe6546b64;
>
> I can only assume that s/*/+ is not going to yield proper results :)
OUPS, well it will give *different* results, but consistent.
Anyway I'll fix it and send back a fixed patch.
>
> I fail to understand why you use (nblocks * 4) instead of 'len', given
> that nblocks was defined as len/4
Because (nblocks * 4) != len and it is 3 times out 4 < len :-)
Remember nblocks is an integer, so any fractional part is lost, which is
what we want here, we want to process len - (nblocks * 4) remaining
bytes.
Thanks, I was misreading it.
> While I know that it's a fixed value, I'd rather see
sizeof(uint32_t)
> instead of '4' in getblock() (for readability and clarity)
Ok.
> As for the tests, I'd prefer them to guarantee also that the same string
> hashes to the same value twice. Just in case it's not actually
> deterministic :)
I don't see how it can't be, but I guess it could catch bugs where some
uninitialized variable is used by mistake ?
Yeah, it could catch uninitialized variables or memory corruption due to
buffer overrun, off-by-one error, etc.
> The tests there aren't too exhaustive, given that it only
guarantees
> that six pre-chosen strings don't collide.
I used them to find a bug in the 1st implementation :-)
I don't think we really need exhaustive tests here, upstream has a lot
more tests as they are calculating the actual collision rates and
comparing them to other hashes, but we don't need to. We can trust
upstream got the algorithm good enough.
But if you have any important test to suggest I can try to add them.
That was just me noting that they were only marginally useful. I don't
have any great ideas otherwise. You're right about upstream being fine,
as long as we keep track of any changes they make.