Repository navigation
perf(object-cache): find expired entries with a deadline heap, not a scan (LAB-7106) - #658
Conversation
…scan At a full bound, ObjectCache.put scanned every entry for expired ones before it evicted the LRU head, under the lock every hit takes. With ttl=None nothing expires, so every L1-only miss at a full budget paid a full scan for nothing. A min-heap of (deadline, key) now finds expired entries: one comparison when none is due, O(log n) per due entry, and expired entries still go before live LRU ones. Deletion is lazy; a put rebuilds the heap from the store once dead tuples outnumber entries. complete_refresh pushes its new deadline and evicts through the same path. A min-expiry watermark was tried first and rejected: on a Zipf workload at capacity it still scanned on 18-27% of over-bound puts with one TTL near the turnover time, and on 45-51% with mixed TTLs. Adds the object_cache_put_full instruction-budget path and re-records every budget, since editing ir_workload.py moves the heap layout.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
|
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 12 minutes. View limit details
Summary by CodeRabbit
WalkthroughThe object cache now tracks expiry deadlines in a heap and uses them to remove expired entries during capacity eviction. Unit tests cover eviction order, refreshed and replaced entries, heap bounds and clearing. Performance workloads, instruction budgets and documentation cover puts into a full cache. ChangesObject-cache expiry eviction
Priority: Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Refactor Suggested reviewers: Merge Risk:
 |
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly and concisely describes the main change: replacing the full expiry scan with a deadline heap. The issue reference is relevant. |
| Description check | |
The description is detailed and covers the change, motivation, implementation, performance results, tests, documentation, and compatibility impact. It does not reproduce every template heading or chec… |
| Docstring Coverage | |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Description check
-
Explanation
The description is detailed and covers the change, motivation, implementation, performance results, tests, documentation, and compatibility impact. It does not reproduce every template heading or checkbox, but it provides the critical information required for review.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 9 functions across 4 files. (3 skipped: 3 unsupported.)
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔇 Additional comments: 1
src/cachekit/object_cache.py-145-146: 1
145-146:Minor · Maintainability & Code Quality · Quick win
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Qualify the no-scan claim.
When
push()rebuilds the heap, it scanss.store.items()even if no entry has expired. State that an ordinary capacity eviction avoids the scan, but a rebuild performs one. Make the same qualification indocs/performance.mdLine 98.
-
-
- Source mode: Auto
- Repository: cachekit-io/cachekit-py
- Branch: perf/lab-7106-objectcache-expiry-heap
- File: .coderabbit.yaml
- Commit: 6fb59fc
- Layers: Repository YAML
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
fc362feb-05a2-4cf7-9033-362dbf1bd79b
-
docs/free-threading.mddocs/performance.mdsrc/cachekit/object_cache.pytests/performance/ir_baselines.jsontests/performance/ir_budget.pytests/performance/ir_workload.pytests/unit/test_object_cache.py
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
-
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
Instruction budget: PASSHead
A path fails at +1% or more against the base (+2% for |
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
Summary
Once an
ObjectCacheis at its bound, everyput()used to scan all entries for expired ones before it evicted the LRU head, holding the lock that everyget()on that function also takes. An L1-only@cache(backend=None)function builds one with a 100MB byte budget. Withttl=Noneits entries are stored for a year, so at a full budget nothing ever expired and every miss paid a scan of the whole cache for nothing. At 100,000 entries that scan took longer than everything else a put does by about 200 times.The cache now keeps a min-heap of
(deadline, key). A put over the bound pops only the tuples that are due. When nothing has expired that is one comparison, and each expired entry it removes costs O(log n). Expired entries still go before live LRU entries, wherever they sit in LRU order.How
_ObjectCacheState.expiryholds the heap.put()andcomplete_refresh()push the entry's deadline after they write it. Deletion is lazy: a removal, a replacement or a refresh leaves its old tuple. A tuple is live while its key holds an entry with that exact deadline, so a dead one is dropped when it comes due and can never remove a newer entry.push()rebuilds the heap from the store once it holds twice as many tuples as there are entries. That costs one pass and at least halves the heap, so it amortises to O(1) per push.complete_refresh()restores the byte bound through the same_evict(), so its eviction also removes expired entries first via the heap.clear()empties the heap. The at-fork reset already swaps the whole state object, heap included.Why a heap and not a min-expiry watermark. A watermark adds no memory, so I simulated it first: a Zipf workload over 10 times the cache's capacity, at capacity, with misses filled by
put(). With one TTL close to the time the cache takes to turn over, it still scanned on 18-27% of the puts that hit the bound. With half the keys on an hour and half on a short TTL (2-5 s against a 200 s run), it scanned on 45-51%. A dead heap tuple costs nothing until it comes due, so the heap never scans in either case.Memory. The heap is memory the byte budget does not count. Each tuple shares its float and key with the entry, so it costs 64 bytes per entry on CPython 3.12 (a 56-byte 2-tuple and its 8-byte list slot) and 72 on 3.14, where tuples are 8 bytes larger. Dead tuples add up to the same again before a rebuild.
The rebuild pause. One put in about every n puts at capacity rebuilds the heap. In the interleaved run below, at 100,000 entries that put took about 3 times as long as one put scanning the cache did before this change. Before the change, every put scanned.
Measurements
Instructions (main thread,
tests/performance/ir_budget.py's method, median of the 5 heap layouts), for the newobject_cache_put_fullpath: a put of a new key into a full 10,000-entry byte budget, nothing expired. The path did not exist before this change, so both arms ran this branch'sir_workload.py, one withobject_cache.pyatmainswapped in. Under callgrind themainarm grew past 8 GiB at the gate's 1,000/3,000 loop sizes, so both arms are measured at 100/300:mainAt the gate's own loop sizes this PR's figure is 164,507 on 3.12 and 160,678 on 3.14, the budgets committed here. Most of what remains is
_estimate_object_sizeon the stored value, which the byte bound needs.Wall clock, interleaved in one process:
main,mainagain as the A/A arm, and this PR, with puts round-robined across three identically built caches holding a 1,308-byte (estimated) dict value. The twomainarms stayed within 1.5% of each other at p50. This PR's p50 put was faster at every size, so the sign agrees with the instruction counts:main÷ this PRmain÷ this PRIndicative saving per put at 10,000 entries, from the instruction counts: 5.48 million fewer instructions × 0.068 ns per instruction (the
mainarm's wall p50 over its count, 3.14) ≈ 0.37 ms, which matches the measured p50 difference of 0.367 ms. The saving grows with the entry count, and it lands on a miss: hits gain only by waiting less for the lock.Other paths. Editing
ir_workload.pymoves the heap layout of every measured process, so every budget is re-recorded on both interpreters (--update --allow-increase). They all move by ±0.7% or less exceptserializer_orjson, which drops by 4.6-6.8%. That path is gated at 2% because its figure moves with heap layout (TOLERANCE_PCT). No other path stores into anObjectCache.l1_hitreads one and moves by -0.14% on 3.14 and +0.11% on 3.12. TheInstruction budgetcheck compares against the base, which has noobject_cache_put_fullyet, so that path reports as new.Tests
In
tests/unit/test_object_cache.py:test_put_evicts_expired_before_lruis unchanged and passes.test_put_evicts_expired_entry_deep_in_lru_order_before_live_head: an entry read just before it expired sits at the MRU end, and it is still evicted before the live LRU head.test_put_at_capacity_scans_no_entry_when_none_has_expired: 50 puts at capacity over fresh entries never iterate the store. Fails onmain.test_refreshed_entry_is_not_evicted_at_its_old_deadlineandtest_replaced_entry_is_not_evicted_at_the_old_entrys_deadline: a dead tuple that comes due does not remove the live entry under its key.test_complete_refresh_over_the_byte_bound_evicts_expired_before_lru: a refresh that grows its value evicts the expired entry, not the live LRU head.test_expiry_heap_stays_bounded_at_capacity: with a hot key holding the heap top, the heap reaches twice the entry count, is rebuilt there and never passes it.clear()empties it._consistent(), which the fork tests use, now also checks that every entry's current deadline is in the heap.Each new test was checked against a mutant. Dropping the deadline match from the liveness check fails both dead-tuple tests. Restoring the scan fails the no-scan test.
Locally on CPython 3.14.3:
pytest tests --ignore=tests/fuzzing -m "not slow"gives 7,573 passed and 109 skipped.ruff check,ruff format --checkand basedpyright report nothing new.Docs
object_cache.py: the class,put()and_evict()docstrings describe the heap, its per-entry memory and the rebuild.docs/performance.md: a new "L1-Only Miss at a Full Budget" section, and the new budget row plus the re-recorded figures.docs/free-threading.md: theObjectCacherow lists the heap in the state object. The single-flight row no longer says an expiry sweep can hold the cache lock for a scan of the whole cache.