perf: restore real TTLs on cached list fetchers#13131
Draft
lokesh wants to merge 1 commit into
Draft
Conversation
get_cached_recently_modified_lists and get_active_lists_in_random passed timeout=0 to memcache_memoize, which makes the staleness check (t + timeout < now) true on every call — so every request spawned a background thread re-running the underlying site.things + get_many work. The stranded "# dateutil.HALF_HOUR_SECS)" comment shows a temporary cache-disable that was never reverted. Restore the intended 30-minute TTL for recently-modified lists and use 5 minutes for the random-active-lists layer so the /lists selection keeps rotating. memcache_memoize serves stale values while a single background thread refreshes, so no request blocks on recompute. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ky5Sc19MGPkDgbgTWWWET
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
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.
Closes #
Performance fix: makes two "cached" list fetchers actually cache, instead of spawning a background recompute thread on every single request.
Estimated performance gain
Redundant backend recomputes drop from one per request to at most a few per hour per process — a ~98–99% reduction in this workload. Each recompute is a
site.thingsquery + aget_manyover ~120 list documents plus owner/seed preloads, executed in a spawned thread. The gain is primarily backend load and thread churn (the recompute is off-request), which frees infobase/Postgres capacity on the shared request path.xychart-beta title "Recomputes per hour per process (assuming 10 /lists views/min)" x-axis ["Before (timeout=0)", "After (5-min TTL)", "After (30-min TTL)"] y-axis "background recomputes / hour" 0 --> 600 bar [600, 12, 2]Estimates scale linearly with traffic: at N views/min the "before" bar is 60×N recomputes/hour, while "after" stays capped by the TTL regardless of traffic.
Technical
Problem. In
openlibrary/plugins/openlibrary/lists.py, bothget_cached_recently_modified_listsandget_active_lists_in_randompassedtimeout=0tocache.memcache_memoize:memcache_memoize's staleness check inopenlibrary/core/cache.pyisif t + self.timeout < time.time(): self.update_async(...). Withtimeout=0that is true on every call, so every request that touches these functions spawns a background thread that re-runs the underlying work —_get_recently_modified_listsdoes asite.thingsquery plus aget_manyover the results, and_get_active_lists_in_randomloops calling it withlimit*5 = 120lists per iteration and then preloads owner + seed documents.The stranded
# dateutil.HALF_HOUR_SECS)comment is the tell: a 30-minute TTL was replaced withtimeout=0(presumably a temporary cache-disable during debugging) and never reverted.Where it hits.
get_active_lists_in_randomis@publicand rendered on the/listslanding page (templates/lists/home.html,limit=24). Every view of that page currently kicks off redundant infobase/DB work in background threads (rate-limited only by memcacheadd-flag dedup), and the "cache" never actually shields the backend.Fix.
get_cached_recently_modified_lists:timeout=dateutil.HALF_HOUR_SECS— restoring exactly what the orphaned comment says was intended.get_active_lists_in_random:timeout=5 * dateutil.MINUTE_SECS— a shorter TTL so the random selection on/listskeeps rotating; matches the 5-minute convention used elsewhere (lending.pygroundtruth availability,home.pyhomepage cache).Behavior notes.
memcache_memoizeis stale-while-revalidate: it serves the cached value and refreshes in a single background thread after the TTL, so no user request ever blocks on recompute. Worst-case staleness on/listsbecomes ~30 min for the recently-modified feed — acceptable for that surface, and the previous behavior (timeout=0) provided no fresher guarantees to users anyway, since reads still came from memcache.Testing
New test
test_cached_list_fetchers_use_real_ttlsinopenlibrary/plugins/openlibrary/tests/test_lists.pyasserts both memoize wrappers are constructed withtimeout > 0. The stale-while-revalidate timeout mechanism itself is covered by the existingopenlibrary/tests/core/test_cache.py::Test_memcache_memoize::test_timeout.Reproduce:
make test-py-uv PYTEST_ARGS="openlibrary/plugins/openlibrary/tests/test_lists.py openlibrary/tests/core/test_cache.py"— 34 passed.pre-commit— all hooks pass.Screenshot
N/A — no UI change (only cache freshness on
/lists).Stakeholders
Part of a performance-review series; see the review report branch
claude/performance-review-top-changes-5wcs7g.🤖 Generated with Claude Code
https://claude.ai/code/session_015ky5Sc19MGPkDgbgTWWWET