Summary:
Pull Request resolved: https://github.com/facebook/rocksdb/pull/15394
## Summary
RocksDB's `.clang-tidy` enables several readability checks that existing
code still violates: `readability-isolate-declaration` (476 findings),
`readability-braces-around-statements` (49),
`readability-redundant-string-init` (25), `modernize-use-using` (10),
`misc-definitions-in-headers` (3) and `readability-duplicate-include`
(1). They keep getting flagged whenever nearby lines change. This diff
fixes 553 of the 564; upstream has since fixed the other 11.
What changed:
- Multi-variable declarations such as `std::string a, b;` are split into
one declaration per variable. Pointer and reference declarators are kept
per variable.
- Braces are added around single-statement `if`/`else`/`for`/`while`
bodies, and empty spin loops get an empty `{}` body.
- `std::string s = ""` / `member("")` initializers are removed.
- Remaining `typedef`s in the touched C++ files become `using`. C headers
and `extern "C"` code are untouched.
- `inline` is added to non-template function definitions in test headers.
- A duplicate `#include "util/udt_util.h"` in `db_impl.cc` is removed.
These are clang-tidy's own fix-its, applied mechanically and then
clang-formatted on the changed lines. Three kinds of broken fix-it output
were repaired by hand. Removing a `member("")` initializer left a dangling
comma in constructor initializer lists (`ExternalSstFileInfo`,
`ColumnFamilyMetaData` and others). The function-type typedefs in
`block_test.cc` came out malformed. Empty `while (cond);` spin loops got a
`{;` body. Splitting declarations also exposed two vectors in
`db_etc2_test.cc` to `performance-inefficient-vector-operation`, so a
`reserve()` was added for each.
The lint bot reports every finding in a file a diff touches. This diff
splits declarations in `options/configurable_test.cc`, so it also fixes
that file's 14 older `cert-err58-cpp` and
`cppcoreguidelines-avoid-non-const-global-variables` findings, which come
from seven mutable file-level maps. Six are now `const` and built where
they are used (function-local statics, or locals in their only user).
The unused `imm_struct_option_info` is deleted.
Compatibility: no behavior change. In public headers, the only edits are
removed redundant `""` initializers, split declarations and added braces.
Reviewed By: rban1
Differential Revision: D123186310
fbshipit-source-id: f9c8e88b00e997db0196f8725c7c8d880cf798da
Summary:
Pull Request resolved: https://github.com/facebook/rocksdb/pull/15291
Add experimental coroutine interfaces for implementing asynchronous basic and full `ExternalTableReader`s. Implementations opt in through `CoroExternalTableReaderBase<Mode>` by providing `GetCoroutine()` and may override `MultiGetCoroutine()`; the default implementation runs the individual Get operations concurrently. The external-table adapter uses these APIs when supported and otherwise retains the existing synchronous behavior.
As a smaller supporting fix, Single Get coroutine reads using non-mmap, non-direct I/O on a kFSBuffer-capable `FileSystem` now use `FSRandomAccessFile::SubmitReadAsync()` instead of falling back to synchronous `Read()`.
Reviewed By: xingbowang
Differential Revision: D122187491
fbshipit-source-id: 217e2953756938bbd3f30358a997a7ca3ed5543e
Summary:
Building on D116105558, this change finishes and publishes `format_version = 8` as
production-ready.
The headline feature is a tweaked block encoding that stores the common user-key
prefix shared by all keys in a block **once**, in the block's leading bytes
`[0, restarts[0])`, instead of repeating it at every restart point. This shrinks
prefix-heavy blocks (and their block-cache footprint) and, for bytewise-ordered
keys, speeds up `Seek` by comparing prefix-stripped suffixes. It is applied to
**data blocks** and **every index tier** (leaf/partition index, the
partitioned-index top-level, and the partitioned-filter top-level).
Motivation: MyRocks/ZippyDB/timeseries-style keys carry a long shared prefix
(table/index id, partition, etc.). Repeating it at each restart wastes space and
cache, and every Seek re-compares those bytes. Profiles show index-block seek CPU
is comparable to data-block seek CPU (partitioned index does two index seeks per
data seek), so index blocks are a first-class target, not just data blocks.
### Format (self-describing, no new footer bit)
- The common prefix occupies the block's leading bytes; a non-zero `restarts[0]`
(the offset of the first entry, i.e. the prefix length) self-signals its
presence. `restarts[0]` is 0 in every block that does not use the feature (all
versions, all block types), so a single fv8 reader transparently handles any
mix of stripped and unstripped blocks -- no footer bit is consumed.
- Previous versions would misinterpret this encoding by quietly ignoring the
common prefix, which (along with the lack of extensibility in data-block
metadata) is why it is gated behind format_version 8.
- Only restart-point entries change on disk; non-restart entries are byte
identical to fv7, so the writer produces the exact same bytes regardless of
implementation.
### Public option
New `BlockBasedTableOptions::optimize_key_common_prefix`
(`enum class OptimizeKeyCommonPrefix`), default `kIfFastSeek`:
- `kDisabled` -- never use it.
- `kIfFastSeek` (default) -- use it only where it also enables the Seek speedup:
format_version >= 8 with a built-in (reverse-)bytewise comparator.
- `kEnabled` -- use it for all comparators at fv8; (reverse-)bytewise get space +
fast seek, other comparators get the space savings only (the reader
reconstructs full keys and compares with the real comparator).
Expressed as a criteria so it need not be toggled per CF based on the comparator
(which can vary by CF). It additionally requires `use_delta_encoding` (the
feature builds on delta encoding) and `ts_sz == 0`; when `use_delta_encoding` is
false -- the setting that also preserves zero-copy key pinning -- it is a no-op.
Default format_version is unchanged (still 7); the feature is opt-in via fv8.
### Scope: every index tier
The reader handles all index-like blocks uniformly (a non-zero `restarts[0]`
self-signals a stripped block regardless of block type), so the writer strips the
common prefix from every tier:
- **Data blocks** and **leaf / partition index blocks**: enabled.
- **Partitioned-index top-level block**: enabled. Under the default
`index_shortening = kShortenSeparatorsAndSuccessor` the file's last separator
would otherwise be a short successor sharing no leading bytes, collapsing the
single-block top-level's common prefix to 0. When common-prefix is on the
builder suppresses that one successor (keeps the full last key -- i.e.
`kShortenSeparators` behavior for that one entry), so the top-level strips
uniformly under every shortening mode; the full last key is also a tighter file
upper bound, so it is not worse.
- **Partitioned-filter top-level index**: enabled (same uniform treatment). Its
space/CPU effect is small at default partitioning (the top-level filter index
is tiny) but grows with many small partitions -- a filter miss short-circuits
at, and is dominated by, the top-level filter-index seek (see Performance).
Included for a uniform on-disk surface across tiers.
Bloom filter *contents* still hash full user keys, so prefix/whole-key filtering
is unaffected.
### Implementation
- Writer (`block_builder.{h,cc}`): keys are encoded incrementally in the fv7
layout while the running common prefix is tracked cheaply (reusing the delta
`shared`); at `Finish()` only the restart-point entries are rewritten in place
with the prefix stripped, and the prefix is written once at the block start.
The rewrite is generalized to the value-delta (fv4) index layout used by index
blocks (no `value_size` header; bulk-copy the inline BlockHandle and
non-restart entries verbatim).
- Reader (`block.{h,cc}`, `dbformat.h`): `Block` decodes the prefix from
`restarts[0]`; `BlockIter::ParseNextKey` prepends it on a restart point
(`shared == 0`). The suffix-seek helpers (strip target once, compare shortened
suffixes) live in the templated `BlockIter` base and are shared by data and
index iterators (binary search, templated on `DecodeKey`/`DecodeKeyV4`); for
non-bytewise comparators (data) and the hash `PrefixSeek` refine step, the
reader reconstructs the probed restart key and compares full keys.
- Gating (`block_based_table_builder.cc`, `index_builder.{h,cc}`,
`partitioned_filter_block.{h,cc}`): the criteria is threaded to the data-block
builder and to every index tier -- leaf/partition index, the partitioned-index
top-level (`use_common_prefix_top`), and the partitioned-filter top-level --
together with the last-separator successor suppression
(`ShortenedIndexBuilder::GetSeparatorWithSeq`) that lets the top-level strip
under the default shortening mode. The index base gate also requires
`super_block_alignment_size == 0` (block alignment can trigger
`skip_delta_encoding`, producing non-restart `shared == 0` entries the reader
could not distinguish from stripped restart keys).
- Memory: the per-block/per-iterator prefix is stored as a `uint32_t` length
(the prefix is always at offset 0) rather than a full `Slice`, and `Block` /
`BlockBuilder` members were repacked so the feature adds little or no struct
size.
- Java: a new `OptimizeKeyCommonPrefix` enum and a `BlockBasedTableConfig`
getter/setter, plumbed through JNI in both directions (the
`newTableFactoryHandle` writer and the options-reconstruction constructor).
### Whole-file (table-level) prefix: investigated and rejected
A natural extension is to store the whole-file common prefix once per *file*
(when an `SstPartitioner` guarantees every key in the file shares a fixed prefix)
instead of once per *block* -- e.g. the MyRocks index/table id, or a future
128-bit global id. It was prototyped end-to-end (partitioner-provided prefix
captured at file open, stored in a `user_key_common_prefix` table property, and
stripped from every data/index block; the reader re-prepends it) and then
dropped. The `user_key_common_prefix` table property reserved in the
previous diff is dropped in this one. The rationale, in case it is revisited:
- Marginal benefit over per-block. Per-block already strips the full shared
prefix from every key; the only thing whole-file removes on top is the single
per-block copy of that prefix, i.e. `~= prefix_len / block_size` of the
uncompressed (block-cache) footprint, with no meaningful read-CPU change
(per-block already delivers strip-once + suffix compares). Measured at
prefix_len=12, 4 KB blocks this was ~-0.3% incremental space; at the 8-16 KB
data blocks typical of the space-constrained target (MyRocks/UDB) it is
~0.1-0.2% even with a 16-byte global id -- a fraction of a percent, seen only
by partitioner users. Small block-size workloads are becoming more
niche, and even where they exist, they are also CPU-sensitive (see next
paragraph).
- Real, if small, CPU tax on the *per-block* path. Reconstructing a key from two
non-contiguous parts (file prefix + block residual) cost ~0.5-1.0% additional
retired instructions on the per-block read path (deterministic across gcc and
clang), wall-clock-neutral in cache-resident tests but present even when no
whole-file prefix is configured. Attempts to remove it did not pan out: a
compile-time specialization of the reconstruction *increased* instructions
(+2.9%) by defeating inlining; a per-block combined-prefix buffer trades the
CPU cost for a memory cost on every (default) block; and a per-iterator buffer
was the original larger regression. Only invasive iterator-level
monomorphization would truly zero it, which is not justified on this hot path.
- Decision. A fraction-of-a-percent, partitioner-only space win does not justify
a permanent per-block-read-path CPU tax plus additional on-disk format surface
(a table property that changes whole-file interpretation). Dropped. The generic
`SstPartitioner::ShouldPartitionByPrefix` hook is kept (cheap, and useful
independently), and the reserved `user_key_common_prefix` table property is
removed since no writer will emit it.
### Performance (db_bench, release/native, cache-warm, best-of-3, pinned)
Three arms: (A) fv7 on the base/main branch, (B) fv7 with this change (should be
neutral / byte-identical writing), (C) fv8 with this change and
`optimize_key_common_prefix=enabled` (the optimization is active). Partitioned
index + filters; `compression_type=none`, so SST bytes are the uncompressed
block-cache footprint. `int` = db_bench default keys (variation-early, near worst
case); `pfxN` = an N-byte shared prefix + varying suffix (MyRocks-like).
Setup: `num=3,000,000`, `reads=1,500,000`, single thread,
`metadata_block_size=4096`, cache holds the working set, pinned to one core.
| shape (key/val) | arm | SST bytes | readrandom us/op | seekrandom us/op |
|---|---|---:|---:|---:|
| int (32/8) | A fv7 main | 138,983,658 | 3.484 | 4.995 |
| | B fv7 ours | 138,983,146 | 3.518 | 5.024 |
| | C fv8 ours | 137,681,420 | 3.490 | 4.986 |
| pfx24 (32/8) | A fv7 main | 71,697,531 | 3.417 | 5.561 |
| | B fv7 ours | 71,698,043 | 3.448 | 5.579 |
| | C fv8 ours | 65,578,236 | 3.284 | 5.386 |
| pfx56 (64/16) | A fv7 main | 102,942,266 | 3.624 | 5.830 |
| | B fv7 ours | 102,942,266 | 3.678 | 5.835 |
| | C fv8 ours | 90,554,642 | 3.444 | 5.626 |
fv8 vs fv7-main (C vs A) -- space / readrandom / seekrandom:
- int (32/8): **-0.9%** / ~neutral / ~neutral
- pfx24 (32/8): **-8.5%** / **-3.9%** / **-3.1%**
- pfx56 (64/16): **-12.0%** / **-5.0%** / **-3.5%**
fv7-ours vs fv7-main (B vs A): SST bytes byte-identical (within 512 B; the fv8
paths are gated off), read/seek us/op within run-to-run noise (~1%, best-of-3 on
a shared host). Net: fv8 is a space **and** read/seek-CPU win on prefix-heavy
data and neutral on variation-early (`int`) data, so `kIfFastSeek` is safe on by
default at fv8.
### Space by tier (compression=none, num=3M, mdb=4096; fv7 -> fv8)
Where the win comes from -- the index tiers strip the most:
| shape | data | leaf/partition index | top-level index | filter |
|---|---:|---:|---:|---:|
| int | -0.8% | -29% | -45% | -0.2% |
| pfx24 | -8.3% | -69% | -89% | -0.8% |
| pfx56 | -11.1% | -83% | -96% | -1.6% |
Data bytes dominate the absolute total, but every index tier is stripped:
leaf/partition index -29% to -83% and the partitioned-index top-level -45% to
-96%. Filter *blocks* barely move (-0.2% to -1.6%) because filter bits hash full
user keys; only the small top-level filter index is stripped.
### Point-miss lookups (readmissing)
A partitioned-filter miss short-circuits at the filter (no data-block read), so
it is dominated by the top-level filter-index seek plus the partition bloom. At a
typical `metadata_block_size=4096` (few partitions, tiny top-level filter index)
fv8 is neutral on misses (int +0.9%, pfx24 -1.0%, pfx56 -0.7% us/op -- all
noise). The top-level-filter stripping pays only with many small partitions: at
`metadata_block_size=256` it is pfx24 1.166->1.138 (**-2.4%**) and pfx56
1.273->1.188 (**-6.7%**) us/op.
### Backward / forward compatibility
- Default `format_version` remains 7; the feature is inert unless a CF opts into
fv8. Older RocksDB rejects fv8 files at footer decode (fail-safe).
- Within fv8, the stripped-block set a writer can emit is fixed and every fv8
reader can read it (the `restarts[0]` signal plus uniform reader handling), so
the on-disk surface is self-consistent across builds/downgrade.
### Follow-ups (not in this change)
- Suffix-aware interpolation / hash index seek (currently correct via the
reconstruction fallback, not perf-tuned).
Pull Request resolved: https://github.com/facebook/rocksdb/pull/15117
Test Plan:
- New focused unit tests (`table_test`), run at the full `format_version` matrix
including the now-supported fv8:
`GeneralTableTest.OptimizeKeyCommonPrefix` (data-block space+read+Seek
correctness: existing and missing keys straddling the prefix, before/after-all,
SeekForPrev), `OptimizeKeyCommonPrefixNonBytewise` (reconstruction path), and
`IndexBlockCommonPrefixEquivalence` across `{kBinarySearch,
kBinarySearchWithFirstKey, kHashSearch, kTwoLevelIndexSearch}` x
`{partitioned filters}` x `{bytewise, reverse-bytewise}` (so both top-level
tiers -- partitioned-index and partitioned-filter -- are exercised), asserting
identical reads and a shrunken fv8 index.
- The randomized `ParameterizedHarnessTest` now runs at fv8 (reverse-bytewise,
restart intervals `{1,16,1024}`, special/empty keys fuzzed against the feature).
- `db_crashtest.py` randomizes `format_version` including 8 and
`optimize_key_common_prefix` in {0,1,2}.
- Removed the reserved-property carve-out: dropped the `ReservedUserKeyCommonPrefix`
reader test and the C API getter for the property; C API regenerated via
`tools/c_api_gen/` (`make regen-c-api`), and the intentional public-C-API
removal is recorded in `tools/c_api_gen/api_compatibility_allowlist.json`.
- Java: `BlockBasedTableConfigTest.optimizeKeyCommonPrefix` (default
`kIfFastSeek`, plus set/get of every value).
- `table_test` `FooterTests` updated for the now-published fv8 footer, whose
metaindex-gap encoding stores the metaindex block size in 16 bits (the test
caps that size for fv >= 8).
- Manual verification: the db_bench runs in Performance above (three-arm
space + read/seek CPU, per-tier byte breakdown via `sst_dump`, and
`readmissing`); and `readrandom`/`seekrandom` with
`--block_protection_bytes_per_key=8` find 100% of keys at both fv7 and fv8
(reconstructed keys pass per-key checksum).
Reviewed By: xingbowang
Differential Revision: D116365016
Pulled By: pdillinger
fbshipit-source-id: 66f4b5c60ea4fe0ff48d34c98d191ba8ebdf0665
Summary:
and remove deprecated DB::MaxMemCompactionLevel(). In the process of pushing through a relatively clean refactoring of uses of the old functions, some other minor public APIs are also migrated from raw DB pointers to unique_ptr.
Claude did pretty much all the work, but requiring dozens of prompts to actually push through relatively clean phase out of raw DB pointers from what needed to be touched, and leaving that code in better shape. (Hundreds of `DB*` still remain all over the place even outside C and Java bindings.)
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14335
Test Plan: existing tests; no functional changes intended
Reviewed By: xingbowang, mszeszko-meta
Differential Revision: D93523820
Pulled By: pdillinger
fbshipit-source-id: e4ca22ad81cd2cfe91122d7507d7ca34fe03d043
Summary:
**Context/Summary:**
- Similar to https://github.com/facebook/rocksdb/pull/11288 but for user read such as `Get(), MultiGet(), DBIterator::XXX(), Verify(File)Checksum()`.
- For this, I refactored some user-facing `MultiGet` calls in `TransactionBase` and various types of `DB` so that it does not call a user-facing `Get()` but `GetImpl()` for passing the `ReadOptions::io_activity` check (see PR conversation)
- New user read stats breakdown are guarded by `kExceptDetailedTimers` since measurement shows they have 4-5% regression to the upstream/main.
- Misc
- More refactoring: with https://github.com/facebook/rocksdb/pull/11288, we complete passing `ReadOptions/IOOptions` to FS level. So we can now replace the previously [added](https://github.com/facebook/rocksdb/pull/9424) `rate_limiter_priority` parameter in `RandomAccessFileReader`'s `Read/MultiRead/Prefetch()` with `IOOptions::rate_limiter_priority`
- Also, `ReadAsync()` call time is measured in `SST_READ_MICRO` now
Pull Request resolved: https://github.com/facebook/rocksdb/pull/11444
Test Plan:
- CI fake db crash/stress test
- Microbenchmarking
**Build** `make clean && ROCKSDB_NO_FBCODE=1 DEBUG_LEVEL=0 make -jN db_basic_bench`
- google benchmark version: 604f6fd3f4
- db_basic_bench_base: upstream
- db_basic_bench_pr: db_basic_bench_base + this PR
- asyncread_db_basic_bench_base: upstream + [db basic bench patch for IteratorNext](https://github.com/facebook/rocksdb/compare/main...hx235:rocksdb:micro_bench_async_read)
- asyncread_db_basic_bench_pr: asyncread_db_basic_bench_base + this PR
**Test**
Get
```
TEST_TMPDIR=/dev/shm ./db_basic_bench_{null_stat|base|pr} --benchmark_filter=DBGet/comp_style:0/max_data:134217728/per_key_size:256/enable_statistics:1/negative_query:0/enable_filter:0/mmap:1/threads:1 --benchmark_repetitions=1000
```
Result
```
Coming soon
```
AsyncRead
```
TEST_TMPDIR=/dev/shm ./asyncread_db_basic_bench_{base|pr} --benchmark_filter=IteratorNext/comp_style:0/max_data:134217728/per_key_size:256/enable_statistics:1/async_io:1/include_detailed_timers:0 --benchmark_repetitions=1000 > syncread_db_basic_bench_{base|pr}.out
```
Result
```
Base:
1956,1956,1968,1977,1979,1986,1988,1988,1988,1990,1991,1991,1993,1993,1993,1993,1994,1996,1997,1997,1997,1998,1999,2001,2001,2002,2004,2007,2007,2008,
PR (2.3% regression, due to measuring `SST_READ_MICRO` that wasn't measured before):
1993,2014,2016,2022,2024,2027,2027,2028,2028,2030,2031,2031,2032,2032,2038,2039,2042,2044,2044,2047,2047,2047,2048,2049,2050,2052,2052,2052,2053,2053,
```
Reviewed By: ajkr
Differential Revision: D45918925
Pulled By: hx235
fbshipit-source-id: 58a54560d9ebeb3a59b6d807639692614dad058a
Summary:
Follow a couple best practices:
- Allowed Google benchmark to decide number of iterations. Previously we hardcoded a value, which circumvented benchmark's heuristic for iterating until the result is stable.
- Made each iteration do similar work. Previously, an iteration could do different work depending if the key was found in the first, second, third, or no L0 file.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/11498
Test Plan: none as I am unable to prove it is better
Reviewed By: hx235
Differential Revision: D46339050
Pulled By: ajkr
fbshipit-source-id: fcfc6da4111c5b3ae86d79d908afc5f61f96675b
Summary:
Context:
This is the first PR for WaitForCompact() Implementation with WaitForCompactOptions. In this PR, we are introducing `Status WaitForCompact(const WaitForCompactOptions& wait_for_compact_options)` in the public API. This currently utilizes the existing internal `WaitForCompact()` implementation (with default abort_on_pause = false). `abort_on_pause` has been moved to `WaitForCompactOptions&`. In the later PRs, we will introduce the following two options in `WaitForCompactOptions`
1. `bool flush = false` by default - If true, flush before waiting for compactions to finish. Must be set to true to ensure no immediate compactions (except perhaps periodic compactions) after closing and re-opening the DB.
2. `bool close_db = false` by default - If true, will also close the DB upon compactions finishing.
1. struct `WaitForCompactOptions` added to options.h and `abort_on_pause` in the internal API moved to the option struct.
2. `Status WaitForCompact(const WaitForCompactOptions& wait_for_compact_options)` introduced in `db.h`
3. Changed the internal WaitForCompact() to `WaitForCompact(const WaitForCompactOptions& wait_for_compact_options)` and checks for the `abort_on_pause` inside the option.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/11436
Test Plan:
Following tests added
- `DBCompactionTest::WaitForCompactWaitsOnCompactionToFinish`
- `DBCompactionTest::WaitForCompactAbortOnPauseAborted`
- `DBCompactionTest::WaitForCompactContinueAfterPauseNotAborted`
- `DBCompactionTest::WaitForCompactShutdownWhileWaiting`
- `TransactionTest::WaitForCompactAbortOnPause`
NOTE: `TransactionTest::WaitForCompactAbortOnPause` was added to use `StackableDB` to ensure the wrapper function is in place.
Reviewed By: pdillinger
Differential Revision: D45799659
Pulled By: jaykorean
fbshipit-source-id: b5b58f95957f2ab47d1221dee32a61d6cdc4685b
Summary:
Context:
In pull request https://github.com/facebook/rocksdb/issues/11436, we are introducing a new public API `waitForCompact(const WaitForCompactOptions& wait_for_compact_options)`. This API invokes the internal implementation `waitForCompact(bool wait_unscheduled=false)`. The unscheduled parameter indicates the compactions that are not yet scheduled but are required to process items in the queue.
In certain cases, we are unable to wait for compactions, such as during a shutdown or when background jobs are paused. It is important to return the appropriate status in these scenarios. For all other cases, we should wait for all compaction and flush jobs, including the unscheduled ones. The primary purpose of this new API is to wait until the system has resolved its compaction debt. Currently, the usage of `wait_unscheduled` is limited to test code.
This pull request eliminates the usage of wait_unscheduled. The internal `waitForCompact()` API now waits for unscheduled compactions unless the db is undergoing a shutdown. In the event of a shutdown, the API returns `Status::ShutdownInProgress()`.
Additionally, a new parameter, `abort_on_pause`, has been introduced with a default value of `false`. This parameter addresses the possibility of waiting indefinitely for unscheduled jobs if `PauseBackgroundWork()` was called before `waitForCompact()` is invoked. By setting `abort_on_pause` to `true`, the API will immediately return `Status::Aborted`.
Furthermore, all tests that previously called `waitForCompact(true)` have been fixed.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/11443
Test Plan:
Existing tests that involve a shutdown in progress:
- DBCompactionTest::CompactRangeShutdownWhileDelayed
- DBTestWithParam::PreShutdownMultipleCompaction
- DBTestWithParam::PreShutdownCompactionMiddle
Reviewed By: pdillinger
Differential Revision: D45923426
Pulled By: jaykorean
fbshipit-source-id: 7dc93fe6a6841a7d9d2d72866fa647090dba8eae
Summary:
**Context:**
The existing stat rocksdb.sst.read.micros does not reflect each of compaction and flush cases but aggregate them, which is not so helpful for us to understand IO read behavior of each of them.
**Summary**
- Update `StopWatch` and `RandomAccessFileReader` to record `rocksdb.sst.read.micros` and `rocksdb.file.{flush/compaction}.read.micros`
- Fixed the default histogram in `RandomAccessFileReader`
- New field `ReadOptions/IOOptions::io_activity`; Pass `ReadOptions` through paths under db open, flush and compaction to where we can prepare `IOOptions` and pass it to `RandomAccessFileReader`
- Use `thread_status_util` for assertion in `DbStressFSWrapper` for continuous testing on we are passing correct `io_activity` under db open, flush and compaction
Pull Request resolved: https://github.com/facebook/rocksdb/pull/11288
Test Plan:
- **Stress test**
- **Db bench 1: rocksdb.sst.read.micros COUNT ≈ sum of rocksdb.file.read.flush.micros's and rocksdb.file.read.compaction.micros's.** (without blob)
- May not be exactly the same due to `HistogramStat::Add` only guarantees atomic not accuracy across threads.
```
./db_bench -db=/dev/shm/testdb/ -statistics=true -benchmarks="fillseq" -key_size=32 -value_size=512 -num=50000 -write_buffer_size=655 -target_file_size_base=655 -disable_auto_compactions=false -compression_type=none -bloom_bits=3 (-use_plain_table=1 -prefix_size=10)
```
```
// BlockBasedTable
rocksdb.sst.read.micros P50 : 2.009374 P95 : 4.968548 P99 : 8.110362 P100 : 43.000000 COUNT : 40456 SUM : 114805
rocksdb.file.read.flush.micros P50 : 1.871841 P95 : 3.872407 P99 : 5.540541 P100 : 43.000000 COUNT : 2250 SUM : 6116
rocksdb.file.read.compaction.micros P50 : 2.023109 P95 : 5.029149 P99 : 8.196910 P100 : 26.000000 COUNT : 38206 SUM : 108689
// PlainTable
Does not apply
```
- **Db bench 2: performance**
**Read**
SETUP: db with 900 files
```
./db_bench -db=/dev/shm/testdb/ -benchmarks="fillseq" -key_size=32 -value_size=512 -num=50000 -write_buffer_size=655 -disable_auto_compactions=true -target_file_size_base=655 -compression_type=none
```run till convergence
```
./db_bench -seed=1678564177044286 -use_existing_db=true -db=/dev/shm/testdb -benchmarks=readrandom[-X60] -statistics=true -num=1000000 -disable_auto_compactions=true -compression_type=none -bloom_bits=3
```
Pre-change
`readrandom [AVG 60 runs] : 21568 (± 248) ops/sec`
Post-change (no regression, -0.3%)
`readrandom [AVG 60 runs] : 21486 (± 236) ops/sec`
**Compaction/Flush**run till convergence
```
./db_bench -db=/dev/shm/testdb2/ -seed=1678564177044286 -benchmarks="fillseq[-X60]" -key_size=32 -value_size=512 -num=50000 -write_buffer_size=655 -disable_auto_compactions=false -target_file_size_base=655 -compression_type=none
rocksdb.sst.read.micros COUNT : 33820
rocksdb.sst.read.flush.micros COUNT : 1800
rocksdb.sst.read.compaction.micros COUNT : 32020
```
Pre-change
`fillseq [AVG 46 runs] : 1391 (± 214) ops/sec; 0.7 (± 0.1) MB/sec`
Post-change (no regression, ~-0.4%)
`fillseq [AVG 46 runs] : 1385 (± 216) ops/sec; 0.7 (± 0.1) MB/sec`
Reviewed By: ajkr
Differential Revision: D44007011
Pulled By: hx235
fbshipit-source-id: a54c89e4846dfc9a135389edf3f3eedfea257132
Summary:
This PR avoids allocations and copies for the result of `GetMergeOperands()` when the average operand size is at least 256 bytes and the total operands size is at least 32KB. The `GetMergeOperands()` already included `PinnableSlice` but was calling `PinSelf()` (i.e., allocating and copying) for each operand. When this optimization takes effect, we instead call `PinSlice()` to skip that allocation and copy. Resources are pinned in order for the `PinnableSlice` to point to valid memory even after `GetMergeOperands()` returns.
The pinned resources include a referenced `SuperVersion`, a `MergingContext`, and a `PinnedIteratorsManager`. They are bundled into a `GetMergeOperandsState`. We use `SharedCleanablePtr` to share that bundle among all `PinnableSlice`s populated by `GetMergeOperands()`. That way, the last `PinnableSlice` to be `Reset()` will cleanup the bundle, including unreferencing the `SuperVersion`.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/10458
Test Plan:
- new DB level test
- measured benefit/regression in a number of memtable scenarios
Setup command:
```
$ ./db_bench -benchmarks=mergerandom -merge_operator=StringAppendOperator -num=$num -writes=16384 -key_size=16 -value_size=$value_sz -compression_type=none -write_buffer_size=1048576000
```
Benchmark command:
```
./db_bench -threads=$threads -use_existing_db=true -avoid_flush_during_recovery=true -write_buffer_size=1048576000 -benchmarks=readrandomoperands -merge_operator=StringAppendOperator -num=$num -duration=10
```
Worst regression is when a key has many tiny operands:
- Parameters: num=1 (implying 16384 operands per key), value_sz=8, threads=1
- `GetMergeOperands()` latency increases 682 micros -> 800 micros (+17%)
The regression disappears into the noise (<1% difference) if we remove the `Reset()` loop and the size counting loop. The former is arguably needed regardless of this PR as the convention in `Get()` and `MultiGet()` is to `Reset()` the input `PinnableSlice`s at the start. The latter could be optimized to count the size as we accumulate operands rather than after the fact.
Best improvement is when a key has large operands and high concurrency:
- Parameters: num=4 (implying 4096 operands per key), value_sz=2KB, threads=32
- `GetMergeOperands()` latency decreases 11492 micros -> 437 micros (-96%).
Reviewed By: cbi42
Differential Revision: D38336578
Pulled By: ajkr
fbshipit-source-id: 48146d127e04cb7f2d4d2939a2b9dff3aba18258
Summary:
Ribbon micro-bench needs updating after re-numbering
`BloomLikeFilterPolicy::GetAllFixedImpls()` entries. (CircleCI nightly
failure.)
Also fixed memory leaks while using ASAN to validate my fix. (I assume
the leaks weren't intentional for some performance characteristic.)
Pull Request resolved: https://github.com/facebook/rocksdb/pull/10195
Test Plan: run with ASAN
Reviewed By: jay-zhuang
Differential Revision: D37244459
Pulled By: pdillinger
fbshipit-source-id: 5a363e10de3c4c9c88099c937e3dc3b4cf24fd30
Summary:
Iterator is not freed after test is done (after the main for
loop), which could cause db close failed.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/10058
Test Plan:
Able to reproduce consistently with higher thread number,
like 100, make sure it passes after the fix
Reviewed By: ajkr
Differential Revision: D36685823
Pulled By: jay-zhuang
fbshipit-source-id: 4c98b8758d106bfe40cae670e689c3d284765bcf
Summary:
The new microbenchmarks, DBGetMergeOperandsInMemtable and DBGetMergeOperandsInSstFile, correspond to the two different LSMs tested: all data in one memtable and all data in one SST file, respectively. Both cases are parameterized by thread count (1 or 8) and merge operands per key (1, 32, or 1024). The SST file case is additionally parameterized by whether data is in block cache or mmap'd memory.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/9971
Test Plan:
```
$ TEST_TMPDIR=/dev/shm/db_basic_bench/ ./db_basic_bench --benchmark_filter=DBGetMergeOperands
The number of inputs is very large. DBGet will be repeated at least 192 times.
The number of inputs is very large. DBGet will be repeated at least 192 times.
2022-05-09T13:15:40-07:00
Running ./db_basic_bench
Run on (36 X 2570.91 MHz CPU s)
CPU Caches:
L1 Data 32 KiB (x18)
L1 Instruction 32 KiB (x18)
L2 Unified 1024 KiB (x18)
L3 Unified 25344 KiB (x1)
Load Average: 4.50, 4.33, 4.37
----------------------------------------------------------------------------------------------------------------------------
Benchmark Time CPU Iterations UserCounters...
----------------------------------------------------------------------------------------------------------------------------
DBGetMergeOperandsInMemtable/entries_per_key:1/threads:1 846 ns 846 ns 849893 db_size=0
DBGetMergeOperandsInMemtable/entries_per_key:32/threads:1 2436 ns 2436 ns 305779 db_size=0
DBGetMergeOperandsInMemtable/entries_per_key:1024/threads:1 77226 ns 77224 ns 8152 db_size=0
DBGetMergeOperandsInMemtable/entries_per_key:1/threads:8 116 ns 929 ns 779368 db_size=0
DBGetMergeOperandsInMemtable/entries_per_key:32/threads:8 330 ns 2644 ns 280824 db_size=0
DBGetMergeOperandsInMemtable/entries_per_key:1024/threads:8 12466 ns 99718 ns 7200 db_size=0
DBGetMergeOperandsInSstFile/entries_per_key:1/mmap:0/threads:1 1640 ns 1640 ns 461262 db_size=21.7826M
DBGetMergeOperandsInSstFile/entries_per_key:1/mmap:1/threads:1 1693 ns 1693 ns 439936 db_size=21.7826M
DBGetMergeOperandsInSstFile/entries_per_key:32/mmap:0/threads:1 3999 ns 3999 ns 172881 db_size=19.6981M
DBGetMergeOperandsInSstFile/entries_per_key:32/mmap:1/threads:1 5544 ns 5543 ns 135657 db_size=19.6981M
DBGetMergeOperandsInSstFile/entries_per_key:1024/mmap:0/threads:1 78767 ns 78761 ns 8395 db_size=19.6389M
DBGetMergeOperandsInSstFile/entries_per_key:1024/mmap:1/threads:1 157242 ns 157238 ns 4495 db_size=19.6389M
DBGetMergeOperandsInSstFile/entries_per_key:1/mmap:0/threads:8 231 ns 1848 ns 347768 db_size=21.7826M
DBGetMergeOperandsInSstFile/entries_per_key:1/mmap:1/threads:8 214 ns 1715 ns 393312 db_size=21.7826M
DBGetMergeOperandsInSstFile/entries_per_key:32/mmap:0/threads:8 596 ns 4767 ns 142088 db_size=19.6981M
DBGetMergeOperandsInSstFile/entries_per_key:32/mmap:1/threads:8 720 ns 5757 ns 118200 db_size=19.6981M
DBGetMergeOperandsInSstFile/entries_per_key:1024/mmap:0/threads:8 11613 ns 92460 ns 7344 db_size=19.6389M
DBGetMergeOperandsInSstFile/entries_per_key:1024/mmap:1/threads:8 19989 ns 159908 ns 4440 db_size=19.6389M
```
Reviewed By: jay-zhuang
Differential Revision: D36258861
Pulled By: ajkr
fbshipit-source-id: 04b733e1cc3a4a70ed9baa894c50fdf96c0d6064
Summary:
ToString() is created as some platform doesn't support std::to_string(). However, we've already used std::to_string() by mistake for 16 months (in db/db_info_dumper.cc). This commit just remove ToString().
Pull Request resolved: https://github.com/facebook/rocksdb/pull/9955
Test Plan: Watch CI tests
Reviewed By: riversand963
Differential Revision: D36176799
fbshipit-source-id: bdb6dcd0e3a3ab96a1ac810f5d0188f684064471
Summary:
Use `unique_ptr<DB>` to make sure the DB object is deleted. Previously it was not, which led to accumulating file descriptors for deleted directories because a `DBImpl::db_dir_` from each test remained alive.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/9939
Test Plan: run `lsof -p $(pidof db_basic_bench)` while benchmark runs; verify no FDs for deleted directories.
Reviewed By: jay-zhuang
Differential Revision: D36108761
Pulled By: ajkr
fbshipit-source-id: cfe02646b038a445af7d5db8989eb1f40d658359
Summary:
I tried evaluating https://github.com/facebook/rocksdb/issues/9611 using DBGet microbenchmarks but mostly found the change is well within the noise even for hundreds of repetitions; meanwhile, the InternalKeyComparator CPU it saves is 1-2% according to perf so it should be measurable. In this PR I tried adding a mmap mode that will bypass compression/checksum/block cache/file read to focus more on the block lookup paths, and also increased the Get() count.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/9903
Reviewed By: jay-zhuang, riversand963
Differential Revision: D35907375
Pulled By: ajkr
fbshipit-source-id: 69490d5040ef0863e1ce296724104d0aa7667215
Summary:
This commit was generated using `mgt import`.
pristine code for third-party libraries:
third-party/benchmark
upgrade google benchmark to v1.6.1
contains a local patch that reverts [this](https://github.com/google/benchmark/pull/1227?fbclid=IwAR2CCmIJmjU62SPPQQf_t8kdAsMjYv_Pa_GxabYUOdQpGPZUHKwbnYS_1oE) and changs `enum Flags` to be `enum Flags : uint32_t`.
Reviewed By: chadaustin
Differential Revision: D35136540
fbshipit-source-id: f3662f953cd87956e5e9b767e55e3697f99d3b49
Summary:
DBGet p95 and p99 have high variation, remove them for now.
Also increase the iteration to 3 to avoid false positive.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/9742
Test Plan: Internal CI
Reviewed By: ajkr
Differential Revision: D35082820
Pulled By: jay-zhuang
fbshipit-source-id: facc1d56b94e54aa8c8852c207aae2ae4e4924b0
Summary:
Some changes to make it easier to make FilterPolicy
customizable. Especially, create distinct classes for the different
testing-only and user-facing built-in FilterPolicy modes.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/9567
Test Plan:
tests updated, with no intended difference in functionality
tested. No difference in test performance seen as a result of moving to
string-based filter type configuration.
Reviewed By: mrambacher
Differential Revision: D34234694
Pulled By: pdillinger
fbshipit-source-id: 8a94931a9e04c3bcca863a4f524cfd064aaf0122
Summary:
* Add more micro-benchmark tests
* Expose an API in DBImpl for waiting for compactions (still not visible to the user)
* Add argument name for ribbon_bench
* remove benchmark run from CI, as it runs too long.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/9436
Test Plan: CI
Reviewed By: riversand963
Differential Revision: D33777836
Pulled By: jay-zhuang
fbshipit-source-id: c05de3bc082cc05b5d019f00b324e774bf4bbd96
Summary:
Directory fsync might be expensive on btrfs and it may not be needed.
Here are 4 directory fsync cases:
1. creating a new file: dir-fsync is not needed on btrfs, as long as the
new file itself is synced.
2. renaming a file: dir-fsync is not needed if the renamed file is
synced. So an API `FsyncAfterFileRename(filename, ...)` is provided
to sync the file on btrfs. By default, it just calls dir-fsync.
3. deleting files: dir-fsync is forced by set
`IOOptions.force_dir_fsync = true`
4. renaming multiple files (like backup and checkpoint): dir-fsync is
forced, the same as above.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/8903
Test Plan: run tests on btrfs and non btrfs
Reviewed By: ajkr
Differential Revision: D30885059
Pulled By: jay-zhuang
fbshipit-source-id: dd2730b31580b0bcaedffc318a762d7dbf25de4a
Summary:
Add google benchmark for microbench.
Add ribbon_bench for benchmark ribbon filter vs. other filters.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/8493
Test Plan:
added test to CI
To run the benchmark on devhost:
Install benchmark: `$ sudo dnf install google-benchmark-devel`
Build and run:
`$ ROCKSDB_NO_FBCODE=1 DEBUG_LEVEL=0 make microbench`
or with cmake:
`$ mkdir build && cd build && cmake .. -DCMAKE_BUILD_TYPE=Release -DWITH_BENCHMARK=1 && make microbench`
Reviewed By: pdillinger
Differential Revision: D29589649
Pulled By: jay-zhuang
fbshipit-source-id: 8fed13b562bef4472f161ecacec1ab6b18911dff
