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

Unwind just the "pseudorandom probing" part of recent sets,tables changes #13816

Merged
merged 2 commits into from
Mar 31, 2020
Merged

Conversation

c-blake
Copy link
Contributor

@c-blake c-blake commented Mar 31, 2020

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.

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`.
@c-blake
Copy link
Contributor Author

c-blake commented Mar 31, 2020

All the tests passed. @Araq Re: https://gist.github.com/dom96/f84dd692f42a35c30bbee14309529d9e#gistcomment-3231894 and @dom96 - please take a look. Thanks.

@Araq Araq requested a review from narimiran March 31, 2020 16:20
@juancarlospaco
Copy link
Collaborator

I think that for core stuff like this all assert need to be changed to doAssert,
and add a descriptive string message for if someday eventually it does raise.

@c-blake
Copy link
Contributor Author

c-blake commented Mar 31, 2020

That sounds like a fine clean-up. Maybe for the same follow-on PR that inlines doWhile? (There are actually also a bunch in heapqueue.nim, deques.nim, lists.nim, etc. Someone besides me might be better to do all that.)

@juancarlospaco
Copy link
Collaborator

I was talking about the assert touched on this PR, but Ok. :P

@narimiran
Copy link
Member

When the only two remarks are "should this be a single or double backtick" and "should this be assert or doAssert", you know you did a good job! :)

Joking aside, I've reviewed it locally, and this is a nice, clean, precise PR (when one manually diffs it with 8c22518d6^)!

Good job @c-blake, and looking forward to your future hash-related PRs!

@narimiran narimiran merged commit b1aa3b1 into nim-lang:devel Mar 31, 2020
@c-blake
Copy link
Contributor Author

c-blake commented Mar 31, 2020

Well, I tried to follow up with one alternative hash as promised. (I may have made the git somewhat messier than ideal..sorry if so.)

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

Successfully merging this pull request may close these issues.

4 participants