fix(cg): redraw() forced stable on every frame, defeating zoom cache#616
Merged
softmarshmallow merged 1 commit intomainfrom Mar 30, 2026
Merged
fix(cg): redraw() forced stable on every frame, defeating zoom cache#616softmarshmallow merged 1 commit intomainfrom
softmarshmallow merged 1 commit intomainfrom
Conversation
…ache Application::redraw() unconditionally passed stable=true to apply_changes(), which nuked the zoom image cache on every interaction frame. This forced every zoom frame into a full O(N) draw instead of the O(1) cached texture blit, producing ~3 FPS on large scenes. Fix: derive stable from !camera_change.any_changed() — unstable during active interaction (preserves zoom cache), stable when idle (full quality). Measured on 90K nodes: p50 drops from 4,511µs to 5µs (~900× improvement). Also adds zoom-specific benchmark scenarios to grida-dev bench runner: - zoom_with_settle: settle frames interleaved with zoom (cache-cold spikes) - zoom_forced_stable: reproduces the bug for A/B comparison - frameloop_zoom: real FrameLoop path at various event intervals Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
WalkthroughThe PR modifies zoom image cache handling in the canvas renderer to improve interaction performance. It removes hard eviction logic based on zoom ratio, updates fast-path eligibility to use the zoom cache on no-change frames, and fixes the redraw path to compute stability based on actual camera changes rather than forcing it to true. New benchmark scenarios are added to measure this optimized behavior. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![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
🧹 Nitpick comments (3)
crates/grida-canvas/src/runtime/scene.rs (1)
1098-1110: Extract the zoom-cache eligibility rule into one helper.Lines 1108-1110, 1254-1256, and the zoom-cache branch in Lines 1617-1621 all encode the same
zoom_changed() || Nonepolicy. Keeping that rule in three places makes the ready/deferred paths easy to drift apart the next time this heuristic is tuned.♻️ Possible helper extraction
+ #[inline] + fn zoom_cache_candidate(camera_change: CameraChangeKind) -> bool { + camera_change.zoom_changed() || camera_change == CameraChangeKind::None + } ... - let use_zoom_cache = self.zoom_image_cache.is_some() - && (camera_change.zoom_changed() || camera_change == CameraChangeKind::None); + let use_zoom_cache = + self.zoom_image_cache.is_some() && Self::zoom_cache_candidate(camera_change); ... - let zoom_cache_usable = self.zoom_image_cache.is_some() - && (plan.camera_change.zoom_changed() || plan.camera_change == CameraChangeKind::None); + let zoom_cache_usable = + self.zoom_image_cache.is_some() && Self::zoom_cache_candidate(plan.camera_change);// In `can_defer`, reuse the same helper for the zoom-cache branch and keep // the pan-cache fallback as the separate `CameraChangeKind::None` case.Also applies to: 1249-1256, 1614-1621
🤖 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` around lines 1098 - 1110, Extract the repeated "zoom cache eligibility" logic into a single helper (e.g., a function or method like zoom_cache_applicable(camera_change: CameraChangeKind) -> bool or Scene::zoom_cache_applicable(&self, camera_change: &CameraChangeKind)) that returns true when self.zoom_image_cache.is_some() && (camera_change.zoom_changed() || *camera_change == CameraChangeKind::None); replace the three duplicated checks (the branch that currently reads self.zoom_image_cache.is_some() && (camera_change.zoom_changed() || camera_change == CameraChangeKind::None) and the similar checks in can_defer and the zoom-cache branch) with calls to this helper, and keep the pan-only fallback logic as the separate CameraChangeKind::None case where appropriate.crates/grida-dev/src/bench/runner.rs (1)
1856-1860: Warmup uses pan operations for zoom scenarios.The warmup loop does
renderer.camera.translate(1.0, 0.0)(pan) before running zoom scenarios. This primes the pan cache but not the zoom cache. The first zoom frame will be a cache miss regardless, so this likely doesn't affect measurements materially, and it's consistent with thewarmup()helper used by frameloop scenarios. Just noting for awareness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-dev/src/bench/runner.rs` around lines 1856 - 1860, The warmup loop currently calls renderer.camera.translate(1.0, 0.0) which primes pan caches but not zoom caches, so change the warmup to perform the same camera operation used by zoom scenarios (e.g., call the zoom/scale method on renderer.camera with the same parameters used in your zoom tests) in that for loop, then call renderer.queue_unstable() and renderer.flush() as before; update the loop where renderer.camera.translate, renderer.queue_unstable, and renderer.flush are invoked (and adjust to use the zoom method name used elsewhere, such as camera.zoom or camera.scale) so the warmup primes the zoom cache.docs/wg/feat-2d/optimization.md (1)
879-883: Add blank line before table for markdown compliance.The static analysis tool flagged that tables should be surrounded by blank lines (MD058). Line 879 ends with a colon and the table starts immediately on line 880.
📝 Proposed fix
Measured on 01-135k.perf.grida (135K nodes): + | Scenario | Before p95 | After p95 | Before MAX | After MAX |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/wg/feat-2d/optimization.md` around lines 879 - 883, Insert a single blank line between the line that reads "Measured on 01-135k.perf.grida (135K nodes):" and the Markdown table that immediately follows so the table is separated by an empty line (fixes MD058); specifically, locate the block containing that exact header text and add one blank line before the table header row.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/grida-canvas/src/window/application.rs`:
- Around line 1453-1461: The change makes the `stable` flag (computed from
`self.renderer.camera.change_kind().any_changed()`) drive
`self.renderer.apply_changes(camera_change, stable)`, which can leave dev-only
paths that call `redraw_requested()` without a corresponding full `frame()` in
an unstable state; verify and ensure the dev integration either triggers a
settle/full frame after camera changes or adjust the logic: either (A) ensure
the dev path in `crates/grida-dev/src/platform/native_application.rs` schedules
a follow-up `frame()`/settle when `camera_change.any_changed()` is true, or (B)
modify `apply_changes` call-site to treat redraw-only calls as stable for
recapture (or add a guard) so zoom/pan caches are recaptured—look for the
symbols `camera.change_kind()`, `camera_change.any_changed()`,
`self.renderer.apply_changes(...)`, and the `redraw_requested()` path and make
the chosen fix consistent across dev/test code paths.
---
Nitpick comments:
In `@crates/grida-canvas/src/runtime/scene.rs`:
- Around line 1098-1110: Extract the repeated "zoom cache eligibility" logic
into a single helper (e.g., a function or method like
zoom_cache_applicable(camera_change: CameraChangeKind) -> bool or
Scene::zoom_cache_applicable(&self, camera_change: &CameraChangeKind)) that
returns true when self.zoom_image_cache.is_some() &&
(camera_change.zoom_changed() || *camera_change == CameraChangeKind::None);
replace the three duplicated checks (the branch that currently reads
self.zoom_image_cache.is_some() && (camera_change.zoom_changed() ||
camera_change == CameraChangeKind::None) and the similar checks in can_defer and
the zoom-cache branch) with calls to this helper, and keep the pan-only fallback
logic as the separate CameraChangeKind::None case where appropriate.
In `@crates/grida-dev/src/bench/runner.rs`:
- Around line 1856-1860: The warmup loop currently calls
renderer.camera.translate(1.0, 0.0) which primes pan caches but not zoom caches,
so change the warmup to perform the same camera operation used by zoom scenarios
(e.g., call the zoom/scale method on renderer.camera with the same parameters
used in your zoom tests) in that for loop, then call renderer.queue_unstable()
and renderer.flush() as before; update the loop where renderer.camera.translate,
renderer.queue_unstable, and renderer.flush are invoked (and adjust to use the
zoom method name used elsewhere, such as camera.zoom or camera.scale) so the
warmup primes the zoom cache.
In `@docs/wg/feat-2d/optimization.md`:
- Around line 879-883: Insert a single blank line between the line that reads
"Measured on 01-135k.perf.grida (135K nodes):" and the Markdown table that
immediately follows so the table is separated by an empty line (fixes MD058);
specifically, locate the block containing that exact header text and add one
blank line before the table header row.
🪄 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
Run ID: 44728ded-c707-45ba-a85d-38bbc1481416
📒 Files selected for processing (5)
.agents/skills/cg-perf/SKILL.mdcrates/grida-canvas/src/runtime/scene.rscrates/grida-canvas/src/window/application.rscrates/grida-dev/src/bench/runner.rsdocs/wg/feat-2d/optimization.md
Comment on lines
+1453
to
+1461
| // | ||
| // Use unstable (stable=false) when a camera change is active — this | ||
| // preserves the zoom/pan image caches and allows the fast blit paths. | ||
| // Without this, every zoom frame would nuke the zoom cache and force | ||
| // a full O(N) draw, causing ~3 FPS on large scenes. | ||
| { | ||
| let camera_change = self.renderer.camera.change_kind(); | ||
| self.renderer.apply_changes(camera_change, true); | ||
| let stable = !camera_change.any_changed(); | ||
| self.renderer.apply_changes(camera_change, stable); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python <<'PY'
from pathlib import Path
import re
redraw = re.compile(r'\.\s*redraw_requested\s*\(')
frame = re.compile(r'\.\s*frame\s*\(')
for path in Path('.').rglob('*.rs'):
text = path.read_text(errors='ignore')
if redraw.search(text):
redraw_hits = [text.count('\n', 0, m.start()) + 1 for m in redraw.finditer(text)]
frame_hits = [text.count('\n', 0, m.start()) + 1 for m in frame.finditer(text)]
print(f'== {path} ==')
print(f' redraw_requested calls: {redraw_hits}')
print(f' frame calls: {frame_hits or "NONE"}')
print()
PYRepository: gridaco/grida
Length of output: 288
🏁 Script executed:
head -20 crates/grida-dev/src/platform/native_application.rs && echo "..." && sed -n '360,370p' crates/grida-dev/src/platform/native_application.rsRepository: gridaco/grida
Length of output: 1130
🏁 Script executed:
# Check the broader context around line 364 in native_application.rs
sed -n '355,375p' crates/grida-dev/src/platform/native_application.rsRepository: gridaco/grida
Length of output: 723
🏁 Script executed:
# Check if grida-dev is a dev/test crate or part of the main application
head -30 crates/grida-dev/Cargo.tomlRepository: gridaco/grida
Length of output: 785
Potential redraw-only regression in dev application: verify settle-frame expectation.
The script confirms that crates/grida-dev/src/platform/native_application.rs (lines 364) calls redraw_requested() without a corresponding frame() path. While this is development/test code (publish = false), line 1460's change to make stable depend on camera_change.any_changed() means an unstable frame could be rendered if no subsequent stable settle is guaranteed. The continuous winit event loop makes this less likely to manifest, but the architectural risk remains: any host that relies only on redraw_requested() → redraw() for interaction rendering will not trigger the full-quality settle recapture that the new zoom-cache optimization assumes.
Verify whether this dev integration path is expected to support the new cache behavior, or if this regression is accepted for test code only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/src/window/application.rs` around lines 1453 - 1461, The
change makes the `stable` flag (computed from
`self.renderer.camera.change_kind().any_changed()`) drive
`self.renderer.apply_changes(camera_change, stable)`, which can leave dev-only
paths that call `redraw_requested()` without a corresponding full `frame()` in
an unstable state; verify and ensure the dev integration either triggers a
settle/full frame after camera changes or adjust the logic: either (A) ensure
the dev path in `crates/grida-dev/src/platform/native_application.rs` schedules
a follow-up `frame()`/settle when `camera_change.any_changed()` is true, or (B)
modify `apply_changes` call-site to treat redraw-only calls as stable for
recapture (or add a guard) so zoom/pan caches are recaptured—look for the
symbols `camera.change_kind()`, `camera_change.any_changed()`,
`self.renderer.apply_changes(...)`, and the `redraw_requested()` path and make
the chosen fix consistent across dev/test code paths.
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
Application::redraw()passedstable=truetoapply_changes()on every frame, nuking the zoom image cache during active interaction. Every zoom frame did a full O(N) draw instead of the O(1) cached texture blit. Changed to derivestablefrom!camera_change.any_changed().grida-dev bench—zoom_with_settle,zoom_forced_stable(bug reproduction), andframeloop_zoom(real FrameLoop path at various event intervals).optimization.mditem 21 andcg-perf/SKILL.mdwith new scenario types.Measured impact (90K nodes, headless GPU, Apple M2 Pro)
Test plan
cargo test -p cg— all 33 tests passcargo check -p cg -p grida-canvas-wasm -p grida-devcargo run -p grida-dev --release -- bench --size 300 --frames 100confirms fixwd_windowed_mode) should be smooth🤖 Generated with Claude Code
Summary by CodeRabbit
Performance Improvements
Documentation & Testing