Closed
Bug 1379290
Opened 8 years ago
Closed 8 years ago
HashGeneric() hashes 64-bit integers quite poorly
Categories
(Core :: MFBT, enhancement)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
Details
Attachments
(1 file)
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•8 years ago
|
||
Kind of a dupe of bug 1377947?
Assignee | ||
Comment 2•8 years 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•8 years 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•8 years 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•8 years ago
|
||
Attachment #8884443 -
Flags: review?(mh+mozilla)
Updated•8 years ago
|
Assignee: nobody → ehsan
Comment 6•8 years ago
|
||
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+
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
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2604d9b17f09
follow-up: Fix typo, DONTBUILD
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8f5e29f603da
https://hg.mozilla.org/mozilla-central/rev/2604d9b17f09
Status: NEW → RESOLVED
Closed: 8 years 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.
Description
•