[rbhanot]

Summary:
Adds a concurrent sorted-hash MemTableRep for point-lookup-heavy column families. The WAL and SST formats are unchanged, and RocksDB's default memtable stays skip_list.

Why: a skip_list point lookup descends about log2(n) levels with a likely cache miss at each. Workloads that are almost entirely whole-key Get/MultiGet (for example Hypernode's chunk-metadata column family) pay that on every lookup. A whole-key hash memtable reaches the entry through one short, sorted chain instead.

New behavior

• NewSortedHashRepFactory(bucket_count), also sorted_hash:<bucket_count> and id=SortedHashRepFactory;bucket_count=N (so OPTIONS files round-trip). There is no default bucket count.
• Each bucket is a linked list in internal-key order. Inserts take a per-bucket SpinMutex and run concurrently across buckets (IsInsertConcurrentlySupported()); readers are lock-free; duplicate <key, seq> inserts return false.
• Ordered iteration sorts all entries into a cached pointer view, rebuilt only after new inserts. The view is built once when the memtable becomes immutable (MarkReadOnly()), so immutable-memtable memory accounting does not change afterwards.
• Column families using this factory fail DB::Open, CreateColumnFamily and SetOptions when the rep cannot honor the configuration: user-defined timestamps or a comparator under which byte-different keys can compare equal (bucket hashing would miss keys); paranoid_memory_checks or memtable_verify_per_key_checksum_on_seek (validated reads are not implemented); inplace_update_support (each update would rebuild the view); bucket_count == 0; or a bucket array at least as large as write_buffer_size.
• Tailing iterators (ReadOptions::tailing) fail with NotSupported, because this rep's iterators see a fixed view.

Implementation

• memtable/sorted_hash_rep.cc: the rep and factory; SortedHashRepFactory::ValidateOptions() implements the rejections. Lookups decode the memtable key once. ApproximateMemoryUsage() reads the cached view size from an atomic rather than taking the view mutex on every write. The entry count that versions the cached view is incremented with release ordering, pairing with the reader's acquire load.
• New MemTableRepFactory::IsTailingIteratorSupported() (default true); DBImpl::NewIterator/NewIterators fail tailing iterators when it returns false.
• ColumnFamilyData::ValidateOptions() now also calls memtable_factory->ValidateOptions(), so factory checks run on CreateColumnFamily and SetOptions (covering mutable options such as paranoid_memory_checks and write_buffer_size), not only DB::Open.
• db_stress --memtablerep=sorted_hash (one bucket per KiB of write buffer); db_crashtest.py adds it to the memtablerep choices with UDT and in-place updates forced off. DBTestBase gains a kSortedHash option config, skipped by the two UDT ingestion tests that loop over configs.
• Header docs with sizing guidance and limitations, release notes, and build files (BUCK regenerated by buckifier).

Safety, compatibility and limitations

• Opt-in. The only change for other memtable factories is that their ValidateOptions() also runs on CreateColumnFamily and SetOptions; the built-in factories register no validated options.
• Read-only, secondary and follower instances do not validate options, so the rejections are not enforced there.
• Chains never resize, so bucket_count must scale with write_buffer_size.
• Creating an iterator after any write is O(n log n) and visits every bucket. Older views held by live iterators are not counted toward memtable memory, and no view memory is charged to the WriteBufferManager.
• Seeks while a writer is active pay the view rebuild on almost every new iterator: about 9x slower than skip_list with an 8 MiB memtable, and the cost grows with memtable size. Workloads that create many iterators while writing should keep skip_list.
• Deferred to follow-ups: timestamp-stripped hashing for UDT, the *AndValidate read methods, live (tailing-capable) iteration, accounting for pinned views, and a bucket-only GetDynamicPrefixIterator for max_successive_merges.

Benchmarks (memtable-resident db_bench A/B on this version, ~130-byte entries, one bucket per KiB of write buffer; full table in the test plan): sorted-hash was 53-65% faster on point reads, 46-51% faster on MultiGet, 55-58% faster on single-threaded inserts and 21-38% with 8 writer threads, and 15-40% faster on readwhilewriting; readseq was unchanged. perf stat CPU per point read dropped 45-64%, and the full insert path with flushes dropped 4-10%. A fixed 4096 buckets at ~600K entries made point reads 3.5-5x slower than skip_list.

Next in stack: wire the memtable into Hypernode RocksKVStore behind a default-off config.

Differential Revision: D119258724

[meta-codesync]

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

[github-actions]

⚠️ clang-tidy: 1 warning(s) on changed lines

Completed in 489.3s.

Summary by check

Check	Count
performance-inefficient-vector-operation	1
Total	1

Details

db/db_memtable_test.cc (1 warning(s))

db/db_memtable_test.cc:888:5: warning: 'emplace_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation]

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 080a7ec

---

Summary

Well-designed, opt-in memtable implementation with solid test coverage and correct concurrent data structure semantics. The core hash-table-with-sorted-chains approach is sound, thread safety via per-bucket SpinMutex and release/acquire ordering is correct, and the behavioral restrictions are properly enforced through ValidateOptions.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. NewIterators tailing check is placed after SuperVersion acquisition — db/db_impl/db_impl.cc

• Issue: In NewIterator, the FailIfTailingIteratorUnsupported check is placed before GetReferencedSuperVersion (good — no resource leak on failure). In NewIterators, the check is placed inside the validation loop after all SuperVersions have been acquired via MultiCFSnapshot. While the code correctly calls cleanup_super_versions() on failure, this acquires and immediately releases SuperVersion refs for a request that could have been rejected earlier (before MultiCFSnapshot).
• Root cause: Inconsistent placement between NewIterator and NewIterators. The tailing flag is known before the SuperVersion loop.
• Suggested fix: Move the tailing check before the MultiCFSnapshot call. The current approach is functionally correct, so this is a consistency/efficiency nit.

M2. Factory registration creates factory with bucket_count=0 for bare name — table/plain/plain_table_factory.cc

• Issue: When CreateFromString is called with just "SortedHashRepFactory" (no colon), the factory creates NewSortedHashRepFactory(0), producing a factory that fails ValidateOptions ("bucket_count must be positive") when any column family tries to use it. This differs from NewHashLinkListRepFactory() and NewHashSkipListRepFactory() which have sensible defaults.
• Root cause: Intentional ("There is no default bucket count"), but creates a factory in an unusable state.
• Suggested fix: Consider returning an error from the factory lambda when no bucket_count is provided (set *errmsg and return nullptr), or document this clearly.

M3. GetDynamicPrefixIterator defaults to GetIterator — perf concern for max_successive_merges

• Issue: SortedHashRep does not override GetDynamicPrefixIterator, so the max_successive_merges counting logic (memtable.cc:2063,2126,2220) uses the sorted-view iterator, triggering an O(n log n) rebuild on every merge check. SortedHashRep::Get() is overridden with efficient hash lookup, so normal Get path is fine — only max_successive_merges > 0 is affected.
• Root cause: Deferred to follow-up per PR description.
• Suggested fix: Document this limitation. Consider rejecting max_successive_merges > 0 in ValidateOptions if the performance impact is severe.

🟢 LOW / NIT

L1. Header comment count mismatch — include/rocksdb/memtablerep.h:32

• Issue: "The last four implementations" should be "The last five" (or "These last") after adding HashLinkListRep and SortedHashRep.
• Suggested fix: Update the count.

L2. kSortedHash option config uses very small bucket_count=16 — db/db_test_util.cc:422

• Issue: With 16 buckets and default 64MB write buffer, each bucket averages ~4M entries at capacity. This is fine for correctness testing but far from realistic performance characteristics.
• Suggested fix: Consider using 256 or 1024 for more representative coverage, though 16 is fine for correctness.

Cross-Component Analysis

Context	Affected?	Safe?	Notes
WritePreparedTxnDB	Yes (CanHandleDuplicatedKey=true)	Yes
ReadOnly/Secondary DB	Not validated (intentional)	Yes	Empty memtable
User-defined timestamps	Rejected	N/A
MemPurge	Creates new memtable with same factory	Yes
Concurrent writers	Per-bucket SpinMutex	Yes
Tailing iterators	Rejected at NewIterator/NewIterators	N/A
max_successive_merges	Uses slow GetDynamicPrefixIterator	Functionally correct	See M3
Range deletions	Separate range_del_table_	Yes
DB Mutex	ValidateOptions called under mutex in SetOptions	O(1) work, no regression

Assumption Stress Test

• "readers are lock-free" — HOLDS. GetEncoded/Contains use only acquire loads. Writer release stores ensure reader sees consistent chain.
• "duplicate inserts return false" — HOLDS. Same key hashes to same bucket; lock serializes check.
• "sorted view built once when immutable" — HOLDS. MarkReadOnly builds view; no subsequent inserts change the version.
• "immutable memory accounting stable" — HOLDS. sorted_entries_bytes_ only updates on version change.

Positive Observations

• Clean thread safety: per-bucket SpinMutex + lock-free readers with proper acquire/release.
• Comprehensive tests: concurrent writes, option validation (Open/CreateCF/SetOptions), tailing rejection, read semantics, duplicate detection, factory serialization, options round-trip.
• Good crash test integration with proper option sanitization.
• Solid header documentation with sizing guidance and limitations.
• Zero behavioral change for existing memtable factories (base ValidateOptions returns OK, IsTailingIteratorSupported defaults true).

---

ℹ️ 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