Make thumbnails faster#82
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 33 minutes and 43 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis pull request refactors the thumbnail caching system from storing raw bytes to storing Qt QImage objects, introducing size-aware cache management. Supporting changes include externalizing cache size constants, updating related test expectations for the new cache format, and minor import/code cleanup across the codebase. Changes
Sequence DiagramsequenceDiagram
participant Prefetcher as ThumbnailPrefetcher
participant Cache as ThumbnailCache
participant Provider as ThumbnailProvider
participant UI as Qt UI
Note over Prefetcher,UI: Image Decode & Cache Flow (New QImage-based)
Prefetcher->>Prefetcher: _decode_image(path) → QImage
Prefetcher->>Prefetcher: _rgb_to_qimage(array) → QImage
Prefetcher->>Cache: put(key, qimage_value)
activate Cache
Cache->>Cache: _thumbnail_cache_item_size(qimage) → bytes_used
Cache->>Cache: Track (value, size) in LRU
Cache->>Cache: Evict entries if exceeds max_bytes
deactivate Cache
Note over Provider: Cache Retrieval & Rendering
Provider->>Cache: get(key)
Cache-->>Provider: Optional[QImage]
alt Cache Hit
Provider->>Provider: _cached_to_image(qimage) → QImage
Provider->>UI: Return QImage to renderer
else Cache Miss
Provider->>Prefetcher: submit(path)
Provider->>UI: Return placeholder while decoding
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
faststack/thumbnail_view/provider.py (1)
268-272:⚠️ Potential issue | 🟡 MinorStale comment: most cache entries are
QImage, not bytes.After this PR, the common "bad entry" path here is an unsupported cached type (via
_cached_to_imagereturningNonewith a warning) or aQImagethat turned out to be null — not a failed bytes decode. Consider rewording so the rationale stays accurate:✏️ Suggested rewording
- # Decode of cached bytes failed — evict the bad entry so - # the prefetcher can re-decode on the next request. + # Cached entry could not be materialized into a usable QImage + # (null QImage or unsupported payload) — evict so the prefetcher + # can re-decode on the next request.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/thumbnail_view/provider.py` around lines 268 - 272, The comment beside the cache-eviction branch is stale — the failure path is typically an unsupported cached type (when _cached_to_image(...) returns None with a warning) or a null QImage, not a bytes decode error; update the comment near the discard(cache_key) call in provider.py (the block that logs "Evicted bad cache entry: %s") to describe evicting entries that are unsupported types or null QImage instances so the rationale matches _cached_to_image and the actual cache contents.
🧹 Nitpick comments (3)
faststack/thumbnail_view/prefetcher.py (2)
48-52: Minor robustness:len(value)fallback can raise for non-sized payloads.
ThumbnailCacheis typed asDict[str, Tuple[object, int]]andput()callsself._size_of(value)unconditionally. If anything other than aQImageor a sized bytes-like lands here (e.g., a numpy array is fine vialen, but an arbitrary object orNoneisn't),putwill raiseTypeErrorand abort the insert. A small guard keeps the cache well-behaved under unexpected inputs:🛡️ Suggested guard
def _thumbnail_cache_item_size(value: object) -> int: """Return the memory footprint we want to charge against the cache.""" if QImage is not None and isinstance(value, QImage): return value.bytesPerLine() * value.height() - return len(value) + try: + return len(value) # bytes / bytearray / memoryview / ndarray + except TypeError: + # Unknown payload — don't blow up the cache; just don't charge it. + return 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/thumbnail_view/prefetcher.py` around lines 48 - 52, The _thumbnail_cache_item_size function can raise TypeError for objects without a length; update it to defensively compute size: if value is a QImage use bytesPerLine()*height(), otherwise attempt to call len(value) inside a try/except (or check hasattr(value, "__len__")) and return that length, and fall back to a safe default (e.g., 0) when neither works so ThumbnailCache.put() won't raise on unexpected payloads; change only the body of _thumbnail_cache_item_size to implement this guard.
25-25: Operational note: 768 MiB default cache budget.A decoded 200×200 RGB888 thumbnail is ~120 KB, so this budget is plausible for ~5–6k items at target size — but on low-RAM machines (or when
target_sizeis larger, e.g., high-DPI) this can dominate resident memory. Two suggestions to de-risk:
- Consider exposing this via config (or an env var) so it can be tuned without a code change.
- Emit a one-shot
log.infoatThumbnailCacheconstruction reportingmax_bytes/max_itemsso field reports can correlate memory pressure with the chosen budget.Not blocking — just want to make sure this is deliberate and observable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/thumbnail_view/prefetcher.py` at line 25, Replace the hardcoded DEFAULT_THUMBNAIL_CACHE_BYTES with a configurable value (read from a config object or environment variable) and fall back to the current 768 * 1024 * 1024 if not provided; update any code that references DEFAULT_THUMBNAIL_CACHE_BYTES so it reads the configurable setting (e.g., where ThumbnailCache is constructed and where target_size is used for sizing). Also add a single log.info call in the ThumbnailCache constructor that emits the chosen max_bytes and max_items (and include target_size/scale if available) so field reports can correlate memory pressure with the budget; ensure the log is one-shot at construction time.faststack/tests/thumbnail_view/test_prefetcher.py (1)
234-244: Optional: seed with aQImageto reflect the new cache contract.
test_submit_skips_if_cachedstill pre-populates the cache withb"cached_data". It still exercises the "skip if cached" code path (the prefetcher only checksis not None), but it no longer mirrors what actually lives in the cache after the refactor. Using aQImagewould keep this test aligned with the new semantics you just asserted intest_qimage_size_accounting.♻️ Suggested tweak
- # Pre-populate cache - cache.put(cache_key, b"cached_data") + # Pre-populate cache + cache.put(cache_key, QImage(10, 10, QImage.Format.Format_RGB888))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/tests/thumbnail_view/test_prefetcher.py` around lines 234 - 244, Replace the cached payload in test_submit_skips_if_cached with a QImage instance to match the new cache contract: compute the same cache_key (using compute_path_hash(test_image) and mtime_ns), create a QImage sized/filled similarly to what's stored by the prefetcher, and call cache.put(cache_key, qimage) so prefetcher.submit(test_image, mtime_ns) sees a QImage in the cache and returns False; update references in the test to use QImage instead of b"cached_data".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@faststack/repro.py`:
- Line 12: Remove the leftover debug print in repro.py: delete the statement
print(f"DEBUG: sys.path[-1] is {sys.path[-1]}") or, if you want to keep it
temporarily, prefix it with a clear "# DEBUG" comment so it is obviously an
investigative artifact; ensure no other debug-only prints remain in the module
and run tests to confirm no side effects from removing the print.
---
Outside diff comments:
In `@faststack/thumbnail_view/provider.py`:
- Around line 268-272: The comment beside the cache-eviction branch is stale —
the failure path is typically an unsupported cached type (when
_cached_to_image(...) returns None with a warning) or a null QImage, not a bytes
decode error; update the comment near the discard(cache_key) call in provider.py
(the block that logs "Evicted bad cache entry: %s") to describe evicting entries
that are unsupported types or null QImage instances so the rationale matches
_cached_to_image and the actual cache contents.
---
Nitpick comments:
In `@faststack/tests/thumbnail_view/test_prefetcher.py`:
- Around line 234-244: Replace the cached payload in test_submit_skips_if_cached
with a QImage instance to match the new cache contract: compute the same
cache_key (using compute_path_hash(test_image) and mtime_ns), create a QImage
sized/filled similarly to what's stored by the prefetcher, and call
cache.put(cache_key, qimage) so prefetcher.submit(test_image, mtime_ns) sees a
QImage in the cache and returns False; update references in the test to use
QImage instead of b"cached_data".
In `@faststack/thumbnail_view/prefetcher.py`:
- Around line 48-52: The _thumbnail_cache_item_size function can raise TypeError
for objects without a length; update it to defensively compute size: if value is
a QImage use bytesPerLine()*height(), otherwise attempt to call len(value)
inside a try/except (or check hasattr(value, "__len__")) and return that length,
and fall back to a safe default (e.g., 0) when neither works so
ThumbnailCache.put() won't raise on unexpected payloads; change only the body of
_thumbnail_cache_item_size to implement this guard.
- Line 25: Replace the hardcoded DEFAULT_THUMBNAIL_CACHE_BYTES with a
configurable value (read from a config object or environment variable) and fall
back to the current 768 * 1024 * 1024 if not provided; update any code that
references DEFAULT_THUMBNAIL_CACHE_BYTES so it reads the configurable setting
(e.g., where ThumbnailCache is constructed and where target_size is used for
sizing). Also add a single log.info call in the ThumbnailCache constructor that
emits the chosen max_bytes and max_items (and include target_size/scale if
available) so field reports can correlate memory pressure with the budget;
ensure the log is one-shot at construction time.
🪄 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: 376dfdbd-1bc3-48f7-90d1-25fc8be14516
📒 Files selected for processing (11)
faststack/app.pyfaststack/imaging/editor.pyfaststack/repro.pyfaststack/tests/test_editor_reopening.pyfaststack/tests/test_mask.pyfaststack/tests/thumbnail_view/test_folder_stats.pyfaststack/tests/thumbnail_view/test_prefetcher.pyfaststack/tests/thumbnail_view/test_provider_logic.pyfaststack/thumbnail_view/__init__.pyfaststack/thumbnail_view/prefetcher.pyfaststack/thumbnail_view/provider.py
1 participant
Summary by CodeRabbit
New Features
Refactor
Tests