better lock parallelism for views - #4137
Conversation
PR SummaryMedium Risk Overview The read-through cache drops the shared API surface is reorganized with explicit lock contracts ( Reviewed by Cursor Bugbot for commit d100c06. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4137 +/- ##
==========================================
- Coverage 61.35% 60.37% -0.99%
==========================================
Files 2191 2074 -117
Lines 192106 178722 -13384
==========================================
- Hits 117870 107904 -9966
+ Misses 62752 60478 -2274
+ Partials 11484 10340 -1144
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sound restructuring of the shard lock into an RWMutex with an RLock-only read fast path, plus a swap from an exact LRU to an epoch-stamped sampled LRU; locking, snapshot consistency, and size accounting all check out. Remaining notes are non-blocking: an eviction sample that can abandon a pass, one inaccurate rationale comment, and test coverage for the new eviction policy and config guards.
Findings: 0 blocking | 4 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] The core property the new
lastRead/epochmachinery exists for — that an entry marked recently used survives eviction over one that was not — is untested.TestMaintenanceEvictsBackToBudgetdeliberately asserts only that the total lands back under budget ("Which keys survive is deliberately not asserted"), and this PR deletes 408 lines of exact-LRU tests that pinned ordering. The property is deterministically testable without relying on map iteration order: setEvictionSampleSizeto the entry count so the sample is the whole map, stamp some entries viaAttemptFastLookupRLocked/markRecentlyUsedacross two epochs (MaintainWLockedadvances the epoch), then evict and assert the newer-stamped entries remain. Without it, a regression that stops stamping recency — e.g. dropping theentry.lastRead.Store(c.epoch)intrackWLocked, or an inverted comparison inevictWLocked— leaves every existing test green while the cache degrades to random eviction. - [suggestion]
TestConfigValidateRejectsBadFields(sei-db/db_engine/view/view_manager_test.go) carries one row per validated field, includingoverheadZerofor the closely analogousEstimatedOverheadPerEntryguard. The two new guards inValidatehave no rows, so nothing records that a zeroEvictionSlackDivisor(which would panic on divide-by-zero inhardCap) or a zeroEvictionSampleSizeis rejected. AddevictionSlackDivisorZeroandevictionSampleSizeZerorows. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
| } | ||
|
|
||
| if victim == nil { |
There was a problem hiding this comment.
[suggestion] visited++ runs before the entry.size > 0 check, so entries in statusScheduled consume sample slots without being evictable. When all EvictionSampleSize sampled entries are in-flight reads, the function returns having reclaimed nothing and the cache stays above the cap it was asked to enforce — including on the hardCap() insert path, which is the only inline ceiling between maintenance passes.
This is practically unreachable at production sizes (a 64 MiB shard cache with the 256-byte per-entry overhead holds ≥256k entries, against in-flight reads bounded by the read pool, so the odds of 8 random draws all being pending are vanishing), so I'd not block on it. But it's cheap to make robust: bound the walk separately from the sample, e.g. require EvictionSampleSize eligible entries while capping total steps at a small multiple, or retry the sample a bounded number of times before giving up. Either keeps the no-spin property this early return protects.
Also, the comment's "The next maintenance pass samples a different part of the map" understates the recovery: every insert-path call (injectValueUnlocked, PutRetiredWLocked) re-samples too, which is most of why the overshoot stays bounded.
There was a problem hiding this comment.
+1, this might be a blocking issue
| if c.EvictionSlackDivisor == 0 { | ||
| return fmt.Errorf("EvictionSlackDivisor must be greater than 0") | ||
| } | ||
| // Zero would leave eviction unable to find a victim, so the cache would grow without bound. |
There was a problem hiding this comment.
[suggestion] This comment misstates the failure mode. With EvictionSampleSize == 0, visited is incremented to 1 before the visited == c.config.EvictionSampleSize comparison in evictWLocked, so the break never fires and the loop walks the entire entries map, picking the globally oldest entry. Eviction still finds a victim and the cache does not grow without bound — it degrades to an exact-LRU O(n) scan per evicted entry while holding the shard write lock, which on a full shard cache means hundreds of thousands of map steps per victim.
The guard is worth keeping; only the stated reason needs rewriting to the real one (a per-victim full-map scan under the shard lock).
| } | ||
| } | ||
|
|
||
| if victim == nil { |
There was a problem hiding this comment.
+1, this might be a blocking issue
2 participants
Describe your changes and provide context
Restructure view caching to allow better lock parallelism. Uses an approximate LRU instead of a standard LRU.