1379290 - HashGeneric() hashes 64-bit integers quite poorly

Status: RESOLVED FIXED

(Core :: MFBT, enhancement)

Description

[(no longer active)]

It does so by dropping the high 32-bits: <https://searchfox.org/mozilla-central/rev/f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/mfbt/HashFunctions.h#164>

Thankfully 64-bit pointers go through a different path.

Comment 1

[Mike Hommey [:glandium]]

Kind of a dupe of bug 1377947?

Comment 2

[(no longer active)]

Interestingly, the comment here <https://searchfox.org/mozilla-central/rev/f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/mfbt/HashFunctions.h#139> is actually there because https://searchfox.org/mozilla-central/rev/f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/mfbt/HashFunctions.h#186 on x86 is an overload for unsigned long long, which is why that function is called there.  So the right fix for this bug is to just use this mechanism for all pointer-sized integral types, I think.

Comment 3

[(no longer active)]

(In reply to Mike Hommey [:glandium] from comment #1)
> Kind of a dupe of bug 1377947?

Not necessarily, we can fix this bug quite easily without the fix for that bug (but yes if we fixed that bug then this would fix itself.)

Comment 4

[(no longer active)]

(In reply to :Ehsan Akhgari (Away 7/10-7/17; needinfo please, extremely long backlog) from comment #3)
> (In reply to Mike Hommey [:glandium] from comment #1)
> > Kind of a dupe of bug 1377947?
> 
> Not necessarily, we can fix this bug quite easily without the fix for that
> bug (but yes if we fixed that bug then this would fix itself.)

I have a patch that does this, by piggy-backing on the AddUintptrToHash mechanism for all integral types.

Comment 5

[(no longer active)]

Attachment: Improve the hashing of signed 64-bit integers by piggy-backing on the same mechanism as the one we use for hashing pointers

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8884443 [details] [diff] [review]
Improve the hashing of signed 64-bit integers by piggy-backing on the same mechanism as the one we use for hashing pointers

Review of attachment 8884443 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/HashFunctions.h
@@ +173,5 @@
>  }
>  
> +// We use AddUintptrToHash() for hashing all integral types.  8-byte integral types
> +// are treated the same as 64-bit pointers, and smaller integral types are first
> +// implicitly converted to 32 bits and then passed to AddUintptrToHash() to be hased.

typo: hashed

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f5e29f603da
Improve the hashing of signed 64-bit integers by piggy-backing on the same mechanism as the one we use for hashing pointers; r=glandium

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2604d9b17f09
follow-up: Fix typo, DONTBUILD

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8f5e29f603da
https://hg.mozilla.org/mozilla-central/rev/2604d9b17f09