-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix #13393 better hash for primitive types, avoiding catastrophic (1000x) slowdowns for certain input distributions #13410
fix #13393 better hash for primitive types, avoiding catastrophic (1000x) slowdowns for certain input distributions #13410
Conversation
e618c78
to
e937b25
Compare
@timotheecour Does this also work for the JS backend? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(can't delete this comment, was a duplicate of #13410 (comment))
yes the js backend also improves hashes for primitive types, with improved randomness: import hashes, strformat
template hash2(x) =
block:
let s {.inject.} = astToStr(x)
let j {.inject.} = hash(x)
echo &"""{s:>8} => {j}"""
hash2(0.3)
hash2("foo")
hash2(123)
hash2(124)
hash2(125) import os,strformat
proc main() =
let file = currentSourcePath / "../t10210.nim"
for nim in @["/Users/timothee/git_clone//nim//Nim_devel//bin/nim", "/Users/timothee/git_clone//nim//Nim_prs//bin/nim.pr_fix_11764_hash_revived"]:
for mode in @["c", "js"]:
let cmd = fmt"{nim} {mode} -r --hints:off {file}"
echo cmd
doAssert execShellCmd(cmd) == 0
main() prints:
note that hashes for |
90f5b71
to
92f4f76
Compare
9ea79a5
to
4cec93a
Compare
PTAL:
|
e58c324
to
698a13f
Compare
698a13f
to
e0c65bf
Compare
d456421
to
326d1c0
Compare
Yet there will be other distributions out there that are now much worse than before. The real solution is to add some logic to tables.nim to compensate for the case of very many hash collisions. That's what Python does and Python keeps its hash function for integers really simple. |
seems like a purely theoretical argument; while you can in theory find a (contrived, adversarial) distribution that would be made worse by a given hash function, in practice this shouldn't happen, at least much less often than with current hash (but please prove me wrong with a realistic example that causes lots of collisions with new scheme) Good hash properties are commonly agreed upon, see for eg https://www.sparknotes.com/cs/searching/hashtables/section2/ and https://gist.github.com/badboy/6267743. Of particular importance is avalanche property:
which the previous hashing scheme we had ( Another one was small floats, which before this PR would all collapse. These cases do occur in practice, which prompted #13393 and #11764 in the first place.
we can definitely improve logic for handling collisions, that's not mutually exclusive. But at least with this PR we can avoid those 100x-1000X catastrophic slowdowns from that bug report. |
Well you said the relevant word here, adversarial. This is a problem with hash tables in a stdlib, you don't know what it will be used for but inevitably it will end up for critical software and we need to prevent hash collision attacks. Just coming up with a "better hash" function isn't gonna cut it.
Yet Python does not need these complex hash functions for integers because its other mechanisms work out. Look, I agree that you made collisions less likely for many realworld distributions. But you also made the hashing of integers slower and the bad security is hardly improved. |
actually that's incorrect, only slower for 64 bit integers:
so for performance minded applications, either you have < 4 billions keys and you can benefit from faster hashing via int32/uint32 (and much less chances of catastrophic collisions) [simplifying a bit the argument but you get the idea], or you have > 4 billion keys and need int64, and in that case yes, pay a price for slower key computations but other factors like memory issues (or collisions) would likely be much more important
plenty of applications don't have to deal with adversarial input. And even for the ones that do, there are options to deal with those, eg adding a runtime random seed in hash function, as well as better strategy for dealing with collisions in tables implementation. But reducing chances of collisions in the 1st place should improve performance regardless. |
Your alternative PR was merged, closing. |
* Unwind just the "pseudorandom probing" (whole hash-code-keyed variable stride double hashing) part of recent sets & tables changes (which has still been causing bugs over a month later (e.g., two days ago #13794) as well as still having several "figure this out" implementation question comments in them (see just diffs of this PR). This topic has been discussed in many places: #13393 #13418 #13440 #13794 Alternative/non-mandatory stronger integer hashes (or vice-versa opt-in identity hashes) are a better solution that is more general (no illusion of one hard-coded sequence solving all problems) while retaining the virtues of linear probing such as cache obliviousness and age-less tables under delete-heavy workloads (still untested after a month of this change). The only real solution for truly adversarial keys is a hash keyed off of data unobservable to attackers. That all fits better with a few families of user-pluggable/define-switchable hashes which can be provided in a separate PR more about `hashes.nim`. This PR carefully preserves the better (but still hard coded!) probing of the `intsets` and other recent fixes like `move` annotations, hash order invariant tests, `intsets.missingOrExcl` fixing, and the move of `rightSize` into `hashcommon.nim`. * Fix `data.len` -> `dataLen` problem. * This is an alternate resolution to #13393 (which arguably could be resolved outside the stdlib). Add version1 of Wang Yi's hash specialized to 8 byte integers. This gives simple help to users having trouble with overly colliding hash(key)s. I.e., A) `import hashes; proc hash(x: myInt): Hash = hashWangYi1(int(x))` in the instantiation context of a `HashSet` or `Table` or B) more globally, compile with `nim c -d:hashWangYi1`. No hash can be all things to all use cases, but this one is A) vetted to scramble well by the SMHasher test suite (a necessarily limited but far more thorough test than prior proposals here), B) only a few ALU ops on many common CPUs, and C) possesses an easy via "grade school multi-digit multiplication" fall back for weaker deployment contexts. Some people might want to stampede ahead unbridled, but my view is that a good plan is to A) include this in the stdlib for a release or three to let people try it on various key sets nim-core could realistically never access/test (maybe mentioning it in the changelog so people actually try it out), B) have them report problems (if any), C) if all seems good, make the stdlib more novice friendly by adding `hashIdentity(x)=x` and changing the default `hash() = hashWangYi1` with some `when defined` rearranging so users can `-d:hashIdentity` if they want the old behavior back. This plan is compatible with any number of competing integer hashes if people want to add them. I would strongly recommend they all *at least* pass the SMHasher suite since the idea here is to become more friendly to novices who do not generally understand hashing failure modes. * Re-organize to work around `when nimvm` limitations; Add some tests; Add a changelog.md entry. * Add less than 64-bit CPU when fork. * Fix decl instead of call typo. * First attempt at fixing range error on 32-bit platforms; Still do the arithmetic in doubled up 64-bit, but truncate the hash to the lower 32-bits, but then still return `uint64` to be the same. So, type correct but truncated hash value. Update `thashes.nim` as well. * A second try at making 32-bit mode CI work. * Use a more systematic identifier convention than Wang Yi's code. * Fix test that was wrong for as long as `toHashSet` used `rightSize` (a very long time, I think). `$a`/`$b` depend on iteration order which varies with table range reduced hash order which varies with range for some `hash()`. With 3 elements, 3!=6 is small and we've just gotten lucky with past experimental `hash()` changes. An alternate fix here would be to not stringify but use the HashSet operators, but it is not clear that doesn't alter the "spirit" of the test. * Fix another stringified test depending upon hash order. * Oops - revert the string-keyed test. * Fix another stringify test depending on hash order. * Add a better than always zero `defined(js)` branch. * It turns out to be easy to just work all in `BigInt` inside JS and thus guarantee the same low order bits of output hashes (for `isSafeInteger` input numbers). Since `hashWangYi1` output bits are equally random in all their bits, this means that tables will be safely scrambled for table sizes up to 2**32 or 4 gigaentries which is probably fine, as long as the integer keys are all < 2**53 (also likely fine). (I'm unsure why the infidelity with C/C++ back ends cut off is 32, not 53 bits.) Since HashSet & Table only use the low order bits, a quick corollary of this is that `$` on most int-keyed sets/tables will be the same in all the various back ends which seems a nice-to-have trait. * These string hash tests fail for me locally. Maybe this is what causes the CI hang for testament pcat collections? * Oops. That failure was from me manually patching string hash in hashes. Revert. * Import more test improvements from #13410 * Fix bug where I swapped order when reverting the test. Ack. * Oh, just accept either order like more and more hash tests. * Iterate in the same order. * `return` inside `emit` made us skip `popFrame` causing weird troubles. * Oops - do Windows branch also. * `nimV1hash` -> multiply-mnemonic, type-scoped `nimIntHash1` (mnemonic resolutions are "1 == identity", 1 for Nim Version 1, 1 for first/simplest/fastest in a series of possibilities. Should be very easy to remember.) * Re-organize `when nimvm` logic to be a strict `when`-`else`. * Merge other changes. * Lift constants to a common area. * Fall back to identity hash when `BigInt` is unavailable. * Increase timeout slightly (probably just real-time perturbation of CI system performance).
using the tests provided by @rockcavera in https://github.com/rockcavera/nim-problem:
now completes in 0.075 seconds instead of 75.4 seconds prior to this PR, a speedup of 1000X for this example, which is exactly consistent with what I had already observed in #11767 on my own benchmark, noting a 100 to 1000 speedup (https://github.com/timotheecour/vitanim/blob/master/testcases/tests/t0129b.nim)
before PR
hash(x)=x bitand 2^n-1
which is a terrible hash resulting in lots of (trivial) collisions, ignoring all high order bits.cast[Hash](cast[uint](x) * 11)
which is still not a good hash, resulting in lots of collisions depending on input distributions, as demonstrated by Slow insertion of uint in Tables and Sets Collections depending on the value #13393; for example when input distributions only vary in high order bits, the hash function collides to a single pointafter PR
all primitive types (including pointer etc) with sizeof(x) in {4,8} pass through either a hashUInt64 or hashUInt32 specialized hash that has good hashing properties (cascading effect), as commonly prescribed in articles discussing hashing functions
sizeof(x)<4 was faster using the preexisting identity hash so the fix checks for that sizeof(x)>=4 criterion; indeed in this case the number of unique values is limited so collisions are less of an issue.
float is now properly handled; in particular denormalization is checked so that hash(0.0)==hash(-0.0), and also removes the previous hack based on
x+1.0
which caused collisionsperformance regression test is added, testing various input distributions, and for nim c, cpp, js, as well as VM for those modes.
It's a tricky problem to get it right: it has to work on 32 bit, 64 bit, with js, c, cpp, with VM during defined(js) (where sizeof(int)=4 even on 64bit), etc; and it has be be performant in all cases.