lazily intialize iterators (#14772)

[stephpontikes]

Summary:

Updated the iterator creation scheme to happen lazily (on request) as oppsed to eagerly. this allows us to prune the iterator tree structure at the time of requesting iterator preparation as opposed to creation, and allows pruning to become an implementation detail. Version now skips non-overlapping SST levels and files before adding children to the iterator tree, returns direct table iterators when a level has a single matching file, and uses pruned LevelIterator instances when multiple files in one non-L0 level match. The overload no longer prepares iterators during creation; callers that need prepared multiscan execution still call Prepare explicitly after construction, and MultiScan does that itself.

Benchmark: ran db_bench in opt mode for the base revision and this diff, with fillseq,compact,levelstats,multiscanrandom, --num=1000000, --reads=10000000, single thread, fixed seeds, --multiscan_use_async_io=false, and --use_multiscan=true. Both A and B had exactly one SST file and no memtable/L0 data (L0: 0 files, L1: 1 file, 61 MB). multiscanrandom creates MultiScanArgs and calls NewMultiScan(...), which reaches the new NewIterator(..., scan_opts) pruning path in this diff.

seed     base A      pruning B    delta
424242   21824.333   17693.333    -18.9%
424243   24042.014   19424.056    -19.2%
424244   22424.974   17636.910    -21.4%
424245   22404.213   18612.840    -16.9%

Average: base 22673.9 us/op, pruning 18341.8 us/op, about 19.1% faster.

Differential Revision: D104904298

[meta-codesync]

@stephpontikes has exported this pull request. If you are a Meta employee, you can view the originating Diff in D104904298.

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 553.5s.

[github-actions]

Codex Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit b180438

---

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not update PATH: Refusing to create helper binaries under temporary dir "/tmp" (codex_home: AbsolutePathBuf("/tmp/codex-home"))
error: the argument '--base <BRANCH>' cannot be used with '[PROMPT]'

Usage: codex exec review --commit <SHA> --base <BRANCH> --title <TITLE> --model <MODEL> --config <key=value> --dangerously-bypass-approvals-and-sandbox --output-last-message <FILE> [PROMPT]

For more information, try '--help'.

---

ℹ️ About this response

Generated by Codex CLI.
Review methodology: claude_md/code_review.md

Limitations:

• Codex may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /codex-review [context] — Request a code review
• /codex-query <question> — Ask about the PR or codebase

[github-actions]

Claude Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit b180438

---

Summary

PR #14772 introduces lazy iterator initialization and MultiScan-based pruning for RocksDB iterators. The design is sound -- deferring internal iterator tree construction to first use enables pruning non-overlapping memtables/files/levels, yielding ~19% improvement for single-SST MultiScan workloads. The SuperVersion lifecycle, error handling, and conservative pruning for range deletions are handled correctly. A few medium-severity issues remain around wasteful Refresh behavior, missing branch hints on hot paths, and test coverage gaps.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

No high-severity findings after cross-agent debate and verification.

🟡 MEDIUM

M1. Wasteful Refresh() on uninitialized iterator -- arena_wrapped_db_iter.cc:249

• Issue: When Refresh() is called on an iterator that was never initialized, EnsureInternalIteratorInitialized(nullptr) builds the full (unpruned) iterator tree, then DoRefresh() immediately destroys and rebuilds it. This doubles the initialization cost.
• Root cause: The Refresh path requires an initialized iterator to properly destroy it, but this is wasteful when the iterator was never used.
• Suggested fix: Add a fast path in Refresh() that skips EnsureInternalIteratorInitialized when !internal_iter_initialized_ and instead directly proceeds to build the refreshed tree, cleaning up only the deferred state. The db_block_cache_test.cc change (adding ASSERT_OK(iter->Refresh()) before SeekToFirst) suggests this path is exercised in practice.

M2. Missing LIKELY/UNLIKELY hints on lazy init hot-path check -- arena_wrapped_db_iter.h:63-98

• Issue: Every Seek, Next, Prev, SeekToFirst, SeekToLast, and PrepareValue call now begins with if (!EnsureInternalIteratorInitialized(nullptr).ok()) return;. After the first call, internal_iter_initialized_ is true and the method returns immediately. However, there is no branch prediction hint.
• Root cause: Standard lazy initialization pattern without hot-path optimization.
• Suggested fix: Use LIKELY(internal_iter_initialized_) at the top of EnsureInternalIteratorInitialized to minimize branch misprediction overhead on every iterator operation.

M3. Pruning creates temporary Arena+Iterator per immutable memtable -- memtable_list.cc:303-328

• Issue: In MemTableListVersion::AddIterators with scan_opts, a full memtable iterator is created with total_order_seek=true, then SeekToFirst/SeekToLast are called just for overlap detection. Expensive for many immutable memtables.
• Suggested fix: Consider adding GetSmallestUserKey()/GetLargestUserKey() metadata to ReadOnlyMemTable to avoid temporary iterators.

M4. Refresh loses pruning state -- arena_wrapped_db_iter.cc:217-228

• Issue: The TODO comment acknowledges that Prepare() scan options are not preserved across Refresh(). After refresh, the iterator includes all memtables/files without pruning.
• Suggested fix: Store the MultiScanArgs used in Prepare() and replay during DoRefresh().

M5. Heap allocations in GetMultiScanOverlappingFiles -- version_set.cc

• Issue: std::vector<size_t> and std::vector<char> allocated on heap during iterator construction.
• Suggested fix: Use autovector or arena allocation.

M6. Arena memory waste from discarded probe iterators -- memtable_list.cc:300-344

• Issue: Memtable iterators allocated from shared arena for overlap probing are not reclaimable after discard.
• Suggested fix: Use separate temporary arena for probing, or check overlap before allocating from main arena.

🟢 LOW / NIT

L1. child_read_options_ initialized twice -- arena_wrapped_db_iter.cc

• Issue: Set in both Init() and EnsureInternalIteratorInitialized(). First is unnecessary.

L2. Missing test: iterator destruction without any operation

• Issue: No test for CleanupDeferredSuperVersion path (create + destroy without use).

L3. Missing test: error during lazy initialization

• Issue: No test injects errors during NewInternalIterator in the lazy init path.

L4. CreationFailure test change needs comment -- db_iterator_test.cc

• Issue: SeekToFirst added to trigger deferred error; rationale should be documented.

Cross-Component Analysis

Context	Executes?	Safe?	Notes
WritePreparedTxnDB	YES	YES	read_callback preserved through lazy init
ReadOnly DB	YES	YES	allow_refresh=false
SstFileReader	NO	N/A	Uses direct Init()
CF deletion before use	YES	YES	SuperVersion holds CFD ref; new tests verify
User-defined timestamps	YES	YES	CompareWithoutTimestamp used correctly
Prefix seek	YES	YES	Pruning uses separate ReadOptions

Positive Observations

• Correct SuperVersion lifecycle: Both happy path and destruction-without-init properly managed.
• Conservative pruning: Memtables with range deletions never pruned (NumRangeDeletion() > 0 => return true).
• Good test coverage: 8 new tests cover L0/level/memtable pruning, CF lifecycle, dedup, edge cases.
• Clean factoring: Well-extracted helpers reduce duplication.
• Solid benchmark: ~19% improvement across 4 seeds.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase

[github-actions]

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit 58b5825

---

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not update PATH: Refusing to create helper binaries under temporary dir "/tmp" (codex_home: AbsolutePathBuf("/tmp/codex-home"))
error: the argument '--base <BRANCH>' cannot be used with '[PROMPT]'

Usage: codex exec review --commit <SHA> --base <BRANCH> --title <TITLE> --model <MODEL> --config <key=value> --dangerously-bypass-approvals-and-sandbox --output-last-message <FILE> [PROMPT]

For more information, try '--help'.

---

ℹ️ About this response

Generated by Codex CLI.
Review methodology: claude_md/code_review.md

Limitations:

• Codex may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /codex-review [context] — Request a code review
• /codex-query <question> — Ask about the PR or codebase

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 58b5825

---

Summary

Solid performance optimization (~19% for MultiScan) with careful lifecycle management. The lazy initialization design is sound: SuperVersion references are properly maintained through deferred state, and sequence numbers are correctly captured at creation time. Several medium-severity issues found around missing branch prediction hints, a potential L0 pruning edge case, and test coverage gaps.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🟡 MEDIUM

M1. Missing LIKELY/UNLIKELY on hot-path lazy-init check -- arena_wrapped_db_iter.h

• Issue: Every Seek/Next/Prev/SeekToFirst/SeekToLast/PrepareValue now calls EnsureInternalIteratorInitialized(nullptr) which checks internal_iter_initialized_. After the first call, this flag is always true. Without UNLIKELY, the CPU branch predictor may not optimally handle this pattern on the very first invocation, and the function call overhead is present on every operation even when it's a no-op.
• Root cause: The EnsureInternalIteratorInitialized function is called unconditionally. After the first call, it returns immediately on internal_iter_initialized_, but the function call + branch is still executed.
• Suggested fix: Consider inlining the fast path check using UNLIKELY:

  void SeekToFirst() override {
    if (UNLIKELY(!internal_iter_initialized_)) {
      if (!EnsureInternalIteratorInitialized(nullptr).ok()) return;
    }
    db_iter_->SeekToFirst();
  }

  Or make EnsureInternalIteratorInitialized inline with the fast path in the header.

M2. ForEachMultiScanOverlappingFile L0 inner-loop break assumes sorted scan ranges -- version_set.cc:~185

• Issue: In the L0 branch, the inner loop over scan ranges breaks when scan_range.start > file.largest_key. This optimization assumes scan ranges are sorted by start key. While ValidateScanOptions enforces this ordering, the pruning functions in version_set.cc and memtable_list.cc are called directly from NewInternalIterator without re-validating. If a future caller passes unvalidated ranges, this would silently skip overlapping files.
• Root cause: The break optimization relies on an invariant (sorted ranges) that is enforced elsewhere (in DBIter::ValidateScanOptions) but not documented or asserted at the usage site.
• Suggested fix: Add a debug-only assert or comment at ForEachMultiScanOverlappingFile entry point documenting the sorted-ranges precondition. Consider: assert(scan_opts.GetComparator() != nullptr) or a more specific ordering check in debug builds.

M3. MultiScanIntersectsMemTable creates temporary iterator for empty-check -- memtable_list.cc:~82

• Issue: For each memtable intersection check, a new Arena + temporary iterator is created, SeekToFirst + SeekToLast are called, and then the arena is destroyed. For immutable memtables, this is called per-memtable in AddIterators. With many immutable memtables (e.g., high write rate with slow flush), this adds non-trivial overhead during iterator creation.
• Root cause: No cached min/max key metadata on memtables to avoid iterator creation.
• Suggested fix: Consider caching smallest/largest user keys on ReadOnlyMemTable to avoid creating temporary iterators. This is a follow-up optimization, not a blocker.

M4. DoRefresh does not preserve Prepare() scan options -- arena_wrapped_db_iter.cc:~219

• Issue: The TODO comment acknowledges this: "Preserve Prepare() scan options across Refresh() so a refreshed MultiScan iterator can rebuild the same pruned tree." After Refresh(), a previously-prepared MultiScan iterator loses its pruning optimization and rebuilds the full iterator tree.
• Root cause: DoRefresh rebuilds the iterator tree from scratch using the basic NewInternalIterator without scan_opts.
• Suggested fix: Store the MultiScanArgs used in Prepare() and pass them through DoRefresh/EnsureInternalIteratorInitialized during refresh. This is acknowledged as a TODO and not a correctness issue, but it's a performance regression for refreshed MultiScan iterators.

M5. db_block_cache_test.cc AddRedundantStats test requires Refresh() to force initialization -- db_block_cache_test.cc:1485

• Issue: The test now needs ASSERT_OK(iter->Refresh()) before SeekToFirst to force iterator materialization. This is a behavioral change: previously NewIterator returned a fully initialized iterator, now it's lazy. While the fix works, it reveals that any code relying on immediate initialization after NewIterator() needs updating.
• Root cause: Lazy initialization means cache statistics aren't populated until the iterator is materialized.
• Suggested fix: Document this behavioral change clearly. Consider whether Refresh() is the right API to force materialization, or if a dedicated Materialize() method would be clearer.

🟢 LOW / NIT

L1. child_read_options_ initialized twice -- arena_wrapped_db_iter.cc

• Issue: child_read_options_ is initialized in Init() (line 101: child_read_options_ = read_options) and again in EnsureInternalIteratorInitialized (line: child_read_options_ = read_options_). The first initialization in Init is unnecessary since it's always overwritten.
• Suggested fix: Remove child_read_options_ = read_options; from Init().

L2. HasBoundedScanRanges() iterates all ranges on every call -- options.h:~2007

• Issue: HasBoundedScanRanges() is O(n) over scan ranges. It's called multiple times during pruning (once per level, once for memtable check). For typical small range counts this is negligible, but could be cached as a bool.
• Suggested fix: Cache the result as a member variable, set during insert() or validated in Prepare().

L3. ReadOptions copy in EnsureInternalIteratorInitialized -- arena_wrapped_db_iter.cc

• Issue: child_read_options_ = read_options_ copies the entire ReadOptions struct. While not huge, this copy happens on every lazy initialization.
• Suggested fix: Minor optimization, not a blocker.

L4. Test sync points use string-based callbacks without namespacing -- version_set.cc

• Issue: New sync points like "Version::AddIteratorsForLevel:AddedFile" and "Version::AddIteratorsForLevel:IteratorType" are added for test observability. These are fine for testing but add marginal overhead in debug builds.
• Suggested fix: No action needed, consistent with existing patterns.

L5. range_tombstone_iter_required_ naming -- version_set.cc

• Issue: The new range_tombstone_iter_required_ flag in LevelIterator gates async IO preparation. The name could be confused with "are range tombstones present in this level" vs "does the caller need range tombstone iteration support".
• Suggested fix: Consider renaming to range_tombstone_iter_slot_provided_ to clarify it indicates the caller provided a slot for the tombstone iterator pointer.

Cross-Component Analysis

Context	Affected?	Assessment
WritePreparedTxnDB	YES - overrides NewIterator	Safe: read_callback_ is set during Init() before lazy init; sequence captured at creation
ReadOnly DB	YES - uses NewArenaWrappedDbIterator	Safe: allow_refresh=false, deferred init works the same
SecondaryInstance	YES - uses NewArenaWrappedDbIterator	Safe: same deferred pattern applies
User-defined timestamps	YES - pruning uses CompareWithoutTimestamp	Safe: MultiScanInternalKey correctly handles timestamp_size > 0
BlobDB	YES - allow_blob_write_path_fallback_ extracted from CFH	Safe: pre-extracted at DBIter construction time
FIFO/Universal compaction	Not directly affected	Safe: pruning operates on file metadata, agnostic to compaction style
Concurrent writers	Not directly affected	Safe: SV reference prevents concurrent cleanup
Old snapshots	YES - child_read_options_.snapshot = nullptr	Safe: sequence_ carries the correct seqno, snapshot pointer not dereferenced

Assumption stress-test results:

• Claim: "Pruning is safe" - CONFIRMED. HasBoundedScanRanges() returns false for unbounded ranges. Memtables with range tombstones are conservatively kept (explicit NumRangeDeletion() > 0 check). Scan ranges are validated as sorted/non-overlapping before pruning.
• Claim: "SuperVersion is properly managed" - CONFIRMED. GetReferencedSuperVersion adds a ref. CleanupDeferredSuperVersion calls CleanupIteratorSuperVersion which does Unref(). After lazy init, SV ownership transfers to the SuperVersionHandle cleanup registered on the internal iterator.
• Claim: "CFD is safe" - CONFIRMED. SV holds a reference to CFD. As long as deferred_sv_ is alive, CFD cannot be freed. Tests IteratorSeekAfterCfDelete and IteratorSeekAfterCfDrop verify this.
• Claim: "Setting snapshot to nullptr is safe" - CONFIRMED. LevelIterator now uses explicit read_seq parameter. MemTableListVersion::AddIterators uses explicit read_seq. The snapshot pointer is not needed.

Positive Observations

1. Clean lifecycle management: The DestroyDBIter / CleanupDeferredSuperVersion / DestroyDBIterAndArena decomposition is well-structured and handles all cleanup paths correctly.
2. Conservative memtable pruning: The check NumRangeDeletion() > 0 correctly prevents pruning memtables that might have cross-range tombstones.
3. Good test coverage: New tests cover L0 pruning, level pruning, memtable pruning, multi-range dedup, CF deletion/drop before seek, and repeated Prepare rejection.
4. Refactoring of range_del_iter ownership: Moving from raw new/delete to std::unique_ptr<FragmentedRangeTombstoneIterator> in memtable_list.cc is a good cleanup.
5. DBIter CFH decoupling: Replacing cfh_ with pre-extracted fields (trace_db_, trace_cf_id_, ingest_sst_lock_, etc.) is a sound design that removes the DBIter's dependency on CFH lifetime.
6. Sequence number parameter threading: Passing read_seq explicitly through AddIterators -> AddIteratorsForLevel -> LevelIterator constructor is more robust than relying on read_options.snapshot which may be released.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase