refactor: eliminate per-frame Dictionary allocations in render_worker_overlay - #299
Conversation
…_overlay
Replace `_overlay_sprite_cache` which allocated a new `{"frame": frame, "carrying": carrying}`
Dictionary per stale worker each frame with two separate scalar-value dictionaries
(`_overlay_sprite_cache_frame` and `_overlay_sprite_cache_carrying`). This avoids creating
short-lived Dictionary objects every frame (~60 fps), reducing GC pressure.
The existing `_overlay_collision_slots` and `_overlay_used_slots` scratch buffers were already
reused via `.clear()` — no change needed there. Added a comment explaining the GC-conscious
design to prevent future regressions.
Fixes #292
Signed-off-by: Saffron <263493777+itsmiso-ai@users.noreply.github.com>
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: MiniMax-M2.7@https://litellm.jory.dev/v1 (anthropic) — escalated (fast_low_confidence)
Recommendation: Approve
This is a clean, well-scoped GC refactor that directly addresses issue PR 292. The change replaces a per-worker per-frame {"frame": frame, "carrying": carrying} dictionary allocation with two flat scalar-keyed dictionaries, eliminating the nested dictionary allocation without changing the staleness-check semantics.
Change-by-Change Findings
scripts/main.gd — render_worker_overlay() GC optimization
Before:
var _overlay_sprite_cache: Dictionary = {}
...
if _overlay_sprite_cache.has(name):
var cached: Dictionary = _overlay_sprite_cache[name] # per-worker dict
texture_stale = int(cached["frame"]) != frame or String(cached["carrying"]) != carrying
...
_overlay_sprite_cache[name] = {"frame": frame, "carrying": carrying} # allocated every stale workerAfter:
var _overlay_sprite_cache_frame: Dictionary = {}
var _overlay_sprite_cache_carrying: Dictionary = {}
...
if _overlay_sprite_cache_frame.has(name):
texture_stale = int(_overlay_sprite_cache_frame[name]) != frame or String(_overlay_sprite_cache_carrying.get(name, "")) != carrying
...
_overlay_sprite_cache_frame[name] = frame
_overlay_sprite_cache_carrying[name] = carryingThe PR correctly:
- Splits the composite cache into two
String→Scalardictionaries, eliminating the{"frame", "carrying"}dict per stale worker per frame. - Preserves staleness semantics (frame mismatch OR carrying mismatch → stale).
- Uses
.get(name, "")forcarryingon the first occurrence, which matches the original behavior ofcached["carrying"]defaulting to an empty string equivalent. - Removes the local
var cached: Dictionaryintermediate variable, reducing indirection.
Standards Compliance
The repository has no documented conventions specifically governing dictionary reuse in render loops. The AGENTS.md general guidance does not conflict with this change. The PR follows the codebase's existing pattern of declaring typed member variable caches at class scope.
Linked Issue Fit
| Acceptance criterion | Status |
|---|---|
_overlay_collision_slots / _overlay_used_slots reused via .clear() |
✅ Already reused (.clear() present in original); diff comment now documents this. |
| Sprite staleness cache inline instead of per-worker Dictionary | ✅ Fixed: two String→Scalar dictionaries replace the String→{Dict} cache. |
| Add comment explaining GC-conscious design | ✅ Added: # GC-conscious per-frame scratch buffers (audit #292): with inline explanations. |
| Verify no short-lived Dictionary allocations |
The acceptance criteria are substantially met. Runtime verification with a memory profiler is noted as a suggestion in PR 292 but is not a hard gate.
Tool Harness Findings
git_grepfor_overlay_collision_slots|_overlay_used_slotsreturned no matches in the truncated corpus. However, the diff comment (line 1425) explicitly documents these as "cleared each frame and reused," confirming the existing.clear()pattern satisfies the acceptance criterion. No evidence of removal or regression was found.git_grepfor_overlay_sprite_cacheshows all references correctly updated to the new split dictionaries.git_logshows this is the only commit on the branch (single-commit PR), consistent with the PR body.
CI Status
All checks passed: Export validation (macOS/Windows/Linux), headless smoke test, macOS validation, script test suite.
Unknowns / Needs Verification
- Runtime GC verification: The original issue suggested using
@GDScript warning_unsafe_method_accessor a memory profiler to confirm zero short-lived Dictionary allocations in_process. This PR does not include explicit verification tooling. If this is a blocker for closing PR 292, a manual memory-profiler run should be documented before closing the issue. The change is structurally sound based on code inspection.
What
Replaces the per-stale-worker
{frame, carrying}Dictionary allocation inrender_worker_overlay()(scripts/main.gd) with two scalar-keyed dictionaries (_overlay_sprite_cache_frame,_overlay_sprite_cache_carrying), and adds a comment block explaining the GC-consc…Fixes #292
Opened by foreman on review GO (workload wl-misospace-windowstead-292).