Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mmhash128_4(::Integer, ::Unsigned, ::UInt32) is missing (breaks 32bit) #12

Closed
oxinabox opened this issue Jan 26, 2021 · 3 comments
Closed

Comments

@oxinabox
Copy link
Member

I am using ShortString.jl on a 32 bit system.
On a 32 bit system mmhash128_a is mmhash128_4
which is used here:
https://github.com/JuliaString/ShortStrings.jl/blob/fcacbe7e596f0a2544c3a62ab3500775dbcf139c/src/hash.jl#L3-L6

I get this error from hashing a ShortString:

  LoadError: MethodError: no method matching unsafe_load(::UInt32)
  Closest candidates are:
    unsafe_load(!Matched::Ptr) at pointer.jl:105
    unsafe_load(!Matched::Ptr, !Matched::Integer) at pointer.jl:105
  Stacktrace:
   [1] mmhash128_4(::Int32, ::UInt32, ::UInt32) at depot/packages/MurmurHash3/rOsVB/src/MurmurHash3.jl:408
   [2] hash at depot/packages/ShortStrings/j6S0g/src/ShortStrings.jl:16 [inlined]

The code that is hit is :.
Note that we are passing in a UInt rather than a Ptr

function mmhash128_4(len, pnt, seed::UInt32)
pnt, h1, h2, h3, h4 = mhbody(len >>> 4, pnt, seed, seed, seed, seed)
if (left = len & 15) != 0
h1 = xor(h1, rotl16(unsafe_load(pnt) * e1) * e2)
if left > 4
h2 = xor(h2, rotl16(unsafe_load(pnt+4) * e2) * e3)
if left > 8
h3 = xor(h3, rotl17(unsafe_load(pnt+8) * e3) * e4)
left > 12 && (h4 = xor(h4, rotl18(unsafe_load(pnt+12) * e4) * e1))
end
end
end
mhfin(len, h1, h2, h3, h4)
end

I think the problem is that a method is missing.
On a 64 bit system mmhash128_8_a is hit, and it has an overload that takes an Unsigned

mmhash128_8_a(len::Integer, val::Unsigned, seed::UInt32) =
len === 0 ? mhfin(seed) : _mmhash128(len, val, seed)

@ScottPJones
Copy link
Member

OK! When I was adapting MurmurHash3 to make ShortString efficient, it looks like I missed that 32-bit case!
I'll try to fix this weekend

@oxinabox
Copy link
Member Author

oxinabox commented Apr 8, 2021

bump

@ScottPJones
Copy link
Member

Fixed by PR #13

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants