perf: remove the SIMD after measuring what it was worth - #1262
Conversation
…able_simd
Proof of concept for replacing `std::simd` with fearless_simd across the
workspace, starting with the smallest of the three SIMD sites.
The motivation is not throughput, it is the toolchain: `portable_simd` is
what pins the whole workspace to nightly, and `wacore-appstate/simd` was
part of that chain. fearless_simd resolves the SIMD level at runtime on
stable (MSRV 1.89, below our 1.94), so this crate now builds and tests with
SIMD enabled on stable 1.94.1.
`wacore-appstate/simd` therefore no longer chains to `wacore-binary/simd`,
which is still `portable_simd` and still nightly-only. `wacore/simd`
enables both, so the feature's meaning is unchanged for consumers.
Measured on this machine (Level::new() detects AVX-512):
- appstate_benchmark/bench_lthash_subtract_then_add_812
fastest 992.8 µs -> 990.5 µs, i.e. parity.
- demo binary: +2304 B stripped, +2048 B .text, against PR budgets of
64 KiB and 32 KiB.
- fearless_simd adds one crates.io dependency with no transitive deps.
Isolating the lane math from the HKDF it sits behind (812 operands x 128 B)
explains the flat end-to-end number and is worth recording, because it
bears on where SIMD is worth having at all:
scalar 3.89 µs
portable_simd 3.96 µs
fearless_simd, dispatch! per operand 5.09 µs
fearless_simd, dispatch! hoisted 3.84 µs
Two things follow. The lane math is ~0.4% of LTHash cost -- HKDF dominates
-- and LLVM already auto-vectorizes the scalar loop as well as either SIMD
backend does, so neither backend was buying anything here. And the gap
between the two fearless_simd rows is ~1.5 ns per `dispatch!`, not codegen
quality: hoisted, it matches scalar. Dispatch belongs above a hot loop, not
inside one. That constraint shapes the decoder/encoder port, where the win
is real -- those `swizzle_dyn` calls do not currently lower to `pshufb` on
the default x86-64 baseline.
Dispatch is left per operand here: 1.5 ns against ~1250 ns of HKDF per
operand is invisible, and hoisting it would monomorphize HKDF once per
SIMD level for no gain.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QsMHsab1sPM3nZRbm6rBtf
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR moves runtime-dispatched SIMD to LTHash, removes SIMD from the binary codec, adds scalar lookup-table packing and decoding, and updates feature wiring plus stable and Miri CI coverage. ChangesSIMD and scalar codec paths
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant LTHash
participant SIMD_LEVEL
participant pointwise_chunks
participant fearless_simd
LTHash->>SIMD_LEVEL: Read cached SIMD level
LTHash->>pointwise_chunks: Dispatch 16-byte chunk arithmetic
pointwise_chunks->>fearless_simd: Process SIMD lanes
fearless_simd-->>LTHash: Return processed bytes
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| wacore/binary/src/encoder.rs | Replaces SIMD and match-based packed encoding with bounded lookup tables, aggregate validation, and exhaustive table-equivalence coverage. |
| wacore/binary/src/decoder.rs | Removes SIMD packed decoding and consistently uses the existing scalar lookup-table path. |
| wacore/appstate/src/lthash.rs | Consolidates LTHash lane math into a scalar little-endian implementation with independent boundary and overflow tests. |
| wacore/binary/Cargo.toml | Removes the obsolete public simd feature from the binary codec crate. |
| wacore/appstate/Cargo.toml | Removes the app-state SIMD feature wiring now that LTHash arithmetic is scalar. |
| .github/workflows/main.yml | Updates stable CI to test the default feature configurations that now compile on the declared MSRV. |
| .github/workflows/miri.yml | Removes the redundant no-default-features Miri leg after packed codec paths were unified. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Protocol strings] --> B[Table-driven packed encoder]
B --> C[WhatsApp binary wire bytes]
C --> D[Table-driven packed decoder]
D --> E[Decoded node values]
F[App-state value MACs] --> G[HKDF expansion]
G --> H[Scalar little-endian lane arithmetic]
H --> I[LTHash accumulator]
Reviews (8): Last reviewed commit: "refactor: drop the `simd` feature and th..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
coderabbitai
Bot
added
the
size-increase-ok
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wacore/appstate/src/lthash.rs`:
- Line 86: Add a test-only independent scalar reference for pointwise_chunks,
then update the consistency test around the dispatch! call to compare its result
against that reference rather than comparing two dispatched executions. Exercise
varied 16-byte chunks for both add and subtract, including wrapping arithmetic
boundaries, while preserving the existing SIMD-feature gating.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4fa40ecb-c1dc-4ab3-a5fe-ce5605968d36
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.tomlwacore/Cargo.tomlwacore/appstate/Cargo.tomlwacore/appstate/src/lib.rswacore/appstate/src/lthash.rs
💤 Files with no reviewable changes (1)
- wacore/appstate/src/lib.rs
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
…erence `test_simd_determinism_and_consistency` called `perform_pointwise_with_overflow` twice and compared the results. Both calls take the same path, so despite the name it never compared SIMD against scalar; it only proved the path was deterministic. A lane-conversion or wrapping bug would have been invisible to it. Split it in two. The round-trip property it did check keeps its own test under a name that says so. Alongside it, a straight-line scalar reference written for obviousness rather than speed, compared against the real function over sizes that straddle the 16-byte chunk boundary (under one chunk, exactly one, chunk-plus-tail, several) with inputs seeded onto the wrap boundaries in both directions. Verified by mutation: flipping the SIMD path's lane store to `to_be_bytes` fails the new test. The old one passed with that same mutation applied. Raised in review on #1262. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QsMHsab1sPM3nZRbm6rBtf
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Changes Cargo feature contract: wacore-appstate/simd no longer enables wacore-binary/simd, and introduces a new third-party dependency (fearless_simd). Need human review to confirm downstream compatibility and dependency/supply-chain acceptability.
Re-trigger cubic
Step 2 was meant to port `wacore-binary`'s decoder and encoder to
fearless_simd, on the theory that their `swizzle_dyn` calls were losing a
`pshufb` on the default x86-64 baseline and that runtime dispatch would win
it back. Measuring first killed that theory, so this deletes the vectors
instead of porting them.
Wall clock on the bench host swings ~18% between identical runs, so the
numbers below are callgrind instruction counts over 20k marshal/unmarshal
rounds of an ack node, split by message-id length. 20 characters is the
common id; 31 is where the decoder's chunk loop first engages at all, since
it needs 16 packed bytes.
scalar portable_simd this change
encode, 20 chars 74.15M 69.93M 68.95M
encode, 32 chars 83.05M 74.83M 75.99M
decode, 20 chars 29.10M 30.41M 29.09M
decode, 32 chars 32.88M 36.07M 32.87M
The decoder's vector path was slower than the table it sat in front of, at
every length. Rebuilding with `-Ctarget-cpu=x86-64-v2`, so the shuffles
really did lower to `pshufb`, narrowed the 32-character loss from 9.7% to
7.0% and never turned it into a win: `HEX_PAIRS[byte]` is a single 2-byte
load, and shuffle/interleave/store does not beat that.
The encoder's vector path was a real win, but not against a fair opponent.
It was competing with two `match` ladders reached through a `fn` pointer.
Given the same 256-entry table treatment the decoder already had, scalar
takes the common 20-character case outright and gives up 1.5% at 32.
What that buys, beyond the instruction counts: `wacore-binary` no longer
needs `portable_simd`, so its `simd` feature is gone and
`cargo +stable build -p wacore` now works with default features for the
first time. Combined with the appstate change earlier in this branch, the
demo binary is 5.1 KiB smaller than main rather than 2.25 KiB larger.
CI follows: the Miri matrix drops its scalar leg, which no longer names a
distinct code path, and the stable job tests default features -- the
configuration that ships and that it could never reach before -- keeping
the scalar build as a compile check for wasm and ESP32.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QsMHsab1sPM3nZRbm6rBtf
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
jlucaso1
changed the title
![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: 44303a8400
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| [features] | ||
| default = ["simd"] | ||
| simd = [] | ||
| serde = ["dep:serde", "compact_str/serde", "smallvec/serde"] |
There was a problem hiding this comment.
simd feature as an alias
Removing simd from this published crate's feature table breaks dependency resolution for downstream manifests that explicitly use wacore-binary = { ..., features = ["simd"] }, even though retaining it as an empty compatibility feature would have no runtime cost now that the SIMD implementation is gone. Keep a no-op simd = [] entry so existing consumers can upgrade without changing their manifests.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…solve `wacore-binary` is published, and a manifest that names `features = ["simd"]` fails to resolve against a crate that has no such feature. The error names the consumer's manifest rather than this change, which makes it a poor way to learn the feature is gone. The feature does nothing and stays out of `default`. Raised independently in review on #1262 by two reviewers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QsMHsab1sPM3nZRbm6rBtf
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
coderabbitai
Bot
removed
size-increase-ok
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Auto-approved: Bounded, behavior-preserving refactor: SIMD codec paths become lookup-table scalar code, LTHash moves to fearless_simd on stable, and CI/tests follow. No API, security, data, or operational tradeoff is left for human judgment.
Re-trigger cubic
The lookup tables left the 32-character case 1.5% behind the vectors they
replaced. The cause was the per-pair `assert!`: a conditional branch in the
loop body, which stops LLVM unrolling it.
The check does not need to be there. Legal table entries are 0..=15 and
`PACK_INVALID` is 0xFF, so ORing every lookup into an accumulator and
testing its high nibble once, after the loop, detects exactly the same
inputs. The odd-length byte folds into the same accumulator, and the test
still runs before anything reaches the writer, so an invalid character
cannot escape onto the wire any more than it could before.
Callgrind, 20k marshals of an ack node, against the same baselines:
portable_simd branchy tables this change
encode, 20 chars 69.93M 68.95M 68.09M
encode, 32 chars 74.83M 75.99M 74.71M
That closes the 32-character gap and turns it into a small win, so scalar
is now ahead of the vectors at both lengths rather than trading. Decode is
untouched and measures unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QsMHsab1sPM3nZRbm6rBtf
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: The scalar codec rewrite and feature-semantics change need human sign-off: encoder/validation diffs are truncated so behavior equivalence isn't fully visible, and wacore-binary's published simd feature becomes a no-op outside default.
Re-trigger cubic
I had kept the feature but moved it out of `default`, which cargo-semver-checks flags as `feature_not_enabled_by_default`: downstream crates relying on default features lose it silently. That defeats the point of keeping the name at all. The feature gates nothing, so its presence in `default` costs nothing and leaves the published feature surface exactly where it was. The workspace is unaffected either way -- every internal edge takes wacore-binary with `default-features = false`. This clears the one semver finding this branch introduced. The `enum_variant_added` on `BinaryError::UnexpectedFormatByte` that the same job reports predates the branch and comes from main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QsMHsab1sPM3nZRbm6rBtf
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Encoder diff is truncated, so the new table-driven packing path's behavior equivalence to the old match-ladder cannot be verified; human review should confirm packing and error semantics.
Re-trigger cubic
`HEX_ENC` and `NIBBLE_ENC` were derived from the `match` ladders that `write_packed_bytes` used to call, but nothing held them to that. A review pass on #1262 flagged exactly this: the diff shows a new table-driven path with no visible proof it accepts and rejects the same bytes as the old one. The ladders now live in the test module as the specification, checked exhaustively over all 256 byte values: same accepted set, same nibble for every accepted byte, same rejected set. `None` in the reference is the case the old code panicked on and the tables mark with `PACK_INVALID`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QsMHsab1sPM3nZRbm6rBtf
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Auto-approved: Benchmark-driven performance refactor replacing SIMD paths with faster scalar lookups; LTHash math moves to fearless_simd with behavior preserved and tests/CI updated. No API, schema, or operational tradeoffs require sign-off.
Re-trigger cubic
The packed codec lost its vectors earlier in this branch because scalar lookup tables measured faster. That left LTHash as the only SIMD in the tree, and it does not earn its keep either: over an 812-MAC batch the feature moves the total by 0.28% (831.93M instructions scalar, 829.63M with SIMD), because HKDF above it dominates and LLVM already auto-vectorizes the lane loop about as well as the intrinsics did. Isolated, the lane math alone is scalar 3.89 us, portable_simd 3.96 us, fearless_simd 3.84 us over 812 operands -- roughly 0.4% of LTHash's cost. A dependency and a feature flag across four manifests is a poor trade for that, and we are pre-1.0, so the feature goes rather than lingering as a compatibility no-op. What this leaves: no `simd` feature anywhere, no fearless_simd, no `portable_simd`, and nothing in the tree that needs nightly to build. The demo binary is 7.8 KiB smaller than main, against 5.09 KiB before this commit. The LTHash test that compared the SIMD path against a scalar reference would now be comparing the implementation with a copy of itself, so its reference is rewritten to reach the same answer a different way: lanes assembled by hand from byte positions, arithmetic in `u32` and masked, sharing neither `from_le_bytes` nor `wrapping_*` with the code under test. The sizes it sweeps still straddle the 16-byte boundary a vectorised implementation would chunk on, so the coverage holds if one comes back. Also drops the stale `% 2` workaround in the length assert; `rust-version` is 1.94 and `is_multiple_of` stabilised in 1.87. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QsMHsab1sPM3nZRbm6rBtf
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
jlucaso1
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 039912ea25
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
0 issues found across 9 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: The encoder's new table-driven packed path is truncated, so wire-compatible encoding/validation can't be verified; removing the public simd feature from manifests is also a breaking Cargo-feature change. Human review is needed.
Re-trigger cubic
2 participants
This began as an evaluation of fearless_simd as a stable-Rust replacement for
portable_simd. The port worked, but measuring the three SIMD sites showed there was almost nothing for it to do, so the end state is no SIMD at all: nosimdfeature, no fearless_simd, noportable_simd, and nothing in the tree that needs nightly to build.Wall clock on the bench host swings ~18% between identical runs, so every number below is a callgrind instruction count.
The packed codec was slower with vectors than without
wacore-binary's decoder and encoder usedswizzle_dyn, which emits nopshufbon the default x86-64 baseline (confirmed: 0 at baseline, 8 with-Ctarget-cpu=x86-64-v2). The plan was to recover that with runtime dispatch. Measuring first killed the plan. 20k marshal/unmarshal rounds of an ack node, by message-id length:The decoder's vector path lost to the table in front of it at every length. Building with real
pshufbnarrowed the 32-char loss from 9.7% to 7.0% and never turned it into a win:HEX_PAIRS[byte]is one 2-byte load, and shuffle/interleave/store does not beat that. It also needed a 31-character string before its chunk loop engaged at all, and instrumenting realistic nodes showed 0 of 1400 decode calls reaching it.The encoder's vector path did win, but against two
matchladders reached through afnpointer. Given the same 256-entry table the decoder already had, plus moving the validity test out of the loop into an OR accumulator checked once (the per-pair branch was blocking LLVM from unrolling), scalar comes out ahead at both lengths.LTHash did not earn its keep either
That left the app-state LTHash lane math as the only SIMD. Over an 812-MAC batch the feature is worth 0.28% (831.93M instructions scalar, 829.63M with SIMD). Isolated, the lane math is scalar 3.89 us, portable_simd 3.96 us, fearless_simd 3.84 us over 812 operands — about 0.4% of LTHash's cost, because HKDF above it dominates and LLVM already auto-vectorizes the loop about as well as the intrinsics did.
A dependency plus a feature flag across four manifests is a poor trade for 0.28%. Pre-1.0, so the feature goes rather than lingering as a no-op.
One number worth keeping from the fearless_simd experiment:
dispatch!costs ~1.5 ns per call. Hoisted above a loop it matches scalar; called per small item it does not. That is the constraint to remember if SIMD is ever revisited here.Result
The demo binary is 7.8 KiB smaller than main (stripped;
.text-7.6 KiB), withwacore_binary.textdown ~6.9 KiB, and the dependency count is back to main's.cargo +stable buildnow works with default features across the workspace. CI follows: the Miri matrix drops its scalar leg, which no longer names a distinct code path, and the stable job tests what ships instead of the only half it could previously reach.rust-toolchain.tomlstill pins nightly, but now only for-Zshare-genericsand lld/ICF in.cargo/config.toml, not for any language feature. Dropping those measured +667 KiB stripped (+6.5%), so that is a separate call.Verification
wacore-binary,wacore,wacore-appstatesuites pass, including proptest andpacked_equivalencecargo clippy --all-targets -- -D warningscleancargo +stable teston the touched crates at 1.94.1encode_tables_match_the_ladders_they_replacedholdsHEX_ENC/NIBBLE_ENCagainst thematchladders they were derived from, exhaustively over all 256 byte valuesu32arithmetic and masking), over sizes straddling the 16-byte boundary and inputs seeded onto the wrap edgesInstruction counts are single-host and the detected SIMD level was AVX-512; a host topping out at SSE4.2 was not measured.