perf(kernel-routes): decode artifact pages with base64-simd - #206
Conversation
…iB page spends its time hashing, not decoding The scalar decoder ran at 1.6 GB/s and took 40% of a full-page ingest; base64-simd dispatches to AVX2 or NEON at runtime with a scalar fallback and decodes the same standard alphabet five times faster.
|
Warning Review limit reachedNext included review available in 31 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 108 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Essentials Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| let decoded = base64::engine::general_purpose::STANDARD | ||
| .decode(parsed.bytes_base64.as_bytes()) | ||
| let decoded = base64_simd::STANDARD | ||
| .decode_to_vec(parsed.bytes_base64.as_bytes()) |
There was a problem hiding this comment.
Correctness of this swap rests on base64_simd::STANDARD rejecting/accepting exactly what base64 0.22's STANDARD engine does (same alphabet, same canonical-padding strictness, no forgiving/whitespace-tolerant mode). The PR description says this was checked with an offline differential fuzz run over 200k mutated encodings, but that isn't checked into the repo, and the only in-suite test that sends malformed bytes_base64 (tests/kernel_routes.rs:3373-3379, "not base64 at all!") targets an upload id that was never begin'd, so accepts_page short-circuits with upload_not_found before this line is ever reached — the decode-rejection branch below ("is not standard base64") has no automated coverage today.
Worth adding a small test that begins an upload and then feeds genuinely malformed base64 (bad alphabet char, truncated/extra padding, non-zero padding bits) through ingest.page, so a future base64-simd upgrade or config change that diverges from base64's acceptance rules gets caught in CI rather than relying on a one-off validation run.
There was a problem hiding this comment.
Confirmed and fixed in bc4b3af. The "not base64 at all!" case at tests/kernel_routes.rs:3373 never reached the decoder: accepts_page returns upload_not_found first, so the "is not standard base64" branch had no coverage.
Added a_begun_upload_refuses_a_page_that_is_not_canonical_standard_base64, which begins an upload and sends ten malformed shapes through ingest.page: a byte outside the alphabet, the URL-safe alphabet (-/_), dropped / extra / misplaced padding (QUJDRA, QUJDRA===, QUJDR===, QUJDRA=A), non-zero trailing bits (QUJDRB==), embedded and trailing whitespace, and two frames concatenated. Each must come back invalid_params with the decode message, and each is also asserted rejected by base64 0.22's STANDARD engine, so the test pins base64-simd to the scalar engine's acceptance rule. It then completes the upload with a payload whose encoding contains + and /, checking the standard alphabet on the accept side, and asserts the staging budget returns to zero so each refusal released its decode reservation.
ReviewSmall, well-scoped perf change: swaps the scalar Code quality / correctness
Security
Performance
No blocking issues found; the one suggestion above is a test-coverage improvement, not a bug. |
…ive upload The only test that sent malformed bytes_base64 targeted an upload that was never begun, so accepts_page refused it before the decoder ran. The new test begins an upload and sends ten malformed shapes: a byte outside the alphabet, URL-safe alphabet, dropped/extra/misplaced padding, non-zero trailing bits, embedded and trailing whitespace, and two frames run together. Each must be refused as invalid_params and also rejected by the base64 crate's STANDARD engine, which pins base64-simd to the same acceptance rule. The upload then completes with a payload whose encoding uses '+' and '/', so the standard alphabet is checked on the accepting side too.
Review: perf(kernel-routes): decode artifact pages with base64-simdSolid, well-scoped swap. Went through the decode path in What looks good:
One suggestion (non-blocking): see inline comment on Didn't execute the guard commands myself in this environment (sandboxed, no cargo/network access), so I'm relying on static review — the described guard ( |
| # Page decoding: SIMD with runtime dispatch and a scalar fallback; decodes | ||
| # the same inputs to the same bytes as `base64`'s STANDARD engine and rejects | ||
| # the same malformed ones. | ||
| base64-simd = "0.8" |
There was a problem hiding this comment.
The comment states this crate "decodes the same inputs to the same bytes as base64's STANDARD engine and rejects the same malformed ones" — a strong equivalence claim. The only thing enforcing that in-repo is the 10 fixed cases in the new kernel_routes.rs test; the "differential over 200k mutated encodings" mentioned in the PR description isn't captured anywhere in the repo (no fuzz target, no proptest strategy against base64's decoder).
Since the version constraint ("0.8") allows patch updates, a future base64-simd 0.8.x release could in principle shift an edge case (padding strictness, trailing-bit checks, etc.) with nothing in CI to catch a divergence beyond the 10 hardcoded shapes. Given the repo already has proptest as a dev-dependency, consider committing the differential check as a proptest property (base64_simd::STANDARD.decode(x) == base64::engine::general_purpose::STANDARD.decode(x) for arbitrary mutated byte strings) so the equivalence this comment promises stays enforced as the pin moves. Not blocking — the current test coverage is reasonable for the size of this diff.
There was a problem hiding this comment.
Agreed — the comment promised more than the ten fixed cases enforced. Added tests/base64_differential.rs in 75a9b56, following the existing *_differential.rs pattern with base64's STANDARD engine as the reference:
valid_encodings_decode_to_the_same_bytes(512 cases): arbitrary 0–384-byte payloads encoded withbase64decode to the payload under both engines.mutated_encodings_get_the_same_verdict(4096 cases): a valid encoding with 1–4 edits (replace / insert / delete / truncate), drawing bytes from the boundary set —=,+,/,-,_,\n,\r, space,A/B/Q/R/w/x(zero vs non-zero trailing bits),0x00,0xff, andany::<u8>()— must get the sameOk(bytes)/Errfrom both.arbitrary_bytes_get_the_same_verdict(1024 cases): short strings built only from that boundary set.
Runs in ~4s. A base64-simd 0.8.x that shifts padding strictness, trailing-bit checks, or alphabet now fails CI rather than the comment.
… differential proptest The Cargo.toml note promises that base64-simd accepts, decodes, and rejects exactly what base64's STANDARD engine does. Ten fixed cases in kernel_routes.rs were the only in-repo check, and the "0.8" pin admits patch releases that could move an edge case. Add differential proptests against base64 STANDARD for valid encodings, mutated encodings, and short strings of edge bytes (padding, standard-only and URL-safe alphabet positions, whitespace, non-zero trailing bits).
| } | ||
|
|
||
| proptest! { | ||
| #![proptest_config(ProptestConfig::with_cases(4096))] |
There was a problem hiding this comment.
The PR description says the differential check covered "200k mutated encodings," but the case counts committed here sum to 512 + 4096 + 1024 = 5,632 (no PROPTEST_CASES override found in the repo). Either the description is stale from a larger local run, or the committed config is lower than what was actually validated — worth reconciling so the safety claim backing this decoder swap matches what CI will actually execute.
There was a problem hiding this comment.
The 200k figure was a one-off local run before the test was committed; the committed config is intentionally smaller so the target stays a ~4 s CI check. Reconciled by editing the PR description: it now says the 200k run was local and that tests/base64_differential.rs re-verifies the same property at 5,632 cases per run in CI. The "Guard" paragraph was also stale ("no file under tests/ changed") and now lists both test files.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75a9b561a3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| proptest! { | ||
| #![proptest_config(ProptestConfig::with_cases(4096))] | ||
| #[test] | ||
| fn mutated_encodings_get_the_same_verdict(text in mutated_encoding()) { |
There was a problem hiding this comment.
The new compatibility guard is not executed by the inspected mc-host-lifecycle workflow: .github/workflows/ci.yml runs only --lib and an explicit list of integration-test targets at lines 271-289, none of which includes base64_differential. Consequently, a future allowed base64-simd 0.8 update can change decoding semantics without this test detecting it; add this target to the CI invocation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — neither base64_differential nor kernel_routes was in the enumerated targets. Fixed in 13f628c: base64_differential is added to the "Module differential contracts" step, and a new "Kernel route contracts" step runs cargo nextest run -p mc-module --test kernel_routes (the self dev-dependency already enables test-support, so no feature flag is needed). Both suites pass under nextest locally (49 tests, ~40 s).
Review summarySolid, low-risk change: a single call-site swap from Two things worth reconciling before merge:
Neither point blocks the correctness of the decoder swap itself — the differential test methodology is sound, just not fully reflected in the numbers or wired into automated enforcement yet. |
Neither tests/base64_differential.rs nor tests/kernel_routes.rs was in the enumerated mc-module test targets, so a base64-simd bump or an ingest change could break either without CI noticing. The self dev-dependency already turns on test-support, so kernel_routes needs no extra feature flag here.
|
Both points addressed:
|
| fn encoded_payload() -> impl Strategy<Value = Vec<u8>> { | ||
| prop::collection::vec(any::<u8>(), 0..384).prop_map(|bytes| { | ||
| base64::engine::general_purpose::STANDARD | ||
| .encode(bytes) |
There was a problem hiding this comment.
Finding (recall-biased review, PLAUSIBLE): differential fuzz coverage stops far short of production page sizes.
encoded_payload() (and the mutation strategies built on it) only generate payloads in 0..384 bytes. But the handler this suite is meant to protect (crates/mc-module/src/kernel_routes/ingest.rs:715) decodes pages up to PAGE_BYTES_MAX = 16 * MIB, and the PR's own benchmark numbers exercise a 16 MiB page.
base64-simd's AVX2/NEON kernels process input in fixed-size SIMD chunks with a scalar remainder loop; classes of bugs in this kind of code (offset/counter handling across many chunk iterations, remainder-loop stitching only reachable past a certain chunk count) tend to surface at large sizes, not small ones. A 384-byte cap means at most a few dozen SIMD blocks are ever exercised here — nowhere near the ~350k blocks a 16 MiB page would drive through the same code.
Failure scenario: a legitimate or malformed 16 MiB artifact page hits a size-dependent decode divergence in base64_simd::STANDARD that never manifests for inputs ≤384 bytes. CI (and the local 200k-mutation run described in the PR body, which appears to use the same small-payload generator) stays green while a production-sized page is silently mis-decoded or wrongly accepted/rejected.
Suggest widening the size strategy (e.g. prop::collection::vec(any::<u8>(), 0..=PAGE_BYTES_MAX as usize) gated to a smaller case count, or at least a few fixed large-size cases near typical/max page sizes) so the differential check actually covers the sizes this decoder runs against in production.
There was a problem hiding this comment.
Fair — 384 bytes exercises the remainder loop but only a handful of full SIMD blocks. Fixed in f82d0a8: production_sized_pages_decode_to_the_same_bytes runs a fixed-size pass over 4 KiB, 64 KiB+1/+2/+3 (each remainder class past a block boundary), 256 KiB, 1 MiB+17, PAGE_BYTES_MAX - 1, and PAGE_BYTES_MAX (16 MiB) with pseudo-random payloads. Each size checks both engines decode to the payload, then corrupts the encoding at four positions (mid-stream !, = five bytes from the end, B in the last position, \n at one third) and asserts identical verdicts. The 16 MiB case drives ~350k blocks through the kernel; the whole test takes ~5.5 s. I kept the proptest strategies small so shrinking stays fast and put the size coverage in fixed cases instead of an 0..=16 MiB strategy.
| # Artifact pages cross the JSON ring as base64; same version mc-tokenizer uses. | ||
| base64 = "0.22" | ||
| # Page decoding: SIMD with runtime dispatch and a scalar fallback; decodes | ||
| # the same inputs to the same bytes as `base64`'s STANDARD engine and rejects |
There was a problem hiding this comment.
Finding (recall-biased review, PLAUSIBLE): NEON dispatch path is never exercised by CI.
The PR description and this comment both claim base64-simd "dispatches at runtime to AVX2 or NEON with a scalar fallback," and the differential suite (tests/base64_differential.rs) is the mechanism relied on to prove base64_simd::STANDARD matches base64 0.22 STANDARD on acceptance and output. But every CI job in .github/workflows/ci.yml runs on runs-on: ubuntu-latest, i.e. x86_64 only — there is no aarch64 runner anywhere in the workflow.
Failure scenario: if this service is ever built/run on an ARM host (e.g. AWS Graviton, Apple Silicon dev machines), the NEON kernel — the one code path the differential test is supposed to validate but structurally cannot on x86 CI — could diverge from the AVX2/scalar-verified behavior (accept something base64 would reject, or decode to different bytes) and ship straight to production, since nothing in this PR's guard (cargo test/cargo nextest steps, all x86-only) would ever catch it.
Worth at least flagging as a known gap, or adding an aarch64 job (even just for base64_differential) if ARM is a real deployment target.
There was a problem hiding this comment.
Correct — every job in ci.yml was x86-64, and ARM is a real target (the retrieval benchmark already uses a self-hosted ARM64 runner). Fixed in f82d0a8: new base64-differential-arm64 job on the GitHub-hosted ubuntu-24.04-arm runner (free for public repos) that checks out commons the same way mc-host-lifecycle does and runs cargo nextest run -p mc-module --test base64_differential, so the NEON kernel is now checked against the reference decoder on every push.
Remaining known gap: the scalar fallback is only reached on a host without AVX2/NEON, which neither runner is, and vsimd's runtime detection can't be disabled per-test. That path is the one base64-simd upstream tests most directly, so I'm leaving it uncovered here rather than adding a no-SIMD build variant.
…rm64 The proptest strategies stop at 384-byte payloads, so at most a few dozen SIMD blocks ran per case while the handler decodes pages up to 16 MiB. Add a fixed-size pass over 4 KiB through PAGE_BYTES_MAX, including remainders of one to three bytes past a 64 KiB boundary, with corruptions placed mid-stream and in the padding tail. Every existing CI job runs on x86-64, so the NEON kernel base64-simd selects on aarch64 was never executed. Add an arm64 job that runs the differential suite.
| - uses: taiki-e/install-action@nextest | ||
|
|
||
| - name: NEON path against the reference decoder | ||
| run: cargo nextest run -p mc-module --test base64_differential |
There was a problem hiding this comment.
Correctness / test-coverage gap: the scalar fallback of base64-simd is never exercised.
base64-simd = "0.8" does runtime CPU-feature dispatch with a scalar fallback (per the comment added in crates/mc-module/Cargo.toml: "SIMD with runtime dispatch and a scalar fallback"). The new differential coverage only runs on:
- the existing x86 runner (
mc-host-lifecycle,ubuntu-latest) — almost certainly AVX2-capable, so it exercises the AVX2 kernel, and - this new
ubuntu-24.04-armjob — NEON is baseline/mandatory on AArch64, so it always exercises the NEON kernel.
Neither CI job's hardware is ever missing SIMD support, so the scalar decode path (used in production whenever the daemon runs on a CPU without AVX2 — e.g. older/budget x86 instances, some virtualization/sandboxing that masks CPU features, or any non-x86/non-ARM target) is compiled but never run against the reference base64 decoder by base64_differential.rs or the kernel_routes canonical-base64 test. A divergence that exists only in the scalar implementation (e.g. a different acceptance rule for non-canonical padding or trailing bits) would silently corrupt or reject artifact pages in production without ever failing CI.
Failure scenario: the service runs on a host whose CPU lacks AVX2 (or the dispatcher's feature probe conservatively picks scalar), base64-simd's scalar decoder accepts (or rejects) an input where base64::engine::general_purpose::STANDARD disagrees — e.g. a non-canonical padding pattern the scalar path fails to reject — and the ingest handler either stages corrupted bytes as a page or spuriously rejects a valid page, while every CI signal (including this new differential suite) stays green because it only ever runs the SIMD kernels.
Consider forcing the scalar path in at least one CI job (many SIMD-dispatch crates expose a way to disable feature detection, e.g. via an env var or a #[cfg]-gated test build) so the fallback used by real, non-AVX2/non-NEON hardware gets the same reference-decoder differential coverage as the SIMD kernels.
There was a problem hiding this comment.
Confirmed, and this is the gap I flagged in the arm64 thread: ubuntu-latest has AVX2 and NEON is mandatory on AArch64, so neither runner reaches decode_fallback. The PR merged before this comment landed, so it goes to a follow-up rather than this branch.
The mechanism is concrete: vsimd's detect feature (on by default through base64-simd's default = ["std", "detect"]) is what enables runtime dispatch. Without it, vsimd::dispatch! resolves from cfg(target_feature) at compile time, and a baseline x86-64 build resolves straight to the scalar fallback. So the follow-up is: base64-simd = { version = "0.8", default-features = false, features = ["std"] }, an mc-module feature base64-detect = ["base64-simd/detect"] in default so the shipped binary keeps runtime dispatch, and one extra CI step cargo nextest run -p mc-module --no-default-features --test base64_differential that runs the scalar path against the reference decoder on x86. Tracked as magic-context-xmrb.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (5 files)
Reviewed by gemini-3.8-flash · Input: 181.3K · Output: 41.4K · Cached: 1.4M |
1 participant
The scalar
base64decoder ran at 1.6 GB/s and was 40% of a full 16 MiB page ingest; SHA-256 (SHA-NI) was the rest.base64-simddispatches at runtime to AVX2 or NEON with a scalar fallback (notarget-cpu=nativedependency) and decodes the standard alphabet about 5× faster. A one-off local differential over 200k mutated encodings found no input wherebase64_simd::STANDARDandbase640.22STANDARDdiffer in acceptance or output;tests/base64_differential.rscommits that check as three proptest properties (5,632 cases per run, ~4 s) so CI re-verifies it on everybase64-simdbump.base64stays as a dependency for encoding in tests. New crates:base64-simd0.8,vsimd,outref.Measured (marginal): page/16MiB 18.3 → 11.3 ms, page/256k 310 → 213 µs, page/4k 38 → 37 µs. Cumulative 1.62× / 1.46× / 1.04×; 3 improved, 0 regressed.
Stack (9/13): base
perf/kr-08-alignment-reachable-load→ headperf/kr-09-base64-simd.Method. Numbers come from
crates/mc-module/benches/kernel_routes.rs(added in the first PR of this stack): a baseline binary and a candidate binary run as separate processes in 10 ABBA blocks pinned to one core; each cell ismedian_baseline / median_candidatewith a paired bootstrap 95% CI over the blocks. Baseline is the suite commit onstack/kernel-routes-03-routesbefore any optimization; ratios are therefore cumulative through this PR, and the marginal effect of this PR alone is called out. A/A control on the same host: all CIs include 1.0, half-widths about 1%. Host: AMD EPYC 9R14, SQLite 3.51.3 (bundled).Guard.
cargo test -p mc-module --features test-support --test kernel_routes(46),cargo test -p mc-module --test base64_differential(3),cargo test -p mc-kernel --features test-support(all suites),cargo clippy -p mc-module -p mc-kernel --all-targets -- -D warnings.tests/kernel_routes.rsgains one test that drives malformed base64 throughingest.pageon a live upload;tests/base64_differential.rsis new; both are now run by CI. The wire contract, receipts, and serving semantics are unchanged.