Repository navigation
Conversation
|
| Check | Count |
|---|---|
performance-inefficient-vector-operation |
2 |
| Total | 2 |
Details
utilities/trie_index/trie_index_test.cc (2 warning(s))
utilities/trie_index/trie_index_test.cc:296:5: warning: 'push_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation]
utilities/trie_index/trie_index_test.cc:321:5: warning: 'push_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation]
cd9b272 to
a4d2c11
Compare
Claude Code Review - OBSOLETESuperseded by a newer AI review. Expand to see the original review.✅ Claude Code ReviewAuto-triggered after CI passed — reviewing commit a4d2c11 SummaryThis PR adds trie index format 2 (path-compressed trie over front-coded buckets with frame-of-reference columns) and fixes several correctness bugs in format 1 (empty user key handling, malformed block validation, memory accounting). The changes are well-structured and the bug fixes are sound. I identified 3 HIGH and 5 MEDIUM findings, none of which are blockers -- the HIGHs are existing bugs that this PR correctly fixes. High-severity findings (3):
Full review (click to expand)Findings🔴 HIGHH1:
H2: At H3: No unknown flags rejection (existing bug, fixed by PR) Current 🔶 MEDIUMM1: Per CLAUDE.md, defaulted parameters are the "#1 trap on refactoring." The change from M2: Format version dispatch asymmetry (write vs. read)
M3: The extraction of scan bound checking from M4: db_crashtest.py module-level random choices reduce per-invocation coverage diversity
M5: Relaxed atomics in
⚪ LOW / OBSERVATIONSL1: Empty key fix is correct The L2: ~200us per SST open is well within tolerance. The validation runs once per SST open for format 1 indexes and catches structural corruption that would otherwise cause silent wrong results or crashes during read operations. L3: Format 2 portability improvement Format 2 uses L4: Good defense-in-depth. Prevents crafted input from causing oversized vector allocations that could OOM the process during SST open. Cross-Component Analysis
Test Coverage AssessmentThe PR should include tests for:
Behavioral CompatibilityNo breaking behavioral changes for existing users:
ℹ️ About this responseGenerated by Claude Code. Limitations:
Commands:
|
a4d2c11 to
30ff02b
Compare
zaidoon1
changed the title
339b04d to
5942de8
Compare
✅ Claude Code ReviewAuto-triggered after CI passed — reviewing commit 5942de8 SummaryThis is a well-structured replacement of the experimental LOUDS trie index with a new block-tree format. The changes are comprehensive: build system updates, public API documentation, validation flow improvements, crash test coverage, and release notes. The approach of keeping the same magic but bumping the version field to detect incompatible formats is sound. High-severity findings (1):
Full review (click to expand)Findings🔴 HIGHH1. Missing
|
| Context | Affected? | Assessment |
|---|---|---|
| WritePreparedTxnDB | No | Trie index operates at block-level, not key-level visibility |
| ReadOnly DB | No | Reader-only, no builder involvement |
| CompactionService | Potentially | If remote compaction builds SSTs with trie, the new format must be serializable |
| User-defined timestamps | No | Explicitly rejected by existing check + new ValidateOptions |
| MemPurge | No | Trie index is for SST files only |
| BlobDB | No | Trie indexes data blocks, not blob files |
| FIFO / Universal compaction | No | Index format is independent of compaction style |
| Prefix seek | No | Trie operates on full keys, prefix is handled by data iterator |
| SetOptions | Indirectly | index_mode change through SetOptions only affects newly opened SSTs — now documented |
DB Mutex Analysis
The changed code paths (ValidateOptions, factory registration, crash test parameters) do not execute while DBImpl::mutex_ is held. ValidateOptions runs during DB::Open before the mutex is typically contested. No DB mutex impact.
Positive Observations
- Well-structured compatibility story: Version field bump with same magic, clear upgrade/downgrade paths documented in release notes.
- Crash test block_size diversification: Good insight that 16 KiB blocks produce too few separators for meaningful trie testing.
- SetValidateFunc pattern: Clean separation of per-option validation (bypassed) vs. explicit mode-aware validation in ValidateOptions. This correctly handles kStandardOnly ignoring the factory.
- Documentation update for SetOptions routing: The clarification that "index routing is fixed when a table reader opens" is an important correction of prior documentation that implied immediate effect.
- Release notes: Appropriately detailed for a behavior change with compatibility impact, without being overly verbose.
ℹ️ About this response
Generated by Claude Code.
Review methodology: claude_md/ci_review_prompt.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
0a8e540 to
f6c034c
Compare
The LOUDS trie index was larger than the standard index on every key shape tried, and up to 2x slower on Get. Replace it with a block-tree layout: a shared prefix, front-coded buckets of 8 separators, a path-compressed trie over the bucket heads, and frame-of-reference offset columns. The old format has no known users, so it is removed rather than kept readable. Its blocks fail the version check, and earlier releases reject the new blocks the same way. The new builder stores the empty user key and 64-bit block handles. Opening an index checks everything but the separators inside buckets, whose bounds are checked as they are decoded, and bounds the key length an iterator reserves. DB::Open now rejects a non-bytewise comparator except in kStandardOnly, which ignores the factory: BlockBasedTableFactory calls the factory's ValidateOptions() only in the modes that use it. Also correct the rollback text in table.h and the IndexFactory doc: an index_mode change through SetOptions() only reaches SSTs opened afterwards. When the trie is on and no block size is given, the crash test now draws smaller block sizes, so its SSTs have enough separators to reach multi-bucket tries and user keys that span blocks.
f6c034c to
4be1067
Compare
|
@xingbowang has imported this pull request. If you are a Meta employee, you can view this in D129647129. |
pdillinger
left a comment
pdillinger
left a comment
There was a problem hiding this comment.
Old trie blocks: NotSupported rather than Corruption
utilities/trie_index/trie.cc, Trie::Parse():
if (DecodeFixed32(p + kVersionPos) != kVersion) {
return Corrupt("unsupported format version");
}A version-1 block isn't corrupt; it's a valid block in a format this release no longer reads. The status matters for anyone upgrading in a mode that requires the trie. That includes users who never set index_mode: GetEffectiveIndexMode() maps the legacy use_udi_as_primary_index to kCustomDefault and fail_if_no_udi_on_open to kStandardRequired. In those modes the table open fails, and with max_open_files=-1 so does DB::Open(), with a status that operators and automation can reasonably read as data corruption (e.g. restore from backup) rather than "change an option and compact".
Suggest returning NotSupported for a recognized magic with an unrecognized version, with a message that says what to do, e.g.:
const uint32_t version = DecodeFixed32(p + kVersionPos);
if (version != kVersion) {
return Status::NotSupported(
"Trie index: format version " + std::to_string(version) +
" is not readable by this release; open with "
"index_mode=kStandardDefault and compact to rewrite the trie");
}kStandardDefault behavior is unchanged: the fallback in BlockBasedTable::Open() already treats IsNotSupported() like IsCorruption(). And the advice is always actionable: as far as I can tell, no released version writes trie SSTs without a full standard index (stub-only writing came with #14963, which isn't in a release branch), so every old SST is readable through kStandardDefault.
TrieTest.RejectsOtherVersions could then assert the status code, and a DB-level test could cover an old-version block falling back in kStandardDefault and failing with NotSupported in kCustomDefault. The summary's "each version rejects the other's blocks with Corruption" would need updating for the new-reads-old direction.
Write, read, and table open benchmarks through the DB
The numbers in the summary isolate the index nicely (identical data blocks via SstFileWriter + ingest, everything cached), but they don't show the end-to-end cost on the normal write and read paths, or the cost of opening a table reader. I'd like to see db_bench runs along these lines:
Write: fillrandom (or fillseq + overwrite) with normal flush and compaction, comparing
--use_trie_index=0(baseline)--use_trie_index=1 --index_mode=1(kStandardDefault: both indexes built, the rollout mode)--use_trie_index=1 --index_mode=3(kCustomOnly: trie only)
reporting throughput and flush/compaction time or CPU.
Read: on the resulting multi-level DB (--use_existing_db=1), readrandom, seekrandom with --seek_nexts, and a reverse scan (readreverse or seekrandom --reverse_iterator=1), comparing --index_mode=1 vs --index_mode=2 (kCustomDefault).
Table open: every table reader open reads the trie block, and Trie::Init() validates it in full: every block offset is decoded and checked, along with the bucket table, the side tables, and a walk of the whole trie. That is linear in the number of blocks in the SST, and unlike the standard index with cache_index_and_filter_blocks=true, the custom index is loaded eagerly at open. By the summary's own numbers it adds roughly 20-30 us per 64 MiB SST over restart 16. That cost is paid by DB::Open() with max_open_files=-1, on every table cache miss with a bounded max_open_files, and for each new flush or compaction output. Please include DB open time on a DB with many SSTs, and readrandom with a small --open_files so table readers are reopened. If it shows up, the per-block offset check looks like the main candidate: as far as I can tell it exists only so that derived block sizes cannot wrap, which could instead be checked where value() derives the size.
tools/db_crashtest.py: is the mmap_read=0 exclusion still needed?
finalize_and_sanitize() still forces mmap_read=0 with the trie:
# Trie UDI uses zero-copy pointers into block data, which is
# incompatible with mmap_read.
dest_params["mmap_read"] = 0The new format is read in place with unaligned loads, at any alignment. What is the remaining incompatibility with mmap_read? If it no longer applies, the exclusion can go and the crash test regains mmap_read coverage with the trie; if it does, the comment should say why.
| @@ -1,3 +1 @@ | |||
| Custom index factories are rejected with user-defined timestamps in every index mode except `kStandardOnly`, which ignores the factory. Custom-index block-cache hits, misses, and adds now contribute to `BLOCK_CACHE_INDEX_*` instead of `BLOCK_CACHE_DATA_*`. | |||
|
|
|||
| Trie index construction returns `NotSupported` when a data-block offset or size exceeds `UINT32_MAX`, including when the trie is optional in `kStandardDefault`. Flush, compaction, and `SstFileWriter::Finish()` can therefore fail for such output; an SST larger than 4 GiB is not itself the rejection condition. File-size targets do not split L0 outputs or `SstFileWriter` outputs. Bound those outputs or disable trie construction with `kStandardOnly`. | |||
There was a problem hiding this comment.
This editing is not going to make sense with a release cut in the middle. Please only add new release notes. I generally prefer one release note per feature, choosing the most appropriate section.
- A trie block of another version now fails with NotSupported and a hint to compact in kStandardDefault, instead of Corruption. The version is checked before the header size. - Init() no longer decodes every block offset or bucket position. The iterator reads a block's handle when it lands on it and returns Corruption if a derived size would wrap; bucket positions are clamped to the bucket data, where every entry is bounds-checked. Init on a 14.8K-block index goes from 26-28 us to 5-8 us. - db_crashtest.py and db_stress no longer turn mmap_read off with the trie; trie reads are tested with allow_mmap_reads. - The edits to existing release notes are reverted, and the trie changes get a single new note.
|
Thanks, this was useful. Everything is addressed in 74fe042, and the summary is updated. Old trie blocks: NotSupportedDone, with your message. The version is now checked right after the magic and before the header size, so a block whose format has a shorter header still gets NotSupported. One correction to the reasoning: 11.14.fb was cut today (3f56cbf) and it contains #14963. So 11.14.0 will be able to write version-1 tries into kCustomOnly SSTs, which have only the stub standard index. After this PR, nothing can read those files. The release note now tells users to rewrite them in another mode before upgrading. If you'd rather 11.14.0 not ship that state, picking this PR into 11.14.fb before the tag would avoid it. BenchmarksThey're in the summary under "Through the DB". In short: on a 56-SST LZ4 DB, Get through the trie was 3% faster than through the standard index, and Seek with 10 nexts was 7% faster in both directions. fillrandom throughput was within noise in all three modes. Building both indexes in kStandardDefault cost about 3% more compaction CPU, and kCustomOnly matched the baseline without the trie. Open timeIt showed up, and the offset check was the main cost, as you guessed. On a 14.8K-block index it took about 14-18 us of Init's 25-28 us, and the bucket table loop took another 2-4 us. Both are gone from Init:
Init on that index went from 26-28 us to 5-8 us; most of what's left is the trie walk. With mmap_readThe exclusion no longer applies, so it's gone from db_crashtest.py and db_stress. The trie keeps pointers into its block, but the block outlives the reader. An mmap'd block is copied to the heap before it goes into the block cache. Otherwise it stays uncached and points into the table reader's own mapping.
I also couldn't reproduce a crash on main with the exclusion removed: 10 minutes per mode, about half of it with mmap on. So I can't say what the original failure was. Release notesThere's now a single new note, in behavior_changes. It covers the format, the upgrade path including the kCustomOnly case, the empty-key fix and the comparator check. |
This replaces the on-disk format of the experimental trie index (
TrieIndexFactory). The LOUDS format is deleted outright, with no format option or migration path. As far as I know nobody uses it.Why not keep LOUDS
The point of the trie was an index smaller than the standard one, and LOUDS never got there. On every key shape I tried it was 1.3x to 2.7x bigger than the standard index even at restart interval 1, and Get was up to 2x slower. Most of the bytes go to per-edge child position tables, which also get copied to the heap when the SST opens, and to a seqno table that's written whether it's needed or not.
It also has bugs:
DB::Openaccepts a non-bytewise comparator, and then every flush failsFixing those would still leave an index that loses to the standard one, so I replaced the format.
The new format
It's the block-tree layout Lucene and tantivy use. The prefix shared by all separators is stored once. Separators are grouped 8 to a bucket and front coded, and a path-compressed trie over each bucket's first separator sends a Seek to one bucket. Block offsets are a linear frame-of-reference column, and sizes come from consecutive offsets when blocks are evenly spaced. Two small side tables cover user keys that span several blocks and blocks that need a seqno tag. Everything is little-endian, 64-bit and read in place; the spec is at the top of
utilities/trie_index/trie.h.Opening an index validates the header, the section and column layouts, the side tables and the trie. It doesn't decode every block offset or bucket position. Instead the iterator checks a block's derived size when it lands on the block, keeps bucket reads inside the bucket data, and bounds-checks every entry it decodes, returning Corruption for anything out of bounds. Like the other index blocks, the trie relies on the block checksum for anything subtler.
#14409 turned
mmap_readoff in the crash test and db_stress because of LOUDS's zero-copy pointers. The new reader also reads its block in place, but the block always outlives it: an mmap'd block points into the table reader's own mapping, and the block cache only stores heap copies. So the exclusion is gone.TrieIndexFactory::ValidateOptions()rejects non-bytewise comparators, soDB::Open()andCreateColumnFamily()fail up front instead of every flush failing later.SetOptions()doesn't run that validation yet; #15582 fixes that.kStandardOnlyignores the factory, so it still works with any comparator.The docs also said rolling back from kCustomDefault takes effect immediately. It doesn't: index routing is fixed when a table reader opens, so the docs now say an
index_modechange only reaches SSTs opened afterwards.Compatibility
This is a one-way door for any DB that writes SSTs with the trie. DBs without
TrieIndexFactory, or runningkStandardOnly, aren't affected. The block keeps the old magic but bumps the version to 2.The release note covers this.
Numbers
Isolated index
Release db_bench on an Apple Silicon laptop, single thread. 3M keys per shape, 100-byte values, no compression, 4 KiB blocks, format_version 8, 10-bit bloom, everything in the block cache. Each variant is written with SstFileWriter from the same sorted keys and ingested, so the data blocks are identical and only the index differs. Medians of 3 rounds in random order; anything under about 3% is noise.
Shapes:
rand16is 16 random bytes,uuida textual UUIDv4,urlreversed host + path + id,tsdbseries id + tag hash + timestamp, anddbintdb_bench's--key_size=48 --prefix_size=32 --keys_per_prefix=100.So it's about the size of restart 16 with lookups as fast as restart 1 or better, and reverse seeks and build time are a wash. Where it doesn't win:
dbint, and 3-5% bigger on long URL-like keys with 16 KiB blocksThrough the DB
Release db_bench on the same laptop:
fillrandomof 20M keys with 16-byte keys, 100-byte values, LZ4, 4 KiB blocks, 16 MiB target files and 10-bit bloom, then reads on the resulting DB (56 SSTs over L0-L2) with an 8 GiB block cache warmed byreadseq. Writes are the median of 2 runs, everything else of 3.The two no-trie runs were 5% apart, so only the compaction CPU of building both indexes in kStandardDefault (about 3%) looks real.
Table open:
Trie::Init()used to decode every block offset and the whole bucket table, which was about two thirds of its time. Those checks moved into the iterator, and Init on a 14.8K-block index went from 26-28 us to 5-8 us; most of what's left is the trie walk. With--open_files=20, which leaves room for 10 table readers, most Gets reopen a table:So under table cache churn the trie adds about 7 us per Get, down from about 20 us.
DB::Open()of the 56-SST DB took 25-27 ms in every variant; at that size, loading tables on 16 threads is lost in the noise.Testing
trie_index_test(96 tests),trie_index_db_test(277) andtable_testpass, also withASSERT_STATUS_CHECKED=1. Besides randomized Seek/Next/Prev against a model, golden bytes and malformed blocks, they cover NotSupported for other versions, bad offsets and bucket positions failing on read, an old-version block throughDB::Open()in kStandardDefault, kStandardRequired and kCustomDefault, and trie reads withallow_mmap_readsacross table cache evictions, compaction and reopen.db_crashtest.py blackbox --simple --use_trie_index=1 --mmap_read=1ran 15 minutes each in index modes 1 and 3, with mmap on in all 32 invocations.