-
Notifications
You must be signed in to change notification settings - Fork 24
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
src/libkeccak: Use XKCP's non-CPU-specific 64-bit optimized implementation #8
Conversation
Running the benchmark on macOS 10.14.2, Node 10.14.2. (Not a typo -- exact same version number!) Old code:
New code
(This uses the updated benchmark code from #7.) |
0fbea92
to
9442cb0
Compare
@cakoose users with 32-bit CPU will receive segfault? |
I think it will run correctly, but it will not run as quickly as the 32-bit optimized code. Unfortunately, I'm on macOS and can't test because I can't find a 32-bit build of Node. If you're on Linux, I think you can install a 32-bit build of Node (link) and then do:
Would also be interesting to run the benchmark on 32-bit node and see if the current reference code runs faster than the 64-bit optimized code. Ideally we would provide both the 32-bit and 64-bit optimized code and have node-gyp select the code depending on the platform. It seems like that should be possible, but unfortunately I know very little about node-gyp. |
After cakoose#1 we will need figure out how support 32-bit version and can release major version. |
9442cb0
to
fd6af9b
Compare
I figured out how to configure "binding.gyp" to do what we want.
NOTE: I tested the 32-bit optimized C code on my 64-bit machine and it still runs faster than the "reference" C code we currently use (1.7x faster on short inputs, 2.5x faster on long inputs). |
fd6af9b
to
10deb54
Compare
10deb54
to
e7b0021
Compare
I merged your commits into my branch and the tests passed. (I also incorporated your "src/README.md" fix into my own commit.) Is there anything else you would like me to do? |
66dda52
to
27fc7a6
Compare
It looks like we were using the reference C implementation, which is not designed for good performance. Switch to using either the 32-bit or 64-bit optimized C implementations, depending on the Node "arch" variable.
27fc7a6
to
d687da4
Compare
Thank you for big update and performance improvement! Published as 2.0.0 |
It looks like we were using the reference C implementation, which is not
designed for good performance.
Switching to the non-CPU-specific 64-bit optimized implementation makes
us ~2x faster on small inputs and ~6x faster on large inputs.