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

make it compatible with new hashing #13

Closed
wants to merge 1 commit into from
Closed

make it compatible with new hashing #13

wants to merge 1 commit into from

Conversation

narimiran
Copy link

This is now the corrected version: the new hashing algo was slightly tweaked and it won't be part of Nim v1.0.2, so the modifications are needed only for devel (v1.0.99) and for future minor versions (v1.1, etc.).

@def-
Copy link
Owner

def- commented Oct 14, 2019

lgtm, you could also sort the results and then compare so that the comparison stays the same.

I'm not sure I understand why the order changes btw, could you explain?

@narimiran
Copy link
Author

I'm not sure I understand why the order changes btw, could you explain?

Before hash(123) == 123 (for any int its hash was that int), plain and simple. The problem was when collisions start to occur in HashSet or HashTable, because every hash is bitmasked with the container length, so different (consecutive) integers might want to go into the same bucket, over and over and over again, which caused huge slowdowns.

The fix proposed in nim-lang/Nim#12407 makes hash(123) == 123 * 11, which hugely helps with avoiding collisions, making everything three orders of a magnitude faster! The consequence of this different hashing algorithm is that keys are now not sorted the same as they were before.

you could also sort the results

This is probably a better solution, as one should not depend on some internal order of things.

@def-
Copy link
Owner

def- commented Oct 14, 2019

Aaah, finally I understand, it's because of the "let d = {1: 4, 2: 5, 3: 6}.toTable". Better fix would be to make that have deterministic order. Do you want to do so or should I? I think values(d) could just be [4, 5, 6] instead.

@narimiran
Copy link
Author

Do you want to do so or should I?

I think we will save at least one iteration if you do it :)

@def-
Copy link
Owner

def- commented Oct 15, 2019

Done.

@def- def- closed this Oct 15, 2019
def- pushed a commit that referenced this pull request Oct 15, 2019
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.

2 participants