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

Move argon2 to first, average, worst scoring. #24

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kmiller68
Copy link
Contributor

Since the non-simd version of the test is relatively slow I reduced the iteration/worst counts to 15/2, repsectively.

Also, I moved all the benchmark files to the argon2 directory to help organize the wasm directory a bit.

Since the non-simd version of the test is relatively slow I reduced the iteration/worst
counts to 15/2, repsectively.

Also, I moved all the benchmark files to the argon2 directory to help organize the wasm directory a bit.
@kmiller68
Copy link
Contributor Author

CC @danleh @camillobruni @eqrion @jandem for thoughts

@danleh
Copy link
Contributor

danleh commented Jan 21, 2025

Regarding the README.md (which wasn't changed by this PR, but since I looked at it): Could you add a short description where the source code is coming from? Or ideally change build.sh to checkout/download it from some URL, so we could more easily update it with whatever the upstream is?

Edit: I searched a bit, could this NPM package/repo be the upstream? https://github.com/antelle/argon2-browser

@danleh
Copy link
Contributor

danleh commented Jan 21, 2025

Mhm, I think there is something wrong with the non-SIMD variant of this benchmark. In particular, the flamegraph/profile shows tons of samples attributed to capturing stack traces, caused by growing the memory inside _emscripten_resize_heap and emscripten_realloc_buffer. See screenshot:
image

If you modify emscripten_realloc_buffer in argon2-bundle.js to not just swallow the error/failure to grow memory

                function emscripten_realloc_buffer(size) {
                    try {
                        wasmMemory.grow(size - buffer.byteLength + 65535 >>> 16);
                        updateGlobalBufferAndViews(wasmMemory.buffer);
                        return 1
                    } catch (e) { console.log(e) }
                }

you get tons of errors, e.g., in d8 RangeError: WebAssembly.Memory.grow(): Maximum memory size exceeded or in jsc RangeError: WebAssembly.Memory.grow expects the grown size to be a valid page count.

I will briefly try to rebuild this from source to diagnose further. @kmiller68 have you seen this before or is this just a problem on my end? Do you have more information on why/how the memory usage of the non-SIMD variant could be higher than the SIMD variant? Could this be an issue simply because of running multiple iterations after setup, such that later iteration continuously grow memory?

Edit: While I could build the argon2.wasm and argon2-simd.wasm binaries with the provided build.sh, the build script(s) don't seem to produce (or mention) argon2-bundle.js. How can it be re-generated? I stopped investigating further for now.

For the time being, I don't think the non-SIMD variant is measuring something sensible.

"./wasm/argon2.js",
"./wasm/argon2-benchmark.js"
"./wasm/argon2/argon2-bundle.js",
"./wasm/argon2/argon2.js",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the other Emscripten-built line items, could we move the generated files in a build/ subdirectory, to distinguish them from manually written code, such as benchmark.js (just so we/someone in the future doesn't end up mixing/interjecting manually written with generated code again).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, is it correct that both benchmark.js and argon2.js were written by you (or your colleagues)?

},
benchmarkClass: WasmLegacyBenchmark,
benchmarkClass: WasmEMCCBenchmark,
iterations: 15,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even with only 15 iterations, the non-SIMD variant runs quite long (>1 minute), while the SIMD variant runs ~2-3s with the same number of iterations. I am bit surprised by that stark difference (30x or more), so I will spend a brief time trying to understand why we are that slow. We do expect the SIMD and non-SIMD variant to do essentially the same amount of work, right?

@danleh
Copy link
Contributor

danleh commented Jan 21, 2025

(lone comment, since the file wasn't changed in this PR): I believe the ../ prefix should be stripped from the destination paths in line 17 and 21 of wasm/argon2/build.sh, see

mv dist/argon2.wasm ../argon2-simd.wasm

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