Repository navigation
Conversation
ore-rs never decrypts with AES, so Aes128 computed a decryption key schedule on every key setup for nothing. The legacy comparator keys a fresh cipher per call, so it gains most: 4-20% faster compares on aes 0.8 (Apple M4). Output is unchanged; the pinned vectors still pass.
This was referenced Oct 13, 2026
…roize assertion From the review of #109: - `Aes128Prf` derived `Debug`, so whether it rendered key material depended on `Aes128Enc`'s Debug staying opaque across `aes` upgrades. It now has the same hand-written opaque impl as `OreAes128`. - The compile-time `Aes128Enc: ZeroizeOnDrop` assertion lived in the PRNG module but guards every AES key schedule in the crate (PRF, hash, PRNG, CMAC, the PRPs' local ciphers). It moves to `primitives.rs` so a refactor of the PRNG cannot take it away. - Drop a redundant `aes::` path in the CMAC reference test.
Contributor
Author
Code review findings (
|
| # | Finding | Status |
|---|---|---|
| 1 | The saving is hardware-backend only. On the software (fixsliced) backend aes 0.8.4 defines Aes128Enc { inner: Aes128 } over one shared key schedule, so a downstream aarch64 build without --cfg aes_armv8 gains nothing. PR body and #112 stated the cost as universal. |
Fixed in the PR body and #112: claim scoped to AES-NI and ARMv8 backends. |
| 2 | Aes128Prf derived Debug, so not rendering key material depended on Aes128Enc's Debug staying opaque across aes upgrades; OreAes128 spells out an opaque impl for exactly this reason. |
Fixed (f6e403b): hand-written finish_non_exhaustive() impl. |
| 3 | The compile-time Aes128Enc: ZeroizeOnDrop assertion lived in prng.rs but guards every AES key schedule in the crate (PRF, hash, PRNG, CMAC, the PRPs' local ciphers); a PRNG refactor could silently drop it. |
Fixed (f6e403b): hoisted to primitives.rs with a comment naming every dependent. |
| 4 | The +45% compare-decimal-slice reading (286 → 415 ns on M4, rustc 1.89) was attributed to LLVM inlining compare_raw_slices into that bench binary, with no inline attribute pinning it. |
Measured here, not changed. Interleaved A/B on an M1 Max with rustc 1.78 and aes_armv8, two rounds each: compare-decimal 450/455 ns → 425/429 ns, compare-decimal-slice 454/463 ns → 427/427 ns. Both improve ~5.5%; no regression on this machine, consistent with a codegen artefact on the M4 build. No #[inline]/#[inline(never)] added: pinning codegen on a hunch would change every caller's inlining, and the bench is not a public contract. |
| 5 | Touched line kept the fully-qualified aes::Aes128Enc::new though use super::* already imports it. |
Fixed (f6e403b). |
| 6 | PR body and #112 pointed at docs/benchmarks/2026-10-13-aes-0.9-root-cause.md, which is on #108, not this branch or main. |
Fixed in the PR body and #112: the document is named as landing with #108. |
After the fixes: clippy clean, 112 lib tests and all integration tests green, pinned vectors unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #112.
ore-rs only ever encrypts with AES (PRF, PRNG, Z2 hash, CMAC test reference), but it used
aes::Aes128, which on the hardware backends (AES-NI, ARMv8 withaes_armv8) also builds the decryption key schedule on every key setup. This switches those to the encrypt-onlyaes::Aes128Enc.The saving is hardware-backend only. On the software (fixsliced) backend,
aes0.8.4 definesAes128Enc { inner: Aes128 }over one key schedule shared by both directions, so struct size and key-setup cost are unchanged there; a downstream aarch64 build without--cfg aes_armv8sees none of the numbers below.The legacy comparator (
OreAes128, plus the chrono and decimal wrappers) keys a fresh cipher with the ciphertext nonce on every comparison, so it gains the most.Benchmarks
Apple M4, rustc 1.89,
aes0.8.4 withaes_armv8. Interleaved A E A E runs; A (main) varied at most 1–2% between runs. Medians, mean of two runs:compare-decimal-slicereads +45% (286 → 415 ns), but that's not AES: in this build LLVM inlinescompare_raw_slicesinto that one bench binary, and the same jump shows up with anyaesversion whenever it does. The non-slicecompare-decimalrow improves. On an M1 Max with rustc 1.78 (interleaved, two rounds) both rows improve:compare-decimal450/455 → 425/429 ns,compare-decimal-slice454/463 → 427/427 ns, so the slice reading above is specific to that build. The root-cause write-up for the aes 0.9 regression (docs/benchmarks/2026-10-13-aes-0.9-root-cause.md) lands with #108, not this PR.This also matters for the
aes0.9 upgrade (#106). From 0.9.0, dropping a cipher wipes the whole struct byte by byte, so a smaller cipher is cheaper to drop. On upstreamaesmaster,Aes128Enctakes the legacy compare cost from +21–60% over 0.8 down to +1–29%.Safety
Aes128EncimplementsZeroizeOnDropunder the samezeroizefeature. The compile-time assertion inprng.rsnow checksAes128Enc, and the drop-zeroize tests pass.-D warnings), fmt and the full test suite pass on 1.78.