perf(canvas): use FxHash for NodeId-keyed caches#600
Conversation
Replace SipHash (std HashMap) with a FxHash-style multiplicative hasher for all internal rendering caches keyed by NodeId (u64). These caches have trusted-input keys only, so DoS-resistant hashing is unnecessary. The fast hasher reduces per-lookup cost from ~25ns to ~3ns, yielding 5-15% improvement on pan/zoom operations (Criterion-verified). Affected caches: geometry, picture, vector_path, atlas, atlas_set, compositor, painter draw_order, and scene node_map. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR introduces a custom hashing implementation ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/grida-canvas/src/runtime/scene.rs (1)
1395-1395:⚠️ Potential issue | 🟡 MinorFix the
typosfailure before merge.CI reports a spelling failure at Line 1395 (
LOD→LOAD). Please update the text (or reword to avoid the token) so the pipeline passes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/runtime/scene.rs` at line 1395, Update the comment at the invalidated zoom image cache location to fix the typos CI failure by replacing or rewording the token "LOD" to "LOAD" (or another wording that avoids the flagged token) so the comment no longer triggers the spelling check; locate the comment near the invalidate zoom image cache logic in scene.rs (around the comment "// Invalidate zoom image cache on stable frames (always full-quality).") and adjust the text accordingly.
🧹 Nitpick comments (1)
crates/grida-canvas/src/cache/fast_hash.rs (1)
26-31: Minor observation:write()fallback has weak initial mixing.The first byte iteration computes
0 * K + b = b, which provides no mixing. This is acceptable sincewrite()is a fallback path—all intended key types (NodeId,SlotId, tuple keys) invokewrite_u64()directly via theirHashimplementations. No change needed.💡 Optional: Seed with non-zero value for stronger fallback
#[derive(Default)] pub struct NodeIdHasher { - hash: u64, + hash: u64 = 0xcbf29ce484222325, // FNV offset basis }Note: Rust stable doesn't support default field values yet (requires nightly), so this would need a manual
Defaultimpl if desired.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/cache/fast_hash.rs` around lines 26 - 31, The fallback write() in fast_hash.rs starts from zero so the first byte becomes un-mixed (hash = b); modify the implementation of write() (and the hasher's constructor/Default if present) to seed the internal state with a non-zero constant or perform an initial mix before the loop so the first byte is combined (refer to the write() method in this file and the hasher struct/Default impl), ensuring the change is localized to the fallback path and does not affect callers that already use write_u64().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/grida-canvas/src/runtime/scene.rs`:
- Line 1395: Update the comment at the invalidated zoom image cache location to
fix the typos CI failure by replacing or rewording the token "LOD" to "LOAD" (or
another wording that avoids the flagged token) so the comment no longer triggers
the spelling check; locate the comment near the invalidate zoom image cache
logic in scene.rs (around the comment "// Invalidate zoom image cache on stable
frames (always full-quality).") and adjust the text accordingly.
---
Nitpick comments:
In `@crates/grida-canvas/src/cache/fast_hash.rs`:
- Around line 26-31: The fallback write() in fast_hash.rs starts from zero so
the first byte becomes un-mixed (hash = b); modify the implementation of write()
(and the hasher's constructor/Default if present) to seed the internal state
with a non-zero constant or perform an initial mix before the loop so the first
byte is combined (refer to the write() method in this file and the hasher
struct/Default impl), ensuring the change is localized to the fallback path and
does not affect callers that already use write_u64().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a08578fc-7d48-49e0-bf5b-4ca84a25996e
📒 Files selected for processing (11)
crates/grida-canvas/src/cache/atlas/atlas.rscrates/grida-canvas/src/cache/atlas/atlas_set.rscrates/grida-canvas/src/cache/compositor/cache.rscrates/grida-canvas/src/cache/fast_hash.rscrates/grida-canvas/src/cache/geometry.rscrates/grida-canvas/src/cache/mod.rscrates/grida-canvas/src/cache/picture.rscrates/grida-canvas/src/cache/vector_path.rscrates/grida-canvas/src/painter/painter.rscrates/grida-canvas/src/runtime/scene.rscrates/grida-canvas/tests/compositor_effects.rs
Summary
std::collections::HashMap(SipHash) with a FxHash-style multiplicative hasher for all internal rendering caches keyed byNodeId(u64)Affected caches
geometry,picture,vector_path,atlas,atlas_set,compositor,painterdraw_order,scenenode_mapTest plan
cargo test -p cg— all 281+ tests pass (no correctness regressions)cargo check -p cg -p grida-canvas-wasm -p grida-dev— all 3 crates compilecargo bench -p cg --bench bench_camera— confirm improvement on zoom/pan benchmarks🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Tests