perf(leaderboard): decode each logo once, not once per rebuild - #270
Merged
Conversation
Image.open defers the PNG decode to first access, so the decode landed inside _prepare_logo's convert(), with a LANCZOS resize behind it -- for every team, on every rebuild, for files that had not changed. Measured on the real asset directory: 25 logos cost 276ms cold and 0.3ms warm. It matters more than a normal cache miss because leaderboard scroll content is generated on the render thread. The adapter's capture path needs the shared canvas, so it cannot be prepared in the background (render_pipeline.drain_deferred explains why), and the cost lands directly on the Vegas scroll as stutter. Keyed on path and mtime, so a logo the downloader backfills is picked up rather than served stale. A missing file is deliberately not cached: remembering it as absent would keep a logo downloaded moments later invisible until restart. Honest scope: this does not fix the 3.2s freeze that led here. That one is still present with this change in place, and the watchdog work on the core side now shows why it was never caught -- it holds the GIL. This removes a real and separate cost on the same path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe Sports Leaderboard plugin now caches prepared league and team logos with file-change detection and bounded storage. Missing team logos download during leaderboard updates instead of rendering. Release metadata reports version 1.3.2, with an executable validation script. ChangesSports Leaderboard logo handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Manager
participant ImageRenderer
participant LogoDownloader
participant LocalLogoFiles
Manager->>ImageRenderer: fetch leaderboard data
ImageRenderer->>LocalLogoFiles: check team logo paths
ImageRenderer->>LogoDownloader: download missing logos
LogoDownloader->>LocalLogoFiles: save downloaded logos
Manager->>ImageRenderer: build layout
ImageRenderer->>LocalLogoFiles: load cached local logos
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 39 |
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/ledmatrix-leaderboard/image_renderer.py`:
- Around line 300-304: Update image_renderer.py in the logo cache-miss path
around _prepare_logo so layout construction loads only existing local files and
never invokes the downloader; move missing-logo retry behavior into update(). In
test_logo_cache.py lines 97-115, replace immediate render retry assertions with
update()-driven retry coverage and verify that a subsequently downloaded local
file becomes visible on a later render.
🪄 Autofix
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 Plus
Run ID: c45a4c47-0fef-445b-ac92-9fd3eaf478e2
📒 Files selected for processing (4)
plugins.jsonplugins/ledmatrix-leaderboard/image_renderer.pyplugins/ledmatrix-leaderboard/manifest.jsonplugins/ledmatrix-leaderboard/test_logo_cache.py
…271) * fix(leaderboard): move missing-logo downloads off the render thread _get_team_logo ran from layout construction inside display(), and on a cache miss it invoked download_missing_logo -- a network call on the render thread the Vegas scroll depends on. Missing logos are now backfilled by a new download_missing_logos(), called from update() instead; _get_team_logo only ever reads local files, and a backfilled logo becomes visible on the next rebuild via the existing mtime-keyed cache. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RRsTMmLUcpd8tpN1qciEgC * test(leaderboard): drop unused variable in logo cache test Codacy flagged an unused local in the new test coverage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RRsTMmLUcpd8tpN1qciEgC --------- Co-authored-by: Claude <noreply@anthropic.com>
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
2 participants
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.
What
_get_team_logocalledImage.openon every request with no cache, and_prepare_logore-ranconvert+ LANCZOSresizeeach time.Image.openis lazy, so the PNG decode landed insideconvert()— for every team, on every rebuild, for files that had not changed.Measured on the real asset directory (page cache warmed first, to be fair):
A real board carries considerably more than 25.
Why this path in particular
Leaderboard scroll content is generated on the render thread. The adapter's capture path needs the shared canvas, so it cannot be prepared in the background —
render_pipeline.drain_deferred()documents this and trickles one plugin per frame to spread the cost. So work here lands directly on the Vegas scroll as visible stutter.Cache correctness
What this does NOT fix
The 3.2s freeze that led me here is still present with this change in place — I re-measured on the rig and it was unchanged at 3270ms. My hypothesis that logo decoding dominated it was wrong; 276ms is not 3.2s.
Companion core work (LEDMatrix#454) now explains why that freeze was never caught: it holds the GIL, so the stall watchdog's thread could not run during it. That's now detected separately.
This PR stands on its own — it removes a real, repeated, measurable cost on the render thread — but it should not be read as the stutter fix.
Tests
test_logo_cache.py: loaded exactly once across 25 rebuilds, distinct entries per target size, a file replaced on disk is re-read, a missing logo is retried rather than remembered, the cache respects its ceiling, and a warm rebuild is more than 10x cheaper than a cold one.Safety harness passes all 8 sizes;
check_module_collisions.pyclean across 43 plugins.🤖 Generated with Claude Code
https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
Summary by CodeRabbit
Performance Improvements
Bug Fixes
Release