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

Replace FxHash with AHash as the default hasher #97

Merged
merged 7 commits into from Aug 4, 2019

Conversation

@Amanieu
Copy link
Collaborator

commented Jul 4, 2019

Fixes #48

cc @tkaitchuck

@Amanieu

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 4, 2019

Benchmark results are somewhat disappointing:

 name                      fx ns/iter  a ns/iter  diff ns/iter   diff %  speedup 
 insert_erase_fx_highbits  288,744     16,595         -272,149  -94.25%  x 17.40 
 insert_erase_fx_random    14,424      30,487           16,063  111.36%   x 0.47 
 insert_erase_fx_serial    13,538      20,360            6,822   50.39%   x 0.66 
 insert_fx_highbits        140,010     25,987         -114,023  -81.44%   x 5.39 
 insert_fx_random          22,838      26,389            3,551   15.55%   x 0.87 
 insert_fx_serial          21,786      19,765           -2,021   -9.28%   x 1.10 
 iter_fx_highbits          2,018       2,010                -8   -0.40%   x 1.00 
 iter_fx_random            2,035       2,053                18    0.88%   x 0.99 
 iter_fx_serial            2,037       2,038                 1    0.05%   x 1.00 
 lookup_fail_fx_highbits   130,302     3,904          -126,398  -97.00%  x 33.38 
 lookup_fail_fx_random     3,642       4,130               488   13.40%   x 0.88 
 lookup_fail_fx_serial     2,707       3,845             1,138   42.04%   x 0.70 
 lookup_fx_highbits        69,104      3,861           -65,243  -94.41%  x 17.90 
 lookup_fx_random          3,387       4,114               727   21.46%   x 0.82 
 lookup_fx_serial          3,197       3,833               636   19.89%   x 0.83 
@Amanieu

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 4, 2019

Looking at the disassembly, the only difference is the hash function (as expected):

AHash:

movabs    $0xab5ad457fe0d4dbd,%r13
xor       %rsi,%r13
movabs    $0x5851f42d4c957f2d,%rax
imul      %rax,%r13
rol       $0x9,%r13
imul      %rax,%r13

FxHash:

movabs    $0x517cc1b727220a95,%r13
imul      %rsi,%r13
@Amanieu

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 4, 2019

On my system the AHash instruction sequence has a latency of 9 cycles (imul is 3 cycles, all other instructions 1 cycle) while FxHash only has a latency of 4 cycles.

@tkaitchuck Do you think it might be possible to get away with just a single multiply? Possibly using a trick from Absl Hash which is to perform a long u64 * u64 -> u128 multiply and then xor-ing the two words of the result. link

@tkaitchuck

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

I could but that would be x86 speffic. There aren't a lot of processors with that instruction that lack the AES instruction. So I can add it as a third path, but I'm not sure it will see much use.

@tkaitchuck

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

Update: I tried changing the update function in the fallback to
(self.buffer as u128).wrapping_mul(MULTIPLE as u128).convert(); and then two xors.
It actually made it slower in my benchmarks. I don't really understand why.

@tkaitchuck

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

One thing I could do, is remove the final multiply in the finalize block. It would mean that primitives who's input only differed in the highest bit would have hashes that differ by only one bit. But with the rotation I could still guarantee that the difference would be in the low order byte. It would undermine the quality of the hash, but it may not be a problem in practice because longer inputs such as strings would not be materially affected.

@tkaitchuck

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2019

@Amanieu
I tweaked the method you suggested and pushed it to aHash 0.2.2, try it out and let me know how it performs. It now seems to work fairly well for integers. For longer strings I am keeping the previous algorithm.
I may want to put it in a feature gate in the future depending on how fast it is on other platforms.

@Amanieu Amanieu force-pushed the Amanieu:ahash branch from 2cc0b7d to 31fc802 Aug 4, 2019

@Amanieu

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 4, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2019

📌 Commit 31fc802 has been approved by Amanieu

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2019

⌛️ Testing commit 31fc802 with merge 95c19f4...

bors added a commit that referenced this pull request Aug 4, 2019

Auto merge of #97 - Amanieu:ahash, r=Amanieu
Replace FxHash with AHash as the default hasher

Fixes #48

cc @tkaitchuck
@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2019

☀️ Test successful - checks-travis
Approved by: Amanieu
Pushing 95c19f4 to master...

@bors bors merged commit 31fc802 into rust-lang:master Aug 4, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.