-
Notifications
You must be signed in to change notification settings - Fork 623
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
fix(crypto): move FNV hashes from TypeScript to Rust/Wasm and implement iteration functionality #4515
Conversation
(marked as draft because it includes/depends on changes from #4510, but this is mostly finished) |
Here's my latest full output of As Text
If anyone wants to try the benchmark on a machine with more stable performance than mine, please share the results! |
…rator behaviour and performance
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.
I like the overall idea of this PR! Excellent work. However, it introduces some breaking changes without first deprecating. We also prefer that a PR does a single thing. Let's do deprecations in a separate PR. You can search "@deprecated" to see how this is done elsewhere in the codebase.
…ad of removing." This reverts commit 35b2102.
…rypto implementation for correctness, and update crypto benchmark to also compare against runtime node:crypto implementation for performance.
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.
LGTM
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.
LGTM too! Again, great work 👍🏾
Moves std/crypto's FNV hash implementations from TypeScript into the Rust/Wasm bundle.
Background
The current implementation of the FNV hashing functions in std/crypto is implemented in TypeScript, unlike the other (cryptographic) hashing functions which are implemented in Rust/Wasm. The intent of this choice was to avoid the overhead of calling into Wasm, and to avoid the need of depending on third-party code for an algorithm that's simple enough to implement ourselves and for which we don't need to make any cryptographic guarantees.
Performance
Avoiding Wasm does yield performance benefits in some cases. On my machine, FNV32 hashing 64 bytes of data takes an average of 2 microseconds with either the TypeScript or Wasm implementations, but the Wasm performance is higher-variance, at least while it's initializing and warming up (with a 99th percentile performance of 10 microseconds and worst-case of 2 milliseconds). So if you're hashing a lot of very small values with FNV32/FNV32A, the TypeScript implementation can come out ahead.
However, in most other cases the situation is different. The TypeScript FNV64/FNV64A implementation incurs a lot of extra overhead from how it performs 64-bit integer operations using JavaScript's 64-bit float number type. Hashing 64 bytes of data takes an average of 8 microseconds in TypeScript versus 2 microseconds in Wasm. If the data becomes much larger, the TypeScript FNV performance abruptly goes off a cliff:
(Numbers from running the updated benchmark in
crypto/_benches/bench.ts
on my noisy janky machine, rounded for simplicity.)This means that for many cases, using the current FNV implementation can be much slower than using one of the Wasm cryptographic hash implementations, when the reason it exists is supposed to be to provide a faster alternative for non-cryptographic use cases. This is an issue.
Maybe it's possible to optimize the TypeScript implementation to close this gap, but I'm not sure how to do that, while I do know how to move the code into our Rust/Wasm bundle, so that's what this PR does.
Correctness
The TypeScript FNV implementation has the separate issue that it currently doesn't support iterable inputs, such as Web Streams, but that's something our digest functions are meant to support and the Wasm/Rust implementations do (this PR resolves #3811). It can be refactored to handle this case, but when I did so that resulted in some duplication of logic and additional complexity that goes away when we move it into Rust/Wasm instead.
There are several existing implementations of FNV hashes on crates.io, however none of the widely-used ones provide all four variants we need. Since it's only a handful of lines of logic, I think it's safer just to implement it ourselves rather than add a new dependency on a not-widely-used crate, so that's what I've done here.
This PR merges the tests for FNV hashes with our tests for other hash functions, and adds the six test inputs specified in IETF draft-eastlake-fnv-21 and the four from Go's standard library tests to validate our implementation.
This change updates the
crypto/_benches/bench.ts
script to compare Wasm, WebCrypto, node:crypto, and TypeScript implementations, where applicable.Size Impact
This change increases the size of
deno_std_wasm_crypto.generated.mjs
from 194,324 B to 196,614 B (a 1% increase of 2,290 B), or from 77,230 B to 78,068 B aftergzip --best
(a 1% increase of 838 B). (Note that this PR follows #4510 which reduces those initial sizes by 18% and 13% respectively, so the next release will still have a significant net reduction in Wasm size.)The total size of the deleted TypeScript files implementing FNV hashes was 6252 B on disk.
Tangential Minor Breaking Change
This PR also renames the exported(Note by @kt3k: This part is reverted in this PR).wasmDigestAlgorithms
toDIGEST_ALGORITHM_NAMES
, to match Deno's style guide and reflect the fact that it includes all algorithm names supported by the digest function, not just a subset. The exportedWasmDigestAlgorithm
type is removed, as it's now redundant with the existingDigestAlgorithmName
export. (Generally, the fact that some algorithms are implemented in Wasm should probably be considered an implementation detail, and not exposed in the public interface.) TheFNVAlgorithms
export is also removed, since they're no longer a special case.