Skip to content

keccak: optimize one-byte streaming writes - #25

Draft
yperbasis wants to merge 1 commit into
masterfrom
review/one-byte-write
Draft

keccak: optimize one-byte streaming writes#25
yperbasis wants to merge 1 commit into
masterfrom
review/one-byte-write

Conversation

@yperbasis

Copy link
Copy Markdown
Member

Summary

  • add a direct path for one-byte native sponge writes
  • avoid general copy and slice bookkeeping in byte-at-a-time callers
  • keep the block-boundary permutation behavior unchanged
  • add a focused benchmark

Why

Some streaming users feed trie or protocol prefixes one byte at a time. The general buffering path is correct, but its slice and copy setup dominates such small writes. The fast path writes directly into the existing sponge buffer and still permutes immediately when the rate block fills.

This is a performance-only refactor, so TDD is not applicable. The existing seeded fuzz test already checks byte-by-byte writes at, before, and after the 136-byte boundary against x/crypto/sha3.

Benchmark

Apple M2 Max, Go 1.25.7, median of five runs:

name                         before       after
HasherOneByteWrites/32       318.3 ns/op   247.1 ns/op
HasherOneByteWrites/128      966.9 ns/op   491.7 ns/op
HasherOneByteWrites/256     1575.0 ns/op   949.0 ns/op

All cases remain at zero allocations.

Verification

  • go test ./...
  • go test -race ./...
  • go test -tags purego ./...
  • go vet ./...
  • go mod tidy -diff
  • golangci-lint run (twice)

@AskAlexSharov

Copy link
Copy Markdown
Contributor

Code review

No correctness bug found. The fast path is equivalent to the general path at both ends of the range:

  • 0 <= s.absorbed < rate holds on every exit from sponge.Write (the general path either permutes and zeroes on hitting rate, or sets absorbed = copy(s.buf[:], p) with len(p) < rate). Reset zeroes it, Sum256 does not mutate it, and Read sets squeezing so a later Write panics before touching buf. So s.buf[s.absorbed] cannot go out of bounds.
  • absorbed == 0 and absorbed == rate-1 were checked case by case against the general path, including the block-boundary xorAndPermute — identical.
  • go test, -race, -tags purego, go vet all pass; FuzzWriteChunks 30s / 5.9M execs found nothing. Existing byte-at-a-time coverage in FuzzSum256 (seeds at rate, rate+1, rate*3+50) does exercise the new branch, as the PR body says.

Perf reproduces on an M4 Max, baseline being this tree with only the 9-line hunk removed:

bench before after
HasherOneByteWrites/32 252.4n 210.4n (-16.7%)
HasherOneByteWrites/128 602.0n 420.6n (-30.1%)
HasherOneByteWrites/256 1207n 867n (-28.2%)

A +1..12% regression on the bulk FasterKeccakHasher path showed up in the first run, but it flips sign on a longer re-run (1K goes -2.68%, 500K flat), so it is code-layout noise, not a real cost.

Two notes on the benchmark

1. keccak_write_bench_test.go measures the wrong implementation when useASM == false.
On arm64 without the SHA3 extension, or amd64 without BMI2, Hasher.Write routes to h.xc (x/crypto) and never enters the new n == 1 branch. The benchmark still reports plausible ns/op, so an A/B on such a runner comes out flat and looks like "the optimization does nothing" rather than "this machine did not run it". Asserting or reporting useASM in the benchmark would make that visible.

2. It is a new file for a single benchmark.
keccak_test.go already holds BenchmarkFasterKeccakHasher and friends plus the benchName helper. Appending there reuses the existing imports — strconv is only needed here because benchName was not reused — and keeps the benchmark set in one place.

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