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

hash_uint: simplify definitions #36120

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

hash_uint: simplify definitions #36120

wants to merge 2 commits into from

Conversation

StefanKarpinski
Copy link
Member

@StefanKarpinski StefanKarpinski commented Jun 2, 2020

Experimenting with simplifying and slightly speeding up hash_uint.

base/hashing.jl Outdated
# random constants, first must be odd
a = (0xafa64689f53d9ee1 % T) * ((0xcb9de6b205c9c1e2 % T) ⊻ n)
b = (0xe391e8b4ff155ee5 % T) + ((0xb9d8ebf206d49927 % T) ⊻ n)
c = a ⊻ b # bitrotate(a, 43) ⊻ bitrotate(b, -17)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
c = a b # bitrotate(a, 43) ⊻ bitrotate(b, -17)
c = a b

@StefanKarpinski
Copy link
Member Author

Not sure if this is worth all the doctest churn. On the other hand, it's kind of bad that so many doctests depend on the internals of hashing. Another argument for ordered dicts...

@jakobnissen
Copy link
Contributor

It's worse than that though, the doctests also depend on the internals of Dict and Set, which presumably should be free to change. Perhaps, instead of defaulting to ordered dicts, it's better to fix all the docstrings that assumes on well-defined iteration order for Dict and Set?

@KristofferC
Copy link
Member

KristofferC commented Jun 9, 2020

Why? We can just update them whenever needed. It's a single command to do it. It's not different from any other case where we slightly tweak the printing and then have to fix up the doctests.

@jakobnissen
Copy link
Contributor

We could do that, too, but changing the doctests whenever the computed result don't fit with the documentation does sort of defeat the purpose of doc-tests. Ideally, they should only be updated if the documented behaviour changes. Of course, changes to the documentation itself or printing will necessitate updating the docs, so that is an exception.

@KristofferC
Copy link
Member

KristofferC commented Jun 9, 2020

We could do that, too, but changing the doctests whenever the computed result don't fit with the documentation does sort of defeat the purpose of doc-tests.

No, it doesn't. The point of a doctest is to ensure that the examples in the documentation looks roughly correct, and don't become outdated as changes accumulate. A doctest will by itself never block a change from going in, it can just serve as a hint/warning of how a change affects the printing of stuff. If it still looks ok from a manual look then the doctest is just updated. This happens pretty much all the time.

@JeffreySarnoff
Copy link
Contributor

+1 for simplifying the core hash logic

@oscardssmith
Copy link
Member

This seems worth reviving given the potential performance implications.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants