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

KeccakHash minor opt #10

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

KeccakHash minor opt #10

wants to merge 2 commits into from

Conversation

tkstanczak
Copy link

Benchmark below + it required one more pair of eyes
Method | AllZeros | Mean | Error | StdDev | Scaled |
MeadowHashSpan | False | 620.4 ns | 1.379 ns | 1.152 ns | 1.00 |
MeadowHashBytes | False | 615.3 ns | 1.902 ns | 1.779 ns | 1.00 |
New | False | 479.3 ns | 1.464 ns | 1.223 ns | 1.00 |
HashLib | False | 1,228.6 ns | 1.927 ns | 1.609 ns | 1.00 |
MeadowHashSpan | True | 617.6 ns | 6.739 ns | 6.304 ns | 1.00 |
MeadowHashBytes | True | 614.5 ns | 7.798 ns | 6.912 ns | 1.00 |
New | True | 474.5 ns | 1.596 ns | 1.333 ns | 1.00 |
HashLib | True | 1,307.2 ns | 27.010 ns | 52.680 ns | 1.00 |

Benchmark below + it required one more pair of eyes
Method | AllZeros |       Mean |     Error |    StdDev | Scaled |
MeadowHashSpan |    False |   620.4 ns |  1.379 ns |  1.152 ns |   1.00 |
MeadowHashBytes |    False |   615.3 ns |  1.902 ns |  1.779 ns |   1.00 |
New |    False |   479.3 ns |  1.464 ns |  1.223 ns |   1.00 |
HashLib |    False | 1,228.6 ns |  1.927 ns |  1.609 ns |   1.00 |
MeadowHashSpan |     True |   617.6 ns |  6.739 ns |  6.304 ns |   1.00 |
MeadowHashBytes |     True |   614.5 ns |  7.798 ns |  6.912 ns |   1.00 |
New |     True |   474.5 ns |  1.596 ns |  1.333 ns |   1.00 |
HashLib |     True | 1,307.2 ns | 27.010 ns | 52.680 ns |   1.00 |
@zone117x
Copy link
Member

zone117x commented Dec 1, 2018

Thanks for the improvement! Looks like some tests are broken. Can you fix those?
Also can you verify that those large stackallocs don't cause issues on any runtimes? I've tried those for this hash code previously but ran into issues.

@tkstanczak
Copy link
Author

Yes, I had some changes in other places that I did not want to touch in your code and did not remove them properly from this commit - I hope I will get a moment soon to fix the PR.

I was wondering about the stackallocs too at the beginning but they have been behaving well so far. Please note that we only use the code for Keccak256 so the stackallocs are not so big in the end. I imagine it may be more of a problem with bigger hash sizes. What was the nature of problems that you experienced? Which runtimes were affected? We were running it at all three platforms without issues.

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