perf(storage): SCAN cold-plane range resume — hash-ordered cold index (#368) - #382
Conversation
…#368) Completes the two-plane O(COUNT) SCAN page walk. PR #380 made the hot plane a hash-range segment walk; this change gives the cold plane (spilled keys under disk-offload) the same property. ColdIndex's primary map becomes a BTreeMap keyed by (scan_hash48(key), key) — the identical 48-bit cursor hash the hot plane uses (xxh64 top 48 bits). The new `ColdIndex::range_from` + `Database::cold_only_keys_from` seek to the cursor in O(log n) and yield ascending live cold-only candidates; scan_core takes the first COUNT (ascending order makes them exactly the smallest cold candidates) instead of filtering every spilled key on every page. The pathological all-one-hash branch range-scans just the equal-hash group. The cold-only liveness predicate (TTL from the cached ColdLocation + hot-shadow check) is extracted into one shared helper so the full-walk (KEYS/RANDOMKEY) and range-resume paths cannot diverge on the two-plane partition invariant (#364). Trade-offs, deliberate: - lookup/remove pay an O(log n) tree descent instead of an O(1) hash probe via an equal-hash range probe. These sit on disk-read-through, promotion, DEL-of-cold-key, and sweep paths where the descent is noise next to the I/O they front; no command hot path touches them. - The ordered map REPLACES the hash map rather than sidecar-ing it: no duplicate per-entry Bytes, per-entry RAM stays comparable at the G2 tens-of-millions-of-entries scale, and there is no dual-structure drift risk — every mutation still funnels through ColdIndex's own methods (map stays private). - BTreeMap cannot borrow-match a (u64, &[u8]) probe against a (u64, Bytes) key, so remove_raw finds the owned key via the equal-hash range (Bytes clone = refcount bump, no data copy). Public ColdIndex API, recovery rebuild, sweep semantics, and the file_refs reclamation spine are unchanged. sweep_expired's batch-cap comment updated: iteration is now deterministic (hash, key) order; capped-out entries are still never permanently skipped because reclaimed entries stop occupying batch slots. Red/green coverage: range_from hash-order/resume/exactly-once (200 entries, paged), equal-hash-probe lookup/remove independence, SCAN paged drain across planes exactly-once (50 hot + 40 cold + 10 both-planes at COUNT 7), cold spill churn between pages keeps stable keys. All existing cold_enumeration, cold_index, and SCAN semantics tests green on the new path. Refs #368 author: Tin Dang <tindang.ht97@gmail.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe cold index now uses hash-ordered storage to support SCAN cursor range resume. Cold-only enumeration shares liveness checks, and SCAN consumes bounded cold ranges across pages. Tests cover cross-plane draining, churn, ordering, and hash collisions. ChangesSCAN cold-plane range resume
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant SCAN
participant Database
participant ColdIndex
SCAN->>Database: cold_only_keys_from(cursor, now_ms)
Database->>ColdIndex: range_from(cursor)
ColdIndex-->>Database: ordered cold entries
Database-->>SCAN: bounded cold candidates
SCAN->>Database: cold_only_keys_from(h_last, now_ms)
Database-->>SCAN: equal-hash continuation candidates
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (1)
src/storage/tiered/cold_index.rs (1)
514-530: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMake TTL sweep resumable to avoid O(N²) scanning stalls.
The documentation correctly notes that restarting from the front eventually reclaims all expired entries. However, this causes an O(N²) worst-case scanning pattern.
If there are millions of unexpired keys at the front of the ordered map followed by a cluster of expired keys, every sweep tick will scan all the unexpired keys, findmax_batchexpired ones, and break. On the next tick, it starts at the beginning again, rescanning the exact same unexpired keys.Since
self.mapis an orderedBTreeMap, you can completely eliminate this overhead by storing asweep_cursor: u64field inColdIndexand resuming the iterator usingrange.⚡ Proposed fix
First, add a cursor field to the struct (e.g.,
sweep_cursor: u64) and initialize it to0. Then update the sweep loop:- for (k, loc) in self.map.iter() { + for (k, loc) in self.map.range((self.sweep_cursor, Bytes::new())..) { if loc.ttl_ms.is_some_and(|t| now_ms > t) { if expired_keys.len() >= max_batch { more_remain = true; + self.sweep_cursor = k.0; break; } expired_keys.push(k.1.clone()); } } + + if !more_remain { + self.sweep_cursor = 0; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/storage/tiered/cold_index.rs` around lines 514 - 530, Make the TTL sweep resumable by adding and initializing a sweep_cursor field on ColdIndex, then use self.map.range starting from that cursor instead of restarting iteration at the beginning. Advance the cursor as entries are examined, preserve max_batch and expiration filtering, and reset or wrap it appropriately when the ordered keyspace is exhausted so all entries continue to be considered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/storage/tiered/cold_index.rs`:
- Around line 514-530: Make the TTL sweep resumable by adding and initializing a
sweep_cursor field on ColdIndex, then use self.map.range starting from that
cursor instead of restarting iteration at the beginning. Advance the cursor as
entries are examined, preserve max_batch and expiration filtering, and reset or
wrap it appropriately when the ordered keyspace is exhausted so all entries
continue to be considered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 41a520ba-7470-4527-958e-e866b0a51e97
📒 Files selected for processing (4)
CHANGELOG.mdsrc/command/key.rssrc/storage/db.rssrc/storage/tiered/cold_index.rs
…tarting Review follow-up on #382: the pathological all-one-hash branch advanced the cursor to h_last + 1; at the exact 48-bit maximum that value (2^48) is masked back to 0 by the cursor clamp on the next call — an infinite scan restart. Nothing can hash above the max, so the scan is complete: return cursor 0. Not practically constructible (needs a full page of keys at the exact max hash); documented in-line, no test. author: Tin Dang <tindang.ht97@gmail.com>
|
Adversarial review (independent agent, 7 attack vectors incl. two empirical refutation attempts): no defects confirmed. Highlights: |
2 participants
Summary
Completes the two-plane O(COUNT) SCAN page walk (#368). PR #380 made the hot plane a hash-range segment walk; this makes the cold plane (spilled keys under disk-offload) range-resume too.
ColdIndex's primary map becomes aBTreeMapkeyed by(scan_hash48(key), key)— the identical 48-bit cursor hash the hot plane uses. NewColdIndex::range_from+Database::cold_only_keys_fromseek to the cursor in O(log n);scan_coretakes the first COUNT live candidates (ascending order ⇒ exactly the smallest) instead of filtering every spilled key on every page.lookup/removepay an O(log n) descent via an equal-hash range probe — they sit on disk-read-through/promotion/sweep paths where the descent is noise next to the I/O. The ordered map replaces the hash map (no sidecar): no duplicate per-entry RAM at G2 scale and no dual-structure drift risk (the map stays private; every mutation funnels throughColdIndexmethods).file_refsreclamation spine unchanged.Test plan
range_fromhash-order/resume/exactly-once (200 entries paged); equal-hash-probe lookup/remove independence; SCAN paged drain across planes exactly-once (50 hot + 40 cold + 10 both-planes @ COUNT 7); cold spill churn between pages keeps stable keys-D warningsboth feature sets--ignoredreal-server cells): scan_offload_visibility, dbsize_offload_logical, cold_collection_visibility, cold_orphan_sweep, cold_shadow_overwrite_resurrection, crash_recovery_disk_offload_no_aof, crash_recovery_spill_batch_kill9, crash_recovery_cold_del_resurrectionRefs #368
Summary by CodeRabbit
Performance
Bug Fixes
Tests