perf(cache): skip rewriting unchanged CacheManager.set() records - #730
Merged
Merged
Conversation
DiskCache skipped a payload identical to its last write, but CacheManager.set() stamps each record with time.time(), so the payload never matched and every unchanged re-save rewrote the file. The digest now leaves out a header-first record's timestamp (ttl and data still count) and the newer timestamp lives in the file's mtime: a skipped save touches mtime to the record's timestamp, and a real write pins mtime to the record's own timestamp so an old-stamped record cannot borrow the write time's freshness. Readers age a record from the newer of the two, bounded at one hour past the embedded timestamp (unchanged data is rewritten hourly, so an honest lift never reaches the bound and a copy without mtime reads at most an hour fresher). DiskCache.get returns the record with the effective timestamp, so the header check, CacheManager max_age logic, the memory tier and plugins reading record['timestamp'] all agree. The skip also checks the file's inode and size, so a write by another process is never mistaken for ours. 100 identical set() of a 32 KB record: 100 writes before, 1 after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uc5DAbSwGGUTm3MrCtpC2m
The resource monitor wrote a plugin_metrics:<id> record per plugin,
throttled to 30 s each: two SD writes a minute per plugin. All plugins
now share one plugin_metrics_snapshot record ({"schema": 1, "plugins":
{id: record}}, each record shaped as before), written at most once a
minute.
Readers (the web UI's /plugins/metrics routes, through get_metrics)
return the same fields: they read the snapshot and fall back to the old
per-plugin record for a plugin it does not have yet, so an upgrade
loses nothing. Each write starts from the snapshot on disk and replaces
only the plugins this process recorded, so plugins not run since a
restart keep their numbers and a web-side reset of an idle plugin
sticks. Entries idle for 30 days are dropped, mirroring the retention
that used to age out their files.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uc5DAbSwGGUTm3MrCtpC2m
Every CacheManager built a ConfigManager and loaded the config in __init__, for a CacheStrategy that keeps the parameter but reads nothing from it. config_manager is now a property that builds and loads it on first access, so plugins resolving the global timezone through cache_manager.config_manager get the same object as before; assignment still replaces it, an ImportError still yields None, and a failing load_config() still raises (now at first access) and is retried on the next one. CacheStrategy is no longer handed the config manager. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uc5DAbSwGGUTm3MrCtpC2m
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BkfgXMqqwn2w4NN7LRzhxy
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 33 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (9)
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 | 42 |
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.
# Conflicts: # CHANGELOG.md
The disk cache now reads a record's age from the newer of its embedded timestamp and the file's mtime (an unchanged re-save only touches the file). The #728 tests aged a record by rewriting its timestamp, which moved the mtime to now, so the record read as fresh. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # CHANGELOG.md
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
DiskCache.set()already skips rewriting a payload identical to the last one it wrote, but forCacheManager.set()that check never fired: every record embedstimestamp: time.time(), so the bytes always differed and every unchanged re-save was a full SD-card write. The digest now leaves out a header-first record's timestamp. A skipped write moves the file's mtime to the new timestamp instead, readers take a record's age from the newer of the two, and the mtime is never trusted more than an hour past the embedded timestamp (see the "UNCHANGED RE-SAVES" comment indisk_cache.py).Also in this PR:
plugin_metrics:<id>records become oneplugin_metrics_snapshotwritten at most once a minute (the old per-plugin records are still read as a fallback, never written)CacheManagerbuilds itsConfigManageron first use. Nothing in the cache reads it, and the sports plugins that usecache_manager.config_managerget the same object, just later.Type of change
Test plan
Ran on a real Raspberry Pi with hardware: hdpi (Pi 4, 512x64), 40-min A/B/B/A soak against
main, each window starting from a fresh restart. Counts are frominotifywaiton/var/cache/ledmatrix, after a 5-min warmup:main(A1)main(A2)plugin_metrics*writesNo frame-timing change in Vegas mode (same music load, minus warmup): 81.3 fps mean / 20.7 ms median p99, against 81.7 / 20.9 on
main. RSS 333 MB vs 326 and 351 MB onmain. Journal errors were unchanged (4–6 per window, allyoutube-stats"API key not configured").Ran the test suite (
pytest): newtest_cache_unchanged_resave.py(skip, mtime lift, cap, copied/touched/replaced files, other process's file),test_cache_manager_lazy_config.pyandtest_resource_monitor.pyadditions. The full suite passes, apart from 8test_live_status_fields.pytests that needpsutiland fail the same way onmain.Plugin compatibility
plugin_metrics:*.cache_manager.config_manageris still public.)Notes for reviewer
-portouched reads at most an hour fresher than its contents. Short-TTL records from yesterday stay stale.🤖 Generated with Claude Code