perf(cg): unify picture cache variants for effects-free nodes#614
Merged
softmarshmallow merged 1 commit intomainfrom Mar 30, 2026
Merged
perf(cg): unify picture cache variants for effects-free nodes#614softmarshmallow merged 1 commit intomainfrom
softmarshmallow merged 1 commit intomainfrom
Conversation
When a node has no effects (blur, shadow, noise, etc.), the SkPicture recorded under the reduced-quality policy (unstable/interaction frames) is identical to the full-quality policy (stable/settle frames). Store such nodes under variant_key=0 regardless of the active render policy, so the settle pass reuses pictures already cached during interaction instead of re-recording them. On yrr-main.perf.grida (135K nodes, 0 effects), this eliminates ~800µs of redundant LayerEntry clones + SkPicture recordings per settle frame, yielding 20-40% settle-time reduction across pan/zoom scenarios. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
6 Skipped Deployments
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
WalkthroughThe changes implement a cache unification optimization where render policies with only effect-related differences can reuse cached pictures from nodes that have no effects, by conditionally switching their variant key to zero during cache lookups. Changes
Sequence DiagramsequenceDiagram
participant Painter as Painter
participant Policy as RenderPolicy
participant Layer as Layer
participant Cache as SceneCache
rect rgba(100, 150, 200, 0.5)
Note over Painter: Construction Phase
Painter->>Policy: is_effect_only_variant()?
Policy-->>Painter: can_unify_variant (bool)
end
rect rgba(150, 100, 200, 0.5)
Note over Painter: Draw Loop Phase
Painter->>Layer: effects_empty()?
Layer-->>Painter: has_effects (bool)
Painter->>Painter: compute effective_key<br/>(switch to 0 if can_unify && !has_effects)
end
rect rgba(200, 150, 100, 0.5)
Note over Painter: Cache Retrieval Phase
Painter->>Cache: get_node_picture_variant(effective_key)
Cache-->>Painter: cached_picture or None
Painter->>Painter: draw cached_picture or render layer
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
141-141:⚠️ Potential issue | 🟡 MinorFix the typo-lint failure reported by CI.
typosflags Line 141 (LOD). If this acronym is intentional, add it to the typo allowlist; otherwise rename to unblock the pipeline.🤖 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 141, The CI typo-lint flagged the acronym "LOD" in the doc comment, so either add "LOD" to the typos allowlist or rename the acronym to its full form to satisfy the linter; locate the doc comment in the Scene struct/file (the line containing "LOD" in crates/grida-canvas/src/runtime/scene.rs) and either add "LOD" to the project's typo allowlist configuration or replace "LOD" with the spelled-out phrase (e.g., "level of detail") in that comment to unblock the pipeline.
🧹 Nitpick comments (1)
crates/grida-canvas/src/painter/painter.rs (1)
2275-2280: Consider centralizing the effective variant-key rule.This branch now exists in both cache lookup paths. A tiny helper would make future tweaks to the unification rule less likely to drift.
♻️ Suggested cleanup
+ #[inline] + fn effective_variant_key(&self, layer: &PainterPictureLayer) -> u64 { + if self.can_unify_variant && layer.effects_empty() { + 0 + } else { + self.variant_key + } + } + fn draw_render_commands(&self, commands: &[PainterRenderCommand]) { for command in commands { match command { @@ - let ek = if self.can_unify_variant && layer.effects_empty() { - 0 - } else { - self.variant_key - }; + let ek = self.effective_variant_key(layer); if let Some(pic) = scene_cache.get_node_picture_variant(layer.id(), ek) { self.canvas.draw_picture(pic, None, None); self.cache_hits.set(self.cache_hits.get() + 1); @@ for entry in &list.layers { if let Some(scene_cache) = self.scene_cache { - let ek = if self.can_unify_variant && entry.layer.effects_empty() { - 0 - } else { - self.variant_key - }; + let ek = self.effective_variant_key(&entry.layer); if let Some(pic) = scene_cache.get_node_picture_variant(&entry.id, ek) { self.canvas.draw_picture(pic, None, None); self.cache_hits.set(self.cache_hits.get() + 1);Also applies to: 2604-2609
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/painter/painter.rs` around lines 2275 - 2280, The logic computing the "effective variant key" is duplicated around cache lookups (see uses of self.can_unify_variant, layer.effects_empty(), self.variant_key and scene_cache.get_node_picture_variant); extract that into a small helper method (e.g., fn effective_variant_key(&self, layer: &Layer) -> VariantKey) and replace both branches to call it before calling scene_cache.get_node_picture_variant(layer.id(), ek); ensure the helper implements the rule: return 0 when can_unify_variant && layer.effects_empty(), otherwise return self.variant_key so future changes centralize the unification rule.
🤖 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 141: The CI typo-lint flagged the acronym "LOD" in the doc comment, so
either add "LOD" to the typos allowlist or rename the acronym to its full form
to satisfy the linter; locate the doc comment in the Scene struct/file (the line
containing "LOD" in crates/grida-canvas/src/runtime/scene.rs) and either add
"LOD" to the project's typo allowlist configuration or replace "LOD" with the
spelled-out phrase (e.g., "level of detail") in that comment to unblock the
pipeline.
---
Nitpick comments:
In `@crates/grida-canvas/src/painter/painter.rs`:
- Around line 2275-2280: The logic computing the "effective variant key" is
duplicated around cache lookups (see uses of self.can_unify_variant,
layer.effects_empty(), self.variant_key and
scene_cache.get_node_picture_variant); extract that into a small helper method
(e.g., fn effective_variant_key(&self, layer: &Layer) -> VariantKey) and replace
both branches to call it before calling
scene_cache.get_node_picture_variant(layer.id(), ek); ensure the helper
implements the rule: return 0 when can_unify_variant && layer.effects_empty(),
otherwise return self.variant_key so future changes centralize the unification
rule.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 85919148-6008-4954-a2af-372f393a5d25
📒 Files selected for processing (4)
crates/grida-canvas/src/painter/layer.rscrates/grida-canvas/src/painter/painter.rscrates/grida-canvas/src/runtime/render_policy.rscrates/grida-canvas/src/runtime/scene.rs
This was referenced Mar 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
variant_key=0regardless of active render policy, so settle reuses pictures already cached during interaction instead of re-recording them.PainterPictureLayer::effects_empty()andRenderPolicy::is_effect_only_variant()to detect when variant unification is safeprefill_picture_cache_for_plan(settle path) andPainterdraw loop (cache lookup)Benchmark results (135k.perf.grida, 135K nodes)
Zero regressions across all scenarios and fixture files.
Test plan
cargo test -p cg— all tests passcargo check -p cg -p grida-canvas-wasm -p grida-dev— all crates compilecargo clippy -p cg --no-deps— no new warnings🤖 Generated with Claude Code
Summary by CodeRabbit