-
Notifications
You must be signed in to change notification settings - Fork 7
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
Argon2 to JS style scoring #47
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could remove wasm/argon2/build-wasm.sh
now, right?
wasm/argon2/benchmark.js
Outdated
const tCost = 2; | ||
const mCost = 1024; | ||
const parallelism = 1; | ||
const argon2NumberOfTypes = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make it more self-explanatory what the types are, maybe simply with a comment:
// There are three argon2 algorithm variants. We test all three.
// See https://github.com/P-H-C/phc-winner-argon2/blob/f57e61e19229e23c4445b85494dbf7c07de721cb/include/argon2.h#L221.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or if we only test a single variant, we could change this to simply
// See https://en.wikipedia.org/wiki/Argon2 for the three algorithm variants.
const argon2idType = 2
wasm/argon2/build.sh
Outdated
./clean-cmake.sh | ||
EMCC_SDK_PATH=$EMCC_SDK_PATH ARGON_JS_BUILD_BUILD_WITH_SIMD=0 ./build-wasm.sh | ||
mv dist/argon2.wasm ../argon2.wasm | ||
# FIXME: Redownload the source if argon2 ever has source updates. At the time of writing it was last changed 5 years ago so this is probably not a high priority. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment to the repository where the source files are coming from? Is it https://github.com/P-H-C/phc-winner-argon2?
Very cool, thanks for the recompile and refactoring, much cleaner/more streamlined now! To your questions/points:
It is a bit unfortunate that we cannot control whether Regarding crypto algorithm/parameter choices:
I am not a cryptographer, but I trust the default choices of the well-vetted libsodium library. They only allow a time factor of >=3 for
Libsodium also picks |
Yeah, I kinda agree. On the other hand this is likely how I would have written the API were I to write it myself. In theory we could use https://github.com/tc39/proposal-explicit-resource-management if that ever ships. I'll switch it to
I think if we switch to
Sounds good to me I'll switch to just argon2id. @eqrion Do you have thoughts? |
1) Move to an explicit `free` call. 2) Only do `Argon2id` 3) Use `tCost = 3` rather than 2 per best practices. 4) Lower iteration count from 50 -> 30 and worst case from 4 -> 3
Just gonna merge this. We can address further feedback later. |
Second try at this.
This version rebuilds the benchmark without the ffi code originally used. This avoids manually instrumenting the bundle code to make it work in JetStream. I verified that both the old SIMD version and this version don't spend significant time in the glue code so changing from the ffi to directly calling the wasm instance isn't changing the workload much. I also didn't recreate the non-SIMD version of the benchmark per our discussions.
I also made a couple of other changes:
For reference on the last point https://en.wikipedia.org/wiki/Argon2 has a good summary of each of the different modes. If I were to pick only one I would probably go with "Argon2id".
One last thing to note is that the old "startup time" is actually slower than the new "first iteration". This is because the old "startup time" actually ran the "Run time" work too.