Prototype WIP - Add prefetch support for reverse scan#14856
Conversation
|
| Check | Count |
|---|---|
clang-analyzer-deadcode.DeadStores |
1 |
modernize-make-shared |
1 |
| Total | 2 |
Details
table/block_based/block_prefetcher.cc (1 warning(s))
table/block_based/block_prefetcher.cc:119:5: warning: Value stored to 'in_prefetch_range' is never read [clang-analyzer-deadcode.DeadStores]
table/table_test.cc (1 warning(s))
table/table_test.cc:3397:25: warning: use std::make_shared instead [modernize-make-shared]
Codex Code Review - OBSOLETESuperseded by a newer AI review. Expand to see the original review.🟡 Codex Code ReviewAuto-triggered after CI passed — reviewing commit 4b45dca ❌ Codex review failed before producing findings. ℹ️ About this responseGenerated by Codex CLI. Limitations:
Commands:
|
Claude Code Review - OBSOLETESuperseded by a newer AI review. Expand to see the original review.✅ Claude Code ReviewAuto-triggered after CI passed — reviewing commit 4b45dca SummaryThis WIP prototype plumbs a High-severity findings (3):
Full review (click to expand)Findings🔴 HIGHH1.
|
| Context | Does code execute? | Assumptions hold? | Action needed? |
|---|---|---|---|
| Compaction | NO | YES | Always forward; correctly commented |
| PartitionedIndexIterator | YES | NO | Not updated for direction |
| Async IO | YES | NO | Direction passed but not used for actual prefetch |
| ReadaheadCacheLookup | YES | YES | Correctly disabled for backward |
| WritePreparedTxnDB | NO | N/A | Not affected |
| ReadOnly DB | NO | N/A | Not affected |
Positive Observations
- The approach of threading
PrefetchDirectionthrough the call chain is architecturally sound. - Correctly disabling
readaheadsize_cbfor backward direction is a good catch. - The
IsBlockSequentialbackward overload (offset + len == prev_offset_) is correct. - The PR correctly identifies that compaction is always forward.
ℹ️ 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
Enable RocksDB block prefetching for reverse iteration to achieve parity with forward scan. Add PrefetchDirection enum plumbed through FilePrefetchBuffer, BlockPrefetcher, and BlockBasedTableIterator. Implement backward sequential detection, backward FS prefetch range, and direction-aware buffer outdated logic and ReadAheadSizeTuning. Fix useful counter to work for backward direction by prefetching correct byte range. Add unit test ReversePrefetchTest verifying prefetch buffer creation and prefetch bytes ticker on reverse path.
4b45dca to
db12909
Compare
🟡 Codex Code ReviewAuto-triggered after CI passed — reviewing commit db12909 ❌ Codex review failed before producing findings. ℹ️ About this responseGenerated by Codex CLI. Limitations:
Commands:
|
|
/claude-review |
✅ Claude Code ReviewAuto-triggered after CI passed — reviewing commit db12909 SummaryPrototype adds direction-aware prefetching for reverse scans through High-severity findings (3):
Full review (click to expand)Findings🔴 HIGHH1. Self-contradictory
|
| Context | Executes? | Safe? | Action? |
|---|---|---|---|
| WritePreparedTxnDB | YES | YES | None |
| ReadOnly DB | YES | YES | None |
| Compaction | NO | N/A | Correctly excluded |
| Async IO (num_buffers>1) | PARTIALLY | NO | M2 |
| Direct IO | YES | UNCERTAIN | Untested |
| FS prefetch | YES | PARTIAL | H1 |
Positive Observations
- Clean design threading direction through
ReadaheadParams - Backward
IsBlockSequentialoverload inBlockPrefetcheris correct - Backward range calculation in
ReadAheadSizeTuninghandles underflow properly - Compaction path correctly unchanged
IsPrevBlockOutOfReadaheadBoundcorrectly mirrors forward version
ℹ️ 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
✅ Claude Code ReviewRequested by @jaykorean SummaryPrototype WIP adding backward prefetch for reverse iteration. The approach is sound -- plumbing High-severity findings (2):
The full review with 4 MEDIUM and 5 LOW findings is written to ℹ️ About this responseGenerated by Claude Code. Limitations:
Commands:
|
Summary
Enable RocksDB block prefetching for reverse iteration (Prev / SeekForPrev / SeekToLast) to achieve parity with existing forward scan prefetch. Currently BlockPrefetcher and FilePrefetchBuffer assume monotonic increasing file offsets, so reverse scans get zero prefetch benefit even with explicit readahead_size or implicit auto-readahead enabled.
This PR adds direction-aware prefetch through the full iterator stack, fixes useful-bytes accounting for backward direction, and adds unit test coverage for reverse path. Bench harness updated locally for T3 evaluation.
Motivation
Forward scan today has three prefetch modes — explicit user readahead_size, implicit auto-readahead after 2 sequential reads with exponential growth, and compaction readahead — all gated by forward sequential detection
prev_offset+prev_len == offset. Reverse workloads doing SeekToLast + Prev or SeekForPrev see none of this today.Typical secondary index reverse lookup pattern targeted: 10KB average value size, 5–20 keys per scan, with and without prefetch comparison needed to quantify uplift on T3 NVMe hosts.
Design
No public API change. Internal direction enum added with default forward preserving existing behavior.
New types –
file/file_prefetch_buffer.henum class PrefetchDirection { kForward, kBackward }ReadaheadParams.directiondefaults to forward for backward compatBufferInfoextended withIsBufferOutdatedBackward(offset,len)andIsBufferOutdatedBackwardWithAsyncProgressfor symmetry with existing forward outdated checksFilePrefetchBuffer –
file/file_prefetch_buffer.h/.ccdirection_member initialized from ReadaheadParams, plumbed through constructor.ClearOutdatedData(offset,length)now branches on direction: forward frees front buffer whenoffset >= buf_end; backward frees front buffer whenoffset+length <= buf_start. With single buffer case this correctly slides window backward; with double-buffer async case overlapping logic updated to check backward alignmentnext_buf.offset + next_buf.size == buf.offsetand abort when request extends before current buffer start.AbortOutdatedIO(offset,length)similarly direction-aware using new backward outdated check.ReadAheadSizeTuning()fixed to compute correct prefetch range for backward direction:[offset , offset+len+readahead)— unchanged[ max(0, offset-readahead) , offset+len )— new, mirrors BlockPrefetcher FS prefetch fixUpdateStats()unchanged — now correctly recordsPREFETCH_BYTES_USEFULandPREFETCH_HITSon backward cache hits because TryReadFromCache no longer returns false spuriously due to wrong range calculation. Previously useful was always 0 on reverse path; after fix useful >0 on hits.BlockPrefetcher –
table/block_based/block_prefetcher.h/.ccPrefetchIfNeeded(..., PrefetchDirection direction = kForward)new signature default preserves backward compat for existing callers.IsBlockSequential(offset,len,direction)overload added checking both forwardprev_offset+prev_len==offsetand backwardoffset+len==prev_offset.prefetch_start_lower bound alongside existingreadahead_limit_upper bound for FS prefetch range checks.rep->file->Prefetch(start, len)then updatesprefetch_start_andreadahead_limit_.BlockBasedTableIterator –
table/block_based/block_based_table_iterator.h/.ccInitDataBlock(PrefetchDirection)andAsyncInitDataBlock(..., direction)updated with default forward.kBackward; Next path, Seek, SeekToFirst, FindKeyForward use default forward.readahead_cache_lookupcallback disabled for backward direction in prototype — forward continues to useBlockCacheLookupForReadAheadSizewalking index forward for trimming; backward trimming left as follow-up to keep prototype focused. Means useful ratio on reverse may still lag forward until backward trim walk via Prev() is implemented.Compaction path unchanged — compaction iterator always forward, no behavior change.
Files Changed
file/file_prefetch_buffer.h– PrefetchDirection enum, ReadaheadParams.direction, BufferInfo backward outdated methods, FilePrefetchBuffer direction member, UpdateStats unchanged but now triggered correctly for backwardfile/file_prefetch_buffer.cc– direction-aware ClearOutdatedData, AbortOutdatedIO, ReadAheadSizeTuning backward range calculation, prev_buf logic guarded to forward onlytable/block_based/block_prefetcher.h– PrefetchIfNeeded signature, IsBlockSequential overload, prefetch_start_ member, ResetValues updatedtable/block_based/block_prefetcher.cc– backward sequential detection, backward FS prefetch range, direction plumbing to ReadaheadParamstable/block_based/block_based_table_iterator.h– InitDataBlock signature updatetable/block_based/block_based_table_iterator.cc– plumb direction from Prev/SeekForPrev/SeekToLast, disable cache lookup callback for backward, pass direction to prefetchertable/table_test.cc– new TEST_P ReversePrefetchTest covering explicit readahead and implicit auto on reverse pathTest Plan
Unit tests passing locally on devvm:
make table_test && ./table_test --gtest_filter="*Prefetch*"— 146 passed (74 existing forward PrefetchTest + 72 new ReversePrefetchTest across format versions)make table_test && ./table_test --gtest_filter="*ReversePrefetch*"— 72 passedmake prefetch_test && ./prefetch_test— 110 passed unchangedBench harness local devvm run with prototype binary (devvm33065, not T3 yet, for functionality verification):
Follow-ups before Ready for Review -> Ready
file/prefetch_test.ccsimilar to existing PrefetchTest.Basic but using SeekToLast Prev loop asserting fs prefetch called and prefetch bytes >0 — table_test covers table layer already, DB test would add end-to-end coverage through DB iterator merging logic.Testing Done Checklist
table/table_test.ccReversePrefetchTest 72 parameterized sub-tests passing