Drop the external blake3 dependency; use the in-tree module - #269
Conversation
Rung 3a of the SIMD ladder, swapped. The in-tree BLAKE3 landed in #268 but nothing called it; the 15 call sites across seal, merkle_tree, plane, vsa, spo_bundle, crystal_encoder, compression_curves and deepnsm still reached the external crate. They now reach crate::hpc::blake3. Why, concretely: even at default-features = false, features = ["pure"], the crate shipped rust_sse2.rs / rust_sse41.rs / rust_avx2.rs -- a second SIMD surface beside ndarray::simd, which is what the matryoshka pattern exists to prevent. The Cargo.toml comment block named this exact gap and said "Tracked, not done here." It also breaks a cycle: ndarray -> blake3 meant blake3 could never itself be built on ndarray::simd without cargo seeing ndarray -> blake3 -> ndarray. Implementation is one `use super::blake3;` per module rather than 15 rewritten paths -- the in-tree API is shape-compatible (hash, Hasher::{new, new_keyed, new_derive_key, update, finalize, finalize_xof}, Hash::as_bytes -> &[u8; 32]). spo_bundle's use is inside #[cfg(test)] and takes `use crate::hpc::blake3;` there rather than leaning on the hpc subtree's blanket unused_imports allow. Lockfile loses blake3, constant_time_eq, arrayref and arrayvec. The nostd matrix is untouched: every call site is under `pub mod hpc`, itself #[cfg(feature = "std")]. Cost, measured before the decision and accepted with it: 1.25-1.39x across the input sizes this crate hashes (16 B 1.34-1.39x, 256 B 1.25-1.29x, 2 KB 1.30x). The 4.7-4.9x figure at 64 KB is real but off this crate's curve -- no call site here hashes bulk input. That gap is upstream's hash_many, which has no in-tree counterpart. The A/B bench needs both implementations present and is unbuildable from this commit forward, by its own design note. The measurement is recorded, not reproducible. Verified: cargo check --features std; 1734 hpc tests passing, 0 failed; clippy --all-targets -D warnings clean. src/hpc/blake3_test_vectors.json confirmed byte-identical to upstream's test_vectors.json, so the correctness tests run against genuine official vectors. Claude-Session: https://claude.ai/code/session_01VdfbkUCBbtZhy3yjSfCDHp
The lock carried an external crates.io blake3 v1.8.3, reachable only via `ndarray` -- stale from before the dependency was dropped. Re-resolving removes it, along with arrayref and bincode. Re-resolution was blocked by a pre-existing conflict unrelated to blake3: burn-backend at rev 9b2b671 requires rand ^0.10.1 while the lock pinned 0.10.0. Bumped to 0.10.2 to unblock. That surfaced the real problem. The lockfile was resolving burn-backend, burn-std and burn-ir to `git+https://github.com/tracel-ai/burn.git` rev ed72d2b -- UPSTREAM -- while crates/burn/Cargo.toml has declared `https://github.com/AdaWorldAPI/burn.git` rev 9b2b671 for all three. The lock had drifted behind the manifest and was silently building against upstream, which is the fork-rule violation the workspace treats as P0. After this refresh: 0 tracel-ai entries, 3 AdaWorldAPI. This went unnoticed because crates/burn is excluded from the root workspace and is not built by any CI workflow, so nothing ever re-resolved it. Not verified by a build: burn pulls cubecl + wgpu and building it here is not cheap. The resolution itself is coherent (cargo metadata succeeds) and now matches the declared manifests, which the previous state did not. Claude-Session: https://claude.ai/code/session_01VdfbkUCBbtZhy3yjSfCDHp
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR removes the external ChangesBLAKE3 dependency migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_1e8dcb3c-e479-4419-9138-10e6371fe968) |
![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: 9b2dba8c6d
ℹ️ 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".
| # ===================================================================== | ||
| # blake3 — DEPENDENCY OF `std` FEATURE (NOT `hpc-extras`). READ BEFORE | ||
| # TOUCHING. | ||
| # blake3 — NO LONGER A DEPENDENCY. Do not re-add it. |
There was a problem hiding this comment.
After removing the external dependency here, the crate-level rustdoc in src/lib.rs:488-497 and the dependency section in README.md:211-226 still state that enabling std pulls in the external blake3 crate and that the HPC modules import it directly. This now gives downstream users incorrect feature and dependency information, so those public descriptions should be updated to describe the in-tree implementation.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.claude/knowledge/blake3-in-tree-measured.md:
- Around line 6-9: Update the retained migration scope statement near lines
37–40 to report 15 call sites, matching the 15 call sites stated in the opening
decision summary and keeping the document internally consistent.
In `@Cargo.toml`:
- Around line 160-171: Update the BLAKE3 comment block in Cargo.toml to describe
src/hpc/blake3.rs as a portable pure-Rust reference transcription with no SIMD,
and remove the inaccurate claims about ndarray::simd::U32x16 and ChaCha-derived
ARX code. Reframe the dependency-cycle explanation as enabling a future
ndarray::simd-backed BLAKE3 implementation while preserving the rationale for
removing the external blake3 dependency.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 751d9163-134d-4400-9ab1-e737f584a9e3
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockcrates/burn/Cargo.lockis excluded by!**/*.lockcrates/wasm-simd-parity/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.claude/blackboard.md.claude/knowledge/blake3-in-tree-measured.mdCargo.tomlsrc/hpc/compression_curves.rssrc/hpc/crystal_encoder.rssrc/hpc/deepnsm.rssrc/hpc/merkle_tree.rssrc/hpc/plane.rssrc/hpc/seal.rssrc/hpc/spo_bundle.rssrc/hpc/vsa.rs
Four review findings, all valid, all mine. Codex P2 -- src/lib.rs and README.md still told downstream users that enabling `std` pulls the external blake3 crate and that the hpc modules import it directly. That was true before this branch and false after it, which is worse than saying nothing: it is wrong feature and dependency information on the crate's two most-read public surfaces. Both rewritten to state that BLAKE3 is in-tree and that blake3 / constant_time_eq / arrayref / arrayvec are absent from the dependency graph at every feature combination. CodeRabbit -- the Cargo.toml comment I wrote in the first commit claimed the in-tree module is "built on ndarray::simd::U32x16 (it is a ChaCha-derived u32 ARX kernel)". That is simply false. src/hpc/blake3.rs says so in its own header: "a portable-only (no SIMD, no unsafe) transcription" of the reference implementation. Its only `simd` reference is crate::simd_ops::array_chunks, a slicing helper. Corrected, and the cycle paragraph now says dropping the dep enables a FUTURE ndarray-backed implementation and the AdaWorldAPI/BLAKE3 fork -- not this module, which is portable. CodeRabbit -- the knowledge doc said "14 call sites" in one place and the swap record said 15. Both were right about different things: 14 calls plus one type position (merkle_tree::truncate_hash's `&blake3::Hash` parameter). Made explicit rather than reconciled to one number, since the distinction is the reason the counts differed. Verified: cargo build --features std clean, fmt clean, and no "pulls blake3" claim survives anywhere in README.md, src/lib.rs or Cargo.toml. Claude-Session: https://claude.ai/code/session_01VdfbkUCBbtZhy3yjSfCDHp
2 participants
Rung 3a of the SIMD ladder, swapped. The in-tree BLAKE3 landed in #268 but nothing called it — the 15 call sites still reached the external crate. They now reach
crate::hpc::blake3, and the dependency is gone.Why
Even at
default-features = false, features = ["pure"], the crate shippedrust_sse2.rs/rust_sse41.rs/rust_avx2.rs— a second SIMD surface besidendarray::simd, which is what the matryoshka pattern exists to prevent. The Cargo.toml comment block named this exact gap and closed with "Tracked, not done here." This is it being done.It also breaks a cycle:
ndarray → blake3meant blake3 could never itself be built onndarray::simdwithout cargo seeingndarray → blake3 → ndarray. That edge is now gone, which is what unblocks the companion PR on the BLAKE3 fork.Shape of the change
One
use super::blake3;per module rather than 15 rewritten paths — the in-tree API is shape-compatible (hash,Hasher::{new, new_keyed, new_derive_key, update, finalize, finalize_xof},Hash::as_bytes → &[u8; 32]). The diff is 10 added lines across 8 source files.spo_bundle's use is inside#[cfg(test)]and takesuse crate::hpc::blake3;there, rather than leaning on thehpcsubtree's blanketunused_importsallow.Cargo.locklosesblake3,constant_time_eq,arrayrefandarrayvec. The nostd matrix is untouched — every call site is underpub mod hpc, itself#[cfg(feature = "std")].Cost — measured before the decision, accepted with it
Every call site in this crate sits in the small-input band. The 64 KB row is real but off our curve — nothing here hashes bulk input. That gap is upstream's
hash_many, which has no in-tree counterpart.The A/B bench needs both implementations present and is unbuildable from this commit forward, by its own design note. The measurement is recorded, not reproducible.
Second commit:
crates/burnlockfileThe lock carried an external crates.io
blake3v1.8.3 reachable only viandarray— stale. Re-resolving was blocked by an unrelated pre-existing conflict (burn-backendneedsrand ^0.10.1, lock pinned 0.10.0), and clearing that surfaced the real problem:The lockfile was resolving
burn-backend,burn-stdandburn-irtogit+https://github.com/tracel-ai/burn.git— upstream — whilecrates/burn/Cargo.tomlhas declaredAdaWorldAPI/burnrev9b2b671for all three. The lock had drifted behind the manifest and was silently building against upstream. After the refresh: 0tracel-aientries, 3AdaWorldAPI.This went unnoticed because
crates/burnis excluded from the root workspace and no CI workflow builds it, so nothing ever re-resolved it.Verification
cargo check --features stdcleanclippy --all-targets -D warningscleansrc/hpc/blake3_test_vectors.jsonconfirmed byte-identical (31,922 B) to upstream'stest_vectors.jsoninAdaWorldAPI/BLAKE3v1.8.5 — the correctness tests run against genuine official vectors, now verified rather than assertedNot verified by a build:
crates/burnitself. It pulls cubecl + wgpu and building it here is not cheap. The resolution is coherent (cargo metadatasucceeds) and now matches the declared manifests, which the previous state did not.Companion
AdaWorldAPI/BLAKE3— removes all C/asm/FFI/intrinsic backends and adds anndarray::simdU32x16backend. Merge this PR first: that one's dependency on ndarray is only sound once this removal is on master.Generated by Claude Code
Summary by CodeRabbit
Refactor
Documentation