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

add t1ha hash (fastest, pass all tests) #18

Closed
erthink opened this issue Nov 17, 2016 · 13 comments
Closed

add t1ha hash (fastest, pass all tests) #18

erthink opened this issue Nov 17, 2016 · 13 comments

Comments

@erthink
Copy link
Contributor

erthink commented Nov 17, 2016

ready = https://github.com/leo-yuriev/t1ha/tree/smhasher-rurban.t1ha

@rurban
Copy link
Owner

rurban commented Nov 17, 2016

Thanks, looks good

@rurban
Copy link
Owner

rurban commented Nov 18, 2016

Merged, thanks. I just had no room for your README.md docs. Maybe put them into the header.

@rurban rurban closed this as completed Nov 18, 2016
@erthink
Copy link
Contributor Author

erthink commented Nov 18, 2016

Sorry, but I just updated t1ha.
This is a fairly important change.

In general, I was switched to little bit faster 'mux' version (with 64x64 mul) and to C from C++.
But also fixes a possibility of crossing page boundary at the end of key, in case it was unaligned.
This is not a flaw in case using t1ha in the 1Hippeus project, but seriously bug for general hash function.

Please merge again.

@rurban
Copy link
Owner

rurban commented Nov 18, 2016

Can you add the t1ha_mux variant also?
This is now missing from your smhasher-rurban.t1ha branch

@rurban rurban reopened this Nov 18, 2016
@erthink
Copy link
Contributor Author

erthink commented Nov 18, 2016

It was decided to leave only the "mux" variant, at the same time renaming it into "t1ha".
Maybe it's not the best solution, but in general should be better.

I think, I should clarify:

  • The 'mux' version (which now called as main "t1ha") is faster for small keys, but same speed for large keys.
  • The old version (originally called 1Hippeus Hash) was dropped, because it should be fixed/refined against flaw that I noted.
  • This should not be mislead about "t1ha" name, because it just published as "Positive Hash" and it have not been used before.

https://github.com/leo-yuriev/t1ha/blob/smhasher-rurban.t1ha/main.cpp#L184

@rurban
Copy link
Owner

rurban commented Nov 18, 2016

But this would not work on 32bit then. No plan for a 32bit version?

@erthink
Copy link
Contributor Author

erthink commented Nov 18, 2016

It should already work. More over, much faster than mum-hash.

I implemented the required multiplication for the absence of __int128.

https://github.com/leo-yuriev/t1ha/blob/smhasher-rurban.t1ha/t1ha.c#L149

@rurban
Copy link
Owner

rurban commented Nov 18, 2016

Great, updating and testing it now

@rurban
Copy link
Owner

rurban commented Nov 18, 2016

This looks like a bug:

add_with_carry(uint64_t *sum, uint64_t addend) {
  *sum += addend;
  return sum < addend;
}

=>
return *sum < addend;

and it is also invalid C++ with -fpermissive

@erthink
Copy link
Contributor Author

erthink commented Nov 18, 2016

Oops. Sure this is bug. Just lost de-referense while conversion from C++ to C.

It is question for QA. How it passes the tests?

@rurban
Copy link
Owner

rurban commented Nov 18, 2016

I was QA. I didn't pass :)

in smhasher it just added randomly a 0 or 1 to it, which apparently qualifies as good enough.

on 32bit some constants are also too long and are capped. e.g. 2166136261 would need a 2166136261L there. but this is in a different function.

@rurban rurban closed this as completed Nov 18, 2016
@erthink
Copy link
Contributor Author

erthink commented Nov 18, 2016

;)

+1

I am on the road approximately two hours.

@erthink
Copy link
Contributor Author

erthink commented Nov 18, 2016

Thank!

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

No branches or pull requests

2 participants