Skip to content

keccak: defer squeeze permutation until needed - #23

Draft
yperbasis wants to merge 4 commits into
masterfrom
review/lazy-squeeze
Draft

yperbasis wants to merge 4 commits into
masterfrom
review/lazy-squeeze

Conversation

@yperbasis

Copy link
Copy Markdown
Member

Summary

  • permute at the start of the next squeeze block instead of at the end of the current block
  • avoid unused work when Read stops exactly on a 136-byte boundary
  • add an invariant test and a boundary benchmark

Why

The current loop always computes the next Keccak permutation after copying byte 136, even when the caller asks for no more output. Moving that check to the top of the next loop iteration keeps the output unchanged and computes the next block only when it is requested.

This is an independent, focused split of the squeeze optimization in #13.

TDD

The regression test initially failed with:

read index after one full block = 0, want 136

Benchmark

Apple M2 Max, Go 1.25.7, representative results:

name              before       after
SpongeRead/135    154.7 ns/op   165.8 ns/op
SpongeRead/136    293.4 ns/op   163.4 ns/op
SpongeRead/137    298.5 ns/op   297.0 ns/op

The exact-boundary case avoids one full permutation. Adjacent sizes remain in the same range; the 135-byte variation is benchmark noise across runs.

Verification

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

Comment thread keccak_squeeze_asm_test.go Outdated
Comment thread keccak_squeeze_asm_test.go Outdated
Comment thread keccak_squeeze_asm_test.go Outdated
Comment thread keccak_squeeze_asm_test.go Outdated
Both new cases drove sponge directly, so they reached the platform
assembly with no useASM check and would SIGILL on a CPU without it.
Fold them into keccak_asm_test.go, which already carries that convention,
and compare Read(rate)+Read(k) against x/crypto instead of only pinning
readIdx.
Deferring the permutation cannot change the output, so the docstring's
claim was wrong: readIdx is the discriminating assertion.
@AskAlexSharov

Copy link
Copy Markdown
Contributor

Found

The docstring overclaimed. It called Read(rate) + Read(k) "the only sequence where moving the block permutation can change the output". Deferring the permutation is output-neutral: running this test file against master's Read passes the byte comparison on all five sub-tests and fails only on readIdx. So readIdx is the discriminating assertion, not the digest.

Fixed

Reworded to say what each assertion actually pins. e610be5

The byte comparison still earns its place — dropping the permutation from the boundary branch fails all five sub-tests. Nothing else found: the Read change is digest-equivalent to master, and the benchmark is honest — /136 goes 286ns → 140ns while /135 and /137 stay flat, exactly matching the permute-count analysis.

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