Skip to content

Remove the OptimizeLittleEndian unsafe paths - #33

Open
BobDu wants to merge 1 commit into
zeebo:masterfrom
BobDu:remove-optimize-little-endian
Open

Remove the OptimizeLittleEndian unsafe paths#33
BobDu wants to merge 1 commit into
zeebo:masterfrom
BobDu:remove-optimize-little-endian

Conversation

@BobDu

@BobDu BobDu commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

This continues 50f20c8 ("reduce/modernize some unsafe"): it removes the OptimizeLittleEndian punning and the big-endian fallbacks. Every platform now runs the same code, and moving between bytes and words no longer involves unsafe. All of it is internal — the public API is untouched.

Deleting just the punning would be a small diff but a slow one: Compress takes *[16]uint32, so byte-holding callers would convert sixteen words per block — the very work the punning existed to skip.

Instead, the boundaries move. Blocks, outputs, chaining values and keys cross the internal ABI as little-endian bytes (*[64]byte, *[32]byte), and decoding happens once, inside the compression function. The spec already defines these byte forms — a parent's block is the two child chaining values serialized — so callers mostly just copy.

That is also why the change grew past the gates themselves: every word/byte seam left on a hot path benchmarked as a conversion or a stall, and each one pushed the byte form a level deeper. The assembly is untouched; only its generated signature comments change.

Tested: the full suite with and without BLAKE3_PUREGO, on go1.27 and tip; a real big-endian run under qemu (GOARCH=ppc64); 100k randomized asm-vs-pure and default-vs-purego comparisons plus 75s of differential fuzzing, all clean. apidiff reports no public API change, and regenerating the three avo backends reproduces the checked-in assembly byte for byte.

Benchmarks on an idle m8i.large (Xeon 6975P-C), the repository's BenchmarkBLAKE3, benchstat n=6:

default build, Entire master this PR delta
64 B 499 MiB/s 499 MiB/s ~
256 B 819 MiB/s 830 MiB/s +1.4%
1 KiB 959 MiB/s 971 MiB/s +1.2%
2 KiB 928 MiB/s 929 MiB/s ~
4 KiB 1.72 GiB/s 1.71 GiB/s −0.9%
64 KiB 3.66 GiB/s 3.64 GiB/s −0.5%
1 MiB 3.77 GiB/s 3.76 GiB/s ~
BLAKE3_PUREGO, Entire master this PR delta
64 B 394 MiB/s 393 MiB/s −0.3%
256 B 540 MiB/s 577 MiB/s +6.8%
1 KiB 599 MiB/s 651 MiB/s +8.6%
2 KiB 611 MiB/s 615 MiB/s +0.6%
4 KiB 618 MiB/s 625 MiB/s +1.2%
64 KiB 626 MiB/s 598 MiB/s −4.6%
1 MiB 627 MiB/s 594 MiB/s −5.3%

The pure-Go rows track instruction counts. The kernel executes fewer real instructions than master; what grew is inline-mark nops — Go emits one per inlined call, and each m.w(i) is two inlined calls deep (the accessor, then binary.LittleEndian). Dropping a level halves the delta to about +2%, whether by calling the stdlib directly:

-	s0, s4, s8, sc = g(s0, s4, s8, sc, m.w(0), m.w(1))
+	s0, s4, s8, sc = g(s0, s4, s8, sc,
+		binary.LittleEndian.Uint32(m[0:]), binary.LittleEndian.Uint32(m[4:]))

or by writing the accessor bodies as plain shifts. Hand-inlining the shifts at every use goes the other way — the loads stop merging and it measures 3× worse. I kept the accessors for the short call sites and the read-only/write-only types; happy to switch if you'd rather have the 2%.

@BobDu

BobDu commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Measured the binary.LittleEndian-at-every-use variant from the last paragraph, same box and setup as the tables above (benchstat n=6):

default build, Entire master direct delta
64 B 499 MiB/s 499 MiB/s ~
256 B 819 MiB/s 830 MiB/s +1.4%
1 KiB 959 MiB/s 971 MiB/s +1.2%
2 KiB 932 MiB/s 929 MiB/s −0.4%
4 KiB 1.72 GiB/s 1.71 GiB/s −0.9%
64 KiB 3.66 GiB/s 3.64 GiB/s −0.5%
1 MiB 3.77 GiB/s 3.77 GiB/s ~
BLAKE3_PUREGO, Entire master direct delta
64 B 394 MiB/s 395 MiB/s +0.3%
256 B 540 MiB/s 582 MiB/s +7.7%
1 KiB 599 MiB/s 657 MiB/s +9.7%
2 KiB 611 MiB/s 623 MiB/s +2.1%
4 KiB 618 MiB/s 632 MiB/s +2.3%
64 KiB 626 MiB/s 603 MiB/s −3.7%
1 MiB 627 MiB/s 599 MiB/s −4.5%

The "+2%" above is in instruction counts; on this wide core most of the nop cost hides in the front end, so the wall-clock gap vs the accessor form is about a point on the pure rows (1 MiB: −5.3% → −4.5%). Narrower decoders should see more of it.

@zeebo

zeebo commented Aug 10, 2026

Copy link
Copy Markdown
Owner

This change is quite a large diff to remove a small amount of unsafe. Can you talk about your motivation more?

@BobDu

BobDu commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

This one turned into a classic game of whack-a-mole. Originally I just wanted to remove the punning and the tag-selected big-endian fallbacks, nothing more. The lucky part: a very small change did it, and the tests stayed green. The unlucky part: it left a byte/word conversion behind on every seam, and that showed up as about 13% more instructions on a 64-byte Sum256 and 2-8% slower on the pure-Go build.

Compress takes words — *[16]uint32 blocks and *[8]uint32 chaining values — while its callers mostly hold bytes, so every call was paying a sixteen-word conversion that the punning used to hide. The real question was the fix. Making Compress take bytes kills the friction where callers hold bytes, but for the callers that held words it only moves the seam up a level — so before committing I checked the spec and the C and Rust implementations. The spec already treats these values as byte strings — a parent block is just the two child chaining values serialized — and in both C and Rust, compress takes its block as a 64-byte array and compress_xof writes a 64-byte array back out. Bytes are the native form at this boundary rather than a workaround, so I reworked it in that direction, chasing each displaced seam until none of them showed up in benchmarks.

That is how the diff got its size, and most of it is signature changes plus the mechanical fallout across the three backends. I would argue the project comes out simpler — the five OptimizeLittleEndian gates, the two-way conversion helpers in utils, and slowCopy's special cases are all gone, and it is net −26 lines. Curious how you see it — and I hope it does not eat too much of your review time.

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