HashGeneric() hashes 64-bit integers quite poorly

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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.
Kind of a dupe of bug 1377947?
(Assignee)

Comment 2

a year ago
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.
(Assignee)

Comment 3

a year ago
(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.)
(Assignee)

Comment 4

a year ago
(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.
(Assignee)

Comment 5

a year ago
Created 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
Attachment #8884443 - Flags: review?(mh+mozilla)
Assignee: nobody → ehsan
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
Attachment #8884443 - Flags: review?(mh+mozilla) → review+

Comment 7

a year ago
Pushed by eakhgari@mozilla.com:
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 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8f5e29f603da
https://hg.mozilla.org/mozilla-central/rev/2604d9b17f09
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.