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.
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 ?
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.
Simo.
--
Simo Sorce * Red Hat, Inc * New York