perf(layout): don't pin id-keyed source images in fit_image's cache - #732
Conversation
LayoutContext.fit_image keyed images without a cache_key by id() and held a strong reference to the source so the id could not be recycled. A plugin following the documented one-liner -- draw_image(Image.open(path), box) each frame -- never hit that cache and kept the last 64 sources alive: ~64MB for 500x500 RGBA team logos (median size under assets/sports), up to ~600MB for the largest. The entry now holds a weak reference whose callback drops it when the source is freed, and a hit re-checks that the referent is the same image. Sources that cannot be weak-referenced are still pinned. Keyed entries (the only kind any plugin on ledmatrix-plugins main uses today: football-scoreboard's logo fit) are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
ChangesImage cache source tracking
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk remains; the image-cache change is mergeable after normal checks. Security Architecture ReviewSecurity architecture risk: ⚪ Minimal · up to The change reduces source-image retention without expanding drawing authority or changing the public API. Identity checks and guarded cleanup preserve the inspected cache lifecycle. No material security risk was found in the affected drawing path. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
1 participant
Summary
LayoutContext.fit_imagecaches fitted images in a 64-entry LRU. With nocache_key, the key isid(img), and the entry used to hold a strong reference to the source so the id couldn't be recycled. A caller that passes a freshly loaded image every frame, which is the documenteddraw_image(Image.open(path), box)one-liner, never hits that cache and keeps the last 64 sources alive. For 500x500 RGBA logos (the median underassets/sports) that's ~64 MB, and up to ~600 MB with the largest logo in assets.An id-keyed entry now holds a weak reference to its source, and a callback drops the entry when the source is freed. A hit re-checks that the referent is the same image. Sources that can't be weak-referenced are still pinned, and keyed entries are unchanged.
Type of change
Test plan
pytest): the old "pins source" test is replaced by 4 tests (entry doesn't pin and dies with its source, fresh-per-frame holds 0 entries, a recycled id doesn't alias, an unweakrefable source is still pinned), all of which fail against the old code. The full suite passes apart from thepsutil-dependenttest_live_status_fieldstests, which fail the same way onmain.Plugin compatibility
mainthe onlyfit_imagecaller (football-scoreboardgame_renderer.py) passes acache_key, and no plugin callsBasePlugin.draw_image. This is hardening for the documented usage, not a fix for a live leak.🤖 Generated with Claude Code
Summary by CodeRabbit