mirror of
https://github.com/facebook/rocksdb.git
synced 2026-10-23 08:09:02 +03:00
main
30 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Josh Kang
|
014e1c43c1 |
Apply readability fixes (isolate decls, braces) (#15394)
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 |
||
|
Josh Kang
|
da82001d81 |
Mark overriding methods and destructors override (#15366)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15366 ## Summary RocksDB's `.clang-tidy` enables `modernize-use-override`, and clang reports `-Winconsistent-missing-destructor-override` when a class marks its other overrides `override` but not its destructor. A full scan finds 190 `modernize-use-override`, 100 `inconsistent-missing-destructor-override` and 2 `suggest-override` findings in existing code. Each one comes back whenever a nearby line is edited. They also leave classes where it isn't obvious which methods are overrides and which introduce new virtuals. This diff marks every overriding member function and destructor `override` and drops the redundant `virtual` on them. It also changes `final override` to plain `final`, following the check's default. Methods that introduce a new virtual keep `virtual`. Most edits are clang-tidy's own fix-its, applied mechanically and then clang-formatted. Compatibility: no signatures change. In public headers the only effect is `override` on existing overrides (mostly destructors, plus `Cache::Name()` and `Cache::GetPrintableOptions()`, whose base declarations are unconditional), so user code that subclasses these types is unaffected. Every touched class now marks all of its overrides consistently. This matters because a partially-marked class would make clang emit `-Winconsistent-missing-override` in user builds. Reviewed By: rban1 Differential Revision: D123186308 fbshipit-source-id: 1f489ea32d938e3ec0faf9abc4f7ff49e042d37a |
||
|
Josh Kang
|
928527b869 |
Fix bugs and false positives from clang-analyzer/bugprone (#15325)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15325 ## Summary RocksDB's `.clang-tidy` enables the `clang-analyzer-*` and `bugprone-*` checks. A full scan of the repo still reports 78 findings from these checks in existing code. Each time a nearby line is edited, they come back as noise, and a few of them are real defects. This diff resolves 73 of the 78; upstream has since fixed one more, and the four left are listed under "Intentionally not changed". Some fixes repair real bugs. The rest make the code's intent explicit to the analyzer without changing behavior. Real bugs fixed: - `port/mmap.cc`: `MemMapping::operator=(MemMapping&&)` ran the destructor on `*this`, `memcpy`'d over a non-trivially-copyable object, then placement-new'd the source. That is undefined behavior, and `*this` can be the base subobject of a `TypedMemMapping`. It now moves the members one by one, and the previous mapping is released through a local `MemMapping`. - `port/mmap.h`: `TypedMemMapping::operator=(MemMapping&&)` had no `return *this;`, so calling it was undefined behavior. - `cache/typed_cache.h`: `if constexpr (sizeof(TCreateContext) > 0)` is always true, so the branch meant for empty create contexts (for example `BlobContentsCreator`) could never run. The check is now `!std::is_empty_v<TCreateContext>`. Existing instantiations still call the same `Create` function. - `cassandra_row_merge_test.cc`: the test reused a vector after `std::move`-ing it into `RowValue::Merge(std::vector<RowValue>&&)`. That only worked because `Merge` happens not to consume its input. The test now rebuilds the input. Behavior-preserving changes: - Virtual calls in constructors and destructors are now explicitly qualified (for example `DBImpl::CloseImpl()` in `~DBImpl`). During construction and destruction, dispatch already resolved to these functions. - Stale `/*name=*/` argument comments now match the parameter names. - `unique_ptr::release()` after a successful cache `Insert` (where the cache takes ownership) uses `[[maybe_unused]] T* released = x.release();`, the pattern already used in `table_cache.cc`. - Removed `class Mutex;` and `class StaticMeta;` forward declarations that sat in the wrong namespace. - Added default member initializers for the `TablePropertiesCollectorFactory::Context` fields that its default constructor left uninitialized. - Smaller cleanups: a redundant `cond ? 3 : 3` in a test, `sizeof(std::string)` in a memory estimate, and an explicit `Cleanable` base move via `up_cast`. Tests that intentionally use moved-from objects now carry `NOLINT(bugprone-use-after-move)`, matching the existing convention. Intentionally not changed: enum casts in tests that check invalid-value handling; three `clang-analyzer-core.NullDereference` reports the analyzer can't prove safe (the C API timestamp out-parameters in `db/c.cc`, which are only written when the caller asked for a timestamp, and the waiter-list walk in `util/async_file_reader.cc`); bit-flag enums in public headers (`FileOpenContract`, `OptionTypeFlags`), where OR-combined values are valid; ownership transfers the analyzer can't see across translation units (`partitioned_index_reader.cc`, `fs_posix.cc`); and the intentional two-separator check in `file/filename.cc`. Reviewed By: rban1 Differential Revision: D123186307 fbshipit-source-id: 623e871286b1c73263b1309cb194692194199e0f |
||
Ranjan Banerjee
|
576dc39899 |
Fix destination type check in lossless_cast for pointers (#15188)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15188 In the pointer branch of `lossless_cast`, the destination type check tested `std::is_enum_v<To>` instead of `std::is_enum_v<ToDeref>`. Since `To` is a pointer type in that branch, `std::is_enum_v<To>` is always false, so the check collapsed to `std::is_integral_v<ToDeref>` and rejected casting to a pointer-to-enum, e.g. `lossless_cast<MyEnum8*>(&some_uint8)`. Reviewed By: xingbowang Differential Revision: D119422904 fbshipit-source-id: b1f4e205d536ba3d10563fa62adb04867fae02ac |
||
Peter Dillinger
|
021374c62c |
Start format_version=8 development toward new features (#15104)
Summary: format_version background: SST files are shared across RocksDB versions. format_version is a critical tool in ensuring prior (and later) versioned readers do not misinterpret a file created by some version. Opt-in features can sometimes rely on previously-rejected corners of the schema space, but not always. This change: lands much of the format_version=8 (fv8) format surface as an internal-only draft -- readable and testable but not user-writable -- so upcoming features can build on a stable base. fv8 is NOT published: kLatestBbtFormatVersion stays 7, and reading or writing fv8 is gated behind the existing TEST-only TEST_AllowUnsupportedFormatVersion() hook. Publishing fv8 (raising the ceiling) is deferred to a later change. The pieces of fv8 added here, and what each unlocks: - Index value-delta escape. Existing behavior: index blocks (format_version >= 4) store each block handle as a delta against the previous entry's handle, and signal "this value is a full handle instead" by reusing the key sentinel `shared == 0`. Because the two are coupled, an index entry whose handle is not contiguous with the previous one must be forced to a restart, costing a full key as well as a full value. fv8 decouples them with an in-value escape codepoint: varint 0 means "a full BlockHandle follows," and genuine size deltas are remapped to zigzag(delta)+1 so that 0 stays free. A non-contiguous entry can then remain an ordinary key-delta-encoded, on-cadence entry. The escape applies to every index-shaped block, which includes the top-level index of index partitions and the index of filter partitions, not just the index over data blocks. Unlocks: the upcoming optimize_key_common_prefix index feature (removes its need to special-case super_block_alignment) and value-delta encoding for embedded-blob SSTs (previously disabled outright because interleaved blob records break handle contiguity). Guarded strictly on fv8; format_version <= 7 encoding and decoding are byte-for-byte unchanged. Note that restart points must not delta-encode, so they still have full values without a sentinel. - Data block footer bit 30 is reserved as an "extended metadata present" escape. Decode rejects a set bit as unsupported/corrupt at all versions. Unlocks: adding block-level metadata in the future through the one shared footer decoder, without another format_version bump or plumbing format_version into every block parser. (Older reader versions relied on this bit being zero for some integrity/consistency checking; format_version bump now makes it safe to use.) - Footer metaindex gap. For fv8, the top 16 bits of the footer's 4-byte metaindex-size field record the gap (in bytes) between the end of the metaindex block and the start of the footer; the low 16 bits hold the metaindex block size. Unlocks: a space very near the end of the file to save full file checksum state, for a "files carry their own checksums" feature that doesn't require SST parsing to extract the checksum states (to run them to completion on the trivial file tail). - Reserved "user_key_common_prefix" table property. The reader returns NotSupported when it is present and non-empty. Unlocks: a future whole-file optimization that stores the common prefix of all user keys once instead of in every restart key; until implemented, fv8 readers refuse such a file rather than mis-read it. - Because of the problem of inferring such a prefix at the start of constructing an SST file, we expect it will rely on SstPartitioner. Here we've added SstPartitioner::ShouldPartitionByPrefix() (not yet used) that will enable this optimization, and avoid the overhead of a virtual callback to SstPartitioner on every key written to an SST file. Supporting cleanups: the Footer's "unset" format_version sentinel is named (kInvalidFormatVersion) and the FormatVersionUses*() predicates assert against it, as each is a `version >= N` test that an unpopulated Footer would silently satisfy. Index block reader entry points (Block::NewIndexIterator() and friends) drop their defaulted trailing arguments so each caller states which codec it expects, and BlockBuilder::Add() takes delta_value by const reference rather than a pointer whose null case no caller used. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15104 Test Plan: New unit tests, all exercising fv8 through TEST_AllowUnsupportedFormatVersion(): - block_test: value-delta escape codec round-trip (delta 0, negative, large, and the escape; asserts format_version <= 7 bytes match the legacy encoding and a legacy 0x00 still decodes as delta 0), and rejection of footer bit 30 at all versions. - db_flush_test: fv7-vs-fv8 read equivalence for super_block_alignment, including the separate_key_value_in_data_block intersection, confirming padding events (hence escape entries) occur and fv8 index values are delta encoded. - sst_file_reader_test: fv7-vs-fv8 read equivalence for embedded-blob SSTs, confirming fv8 (unlike fv7) delta-encodes their index values and actually emits escape entries (counted through a sync point). - block_based_table_reader_test: a builder+reader test that injects a footer metaindex gap via a sync point (verifies the gap bytes are written and the reader locates the metaindex via the recorded gap, and that too large a gap surfaces as a builder error), and that a non-empty reserved user_key_common_prefix property makes the reader return NotSupported. - table_test: two-level index with partitioned filters and index_block_restart_interval > 1, the configuration in which both top-level indexes delta-encode their values; verifies scan and point lookup round trips at each tested format_version including fv8. - (Not strictly fv8) Basic SstPartitionerFixedPrefix tests in slice_test (ShouldPartition, ShouldPartitionByPrefix, their mutual consistency, and CanDoTrivialMove), which had no non-DB coverage before. fv8 is also added to the table_test harness (GenerateArgList) and the shared footer version set (test::kFooterFormatVersionsToTest), so the existing table-level and DB-level property/checksum suites now also run at fv8. And added to crash test (minor revisions to db_stress). ## Performance details Release db_bench (PORTABLE=0, -march=native) A/B/C on the read/write fast path, comparing (a) parent, (b) this commit @ fv7, (c) this commit @ fv8. All DBs on tmpfs, 20M keys, block_size=2048, compression=none. Note index_block_restart_ interval=16 (not the default 1) is required to actually exercise the fv7<->fv8 index value-delta codec -- with restart interval 1 every index entry is a full- handle restart and the changed path never runs (confirmed delta-value=1 via sst_dump). Reads single-thread + core-pinned, warm cache, interleaved/rotated rounds, paired deltas. Reads (readrandom + seekrandom, non-partitioned single ~1.9MB index block and partitioned index+filters): fv7->fv8 in the *same* binary is within noise on all four workloads (largest -1.0%, CIs cross zero). before->after (fv7 path) is also within noise; perf shows a deterministic +0.4-0.6% instructions/op from the added branches (the format_version>=8 value_delta_escape guard + refactored data-block footer decode), which is below the throughput noise floor. Writes (fillseq + compact total CPU): fv7->fv8 null. compact (which rewrites all index blocks) is null before->after. fillseq showed +3.5% CPU before->after, but perf attributes it entirely to cycles/IPC with *identical* instructions/op (+0.01-0.04%) -- a code-layout effect on the untouched memtable-insert loop, not this change. Sanity: any single throughput/CPU delta <~1% is within the build-to-build noise floor, measured directly by comparing two byte-identical binaries (swings up to ~0.9% reads; the pre-native/PORTABLE=1 build showed a spurious -1.85% that vanished under -march=native). Net: no measurable regression from fv8 draft; fv7 encoding/decoding byte-for-byte unchanged, only the noted sub-1% instruction bump on the shared read path. Reviewed By: xingbowang Differential Revision: D116105558 Pulled By: pdillinger fbshipit-source-id: ead776cdd396d497d47872c0956ea8c651c0d13f |
||
Xingbo Wang
|
14a7646fb5 |
Fix PinnableSlice move construction from pinned slices (#14998)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14998 T280925646 reported an ASAN double-free in db_stress' batched prefix scan. The visible free was in VerifyWideColumns(), but WideColumn values are non-owning Slice views, so that destructor should only free the vector element storage. The stack points to prior ownership corruption rather than an intentional verifier free. The concrete bug is in PinnableSlice's move constructor: it delegates to move assignment without first initializing the target object. When moving from a pinned slice, move assignment does not set buf_ because the target is still pinned. If that moved-to slice is later Reset() and reused with PinSelf(), it writes through an uninitialized buf_ pointer. This became reachable after D112212266 changed PinnableWideColumns to keep backing buffers in std::forward_list<PinnableSlice> and emplace moved PinnableSlice values into backing nodes. Fix the move constructor by delegating to the default constructor first, so buf_ is initialized before move assignment. Add a regression that poisons placement-new storage, move-constructs from a pinned slice, resets it, and reuses it with PinSelf(); this crashed before the fix. Reviewed By: pdillinger Differential Revision: D113480016 fbshipit-source-id: 38ce9030f392ff9f0438d38d53d19e356e2d2fbf |
||
|
Peter Dillinger
|
387cb4aae7 |
Clarify/rename atomic wrapper stuff + blog post (#14213)
Summary: * Some existing commentary and motivation around my atomic wrappers in atomic.h was based on a misreading of documentation. seq_cst *is* a safe substitute for acq_rel in all cases. I still like having a distinct type for RelaxedAtomic (as folly does) and a wrapper also for other cases to avoid readability traps like implicit conversion and implicit memory order. This PR is only comment changes and renaming. * Create a blog post about bit fields API to help with lock-free (and low-lock) programming. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14213 Test Plan: esiting tests Reviewed By: xingbowang Differential Revision: D89971581 Pulled By: pdillinger fbshipit-source-id: 9bd1181d692258d668189c2da8bd0e5d98fd6230 |
||
|
Peter Dillinger
|
1cc1df8dab |
Finish migrating HCC to BitFields API (#14154)
Summary: This change builds on https://github.com/facebook/rocksdb/issues/14027 and https://github.com/facebook/rocksdb/issues/13965 to complete migration of the HyperClockCache implementation to using the hygienic BitFields API. No semantic change in the implementation details is intended, just greatly improving readability and safety of the code while maintaining the same performance. In more detail, * Refactor the main metadata atomic for each slot in an HCC table into SlotMeta using BitFields. * Extended BitFields APIs with some additional features, and renamed BlahTransform classes to BlahTransformer to resolve potential naming conflicts with member functions to create them. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14154 Test Plan: for correctness, mostly existing tests. but also added tests for new BitFields features. I especially ran local TSAN whitebox crash test extensively which caught a couple of refactoring errors. For performance, I verified with release builds of cache_bench, using default options, that there was no noticeable/consistent difference after all these HCC migrations vs. backing them out. That test was with GCC 11 and -O2, which is a reasonable baseline for expected compiler optimizations. Reviewed By: xingbowang Differential Revision: D87960540 Pulled By: pdillinger fbshipit-source-id: e0257b7fea8a5c7709daef18911959201ce4e0f3 |
||
|
Peter Dillinger
|
4951494a27 |
Continue migration of HCC impl to BitFields (#14027)
Summary: Continuing work from https://github.com/facebook/rocksdb/issues/13965. Here I'm migrating the "next with shift" kind of bit field and for that I've added an API for atomic additive transformations that can be combined into a single atomic update for multiple fields. (I implemented more features than needed, just in case they are needed someday and to demonstrate what is possible.) Pull Request resolved: https://github.com/facebook/rocksdb/pull/14027 Test Plan: BitFields unit test updated/added, existing HCC tests Reviewed By: xingbowang Differential Revision: D83895094 Pulled By: pdillinger fbshipit-source-id: e4487f34f5607b20f94b85a645ca654e6401e35d |
||
|
Peter Dillinger
|
f9f408f536 |
Start migration of HCC implementation to BitFields (#13965)
Summary: Start the process of migrating the HCC implementation over to my new system of "bit field atomics" to clean up the code. Here I took on the simplest of the three "bit field atomic" formats in HCC, but ended up moving some things around to end up with less plumbing of definitions and values overall. In the process, updated BitFields to use the CRTP pattern to simplify some things (see updated example, etc.) https://en.wikipedia.org/wiki/Curiously_recurring_template_pattern Pull Request resolved: https://github.com/facebook/rocksdb/pull/13965 Test Plan: existing tests. ClockCacheTest.ClockEvictionEffortCapTest caught a regression during my development, and the crash test has a history of finding subtle HCC bugs. Reviewed By: xingbowang Differential Revision: D82669582 Pulled By: pdillinger fbshipit-source-id: b73dd47361cbe9fbd334413dd4ce01b3c667159e |
||
|
Peter Dillinger
|
29d9798ae8 |
Revamp of parallel compression (#13910)
Summary: Complete redo of parallel compression in block_based_table_builder.cc to greatly reduce cross-thread hand-off and blocking. A ring buffer of blocks-in-progress is used to essentially bound working memory while enabling high throughput. Unlike before, all threads can participate in compression work, for a kind of work-stealing algorithm that reduces the need for threads to block. This builds on improvements in https://github.com/facebook/rocksdb/pull/13850 Previously, there was either * parallel_threads==1, the *emit thread* (caller from flush/compaction) doing all the work * parallel_threads > 1, the emit thread generates uncompressed blocks, `parallel_threads` worker threads compress blocks, and a writer thread writes to the SST file. Total of `parallel_threads + 2` threads participating. (Other bookkeeping in emit and write steps omitted from description for simplicity.) Now we have either * parallel_threads==1 (same), the emit thread doing all the work * parallel_threads > 1, the emit thread generates uncompressed blocks and can take up compression work when the ring buffer is full; `parallel_threads` worker threads have as their top priority to write compressed blocks to the SST file but also take up compression work in priority order of next-to-write. Total of `parallel_threads + 1` threads participating. In some cases, this could result in less throughput than before, but arguably the previous implementation was using more threads than explicitly allowed. ## Future/alternate considerations Although we could likely have used some framework for micro-work sharing across threads, that could be difficult with the asymmetry of work loads and thread affinity. Specifically, (a) it would be quite challenging to allow emit work in other threads, because it happens in the caller of BlockBasedTableBuilder, (b) async programming is unlikely to pay off until we have an async interface for writing SST files, and (c) this implementation will nevertheless serve as a benchmark for what we lose or gain in such a framework vs. a hand-tuned system. This implementation still creates and destroys threads for each SST file created. We hope in the future to have more governance and/or pooling of worker threads across various flushes and compactions, but that is not available currently and would require significant design and implementation work. ## More details * This implementation makes use of semaphores for idling and re-waking threads. `std::counting_semaphore` and `binary_semaphore` offer the best performance (see benchmark results below) but some implementations are known to have correctness bugs. Also, my attempt at upgrading CI for C++20 support (required for these) in https://github.com/facebook/rocksdb/pull/13904 is actually incomplete. Therefore, using these structures is opt-in with `-DROCKSDB_USE_STD_SEMAPHORES` at compile time, and a naive semaphore implementation based on mutex and condvar is used by default. A folly alternative (folly::fibers::Semaphore) was dropped in during development and found to be less efficient than the naive implementation. One CI job is upgraded to test with the new opt-in. * One of the biggest concerns about correctness/reliability for this implementation is the possibility of hitting a deadlock, in part because that is not well checked in the DB crash test (a challenging problem!). Note also that with the parallel compression improvements in this release, I am calling the feature production-ready, so there is an extra level of confidence needed in the reliability of the feature. Thus, for DEBUG builds including crash test, I have added a watchdog thread to each parallel SST construction that heuristically checks for the most likely kinds of deadlock that could happen, including for the case of buggy semaphore implementations. It periodically verifies that some thread is outside of its "idle" state, and if the watchdog wakes up repeatedly to see all live threads stuck in their idle state (even if wake-up was attempted) then it declares a deadlock. This feature was manually verified for several seeded deadlock bugs. (More details in code comments.) * For CPU efficiency, this implementation greatly simplifies the logic to estimate the outstanding or "inflight" size not yet written to the SST file. I expect this size to generally be insignificant relative to the full SST file size so is not worth careful engineering. And based on Meta's current needs, landing under-size for an SST file is better than over-size. See comments on `estimated_inflight_size` for details. * Some other existing atomics in block_based_table_builder.cc modified to use safe atomic wrappers. * Status handling in BlockBasedTableBuilder was streamlined to get rid of essentially redundant `status`+`io_status` fields and associated code. Made small optimizations to reduce unnecessary IOStatus copies (with StatusOk()) and mark status conditional branches as LIKELY or UNLIKELY. * Prefer inline field initialization to initialization in constructor. * Minimize references to the `parallel_threads` configuration parameter for better separation of concerns / sanitization / etc. For example, use non-nullity of `pc_rep` to indicate that parallel compression is enabled (and active). * Some other refactoring to aid the new implementation. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13910 Test Plan: ## Correctness Already integrated into unit tests and crash test. CI updated for opt-in semaphore implementation. Basic semaphore unit tests added/updated. As for the tremendous simplification of logic relating to hitting target SST file size, as expected, the new behavior could under-shoot the single-threaded behavior by a small number of blocks, which will typically affect the file size by ~1/1000th or less. I think that's a good trade-off for cutting out unnecessarily complex code with non-trivial CPU cost (FileSizeEstimator). ``` ./db_bench -db=/dev/shm/dbbench_filesize_after8 -benchmarks=fillseq,compact -num=10000000 -compression_type=zstd -compression_level=8 -compression_parallel_threads=8 ``` Before, PT=8 & PT=1, and After PT=1 the same or very similar ``` -rw-r--r-- 1 peterd users 67474097 Sep 12 15:32 000052.sst -rw-r--r-- 1 peterd users 67474214 Sep 12 15:32 000053.sst -rw-r--r-- 1 peterd users 67473834 Sep 12 15:32 000054.sst -rw-r--r-- 1 peterd users 67473437 Sep 12 15:32 000055.sst -rw-r--r-- 1 peterd users 67473835 Sep 12 15:32 000056.sst -rw-r--r-- 1 peterd users 67473204 Sep 12 15:33 000057.sst -rw-r--r-- 1 peterd users 67473294 Sep 12 15:33 000058.sst -rw-r--r-- 1 peterd users 67473839 Sep 12 15:33 000059.sst ``` After, PT=8 (worst case here ~0.05% smaller) ``` -rw-r--r-- 1 peterd users 67463189 Sep 12 14:55 000052.sst -rw-r--r-- 1 peterd users 67465233 Sep 12 14:55 000053.sst -rw-r--r-- 1 peterd users 67466822 Sep 12 14:55 000054.sst -rw-r--r-- 1 peterd users 67466221 Sep 12 14:55 000055.sst -rw-r--r-- 1 peterd users 67441675 Sep 12 14:55 000056.sst -rw-r--r-- 1 peterd users 67467855 Sep 12 14:55 000057.sst -rw-r--r-- 1 peterd users 67455132 Sep 12 14:55 000058.sst -rw-r--r-- 1 peterd users 67458334 Sep 12 14:55 000059.sst ``` ## Performance, modest load We are primarily interested in balancing throughput in building SST files and CPU usage in doing so. (For example, we could maximize throughput by having worker threads only spin waiting for work, but that would likely be extra CPU usage we want to avoid to allow other productive CPU work to be scheduled.) No read path code has been touched. A benchmark script running "before" and "after" configurations at the same time to minimize random machine load effects: ``` $ SUFFIX=`tty | sed 's|/|_|g'`; for CT in none lz4 zstd; do for PT in 1 2 3 4 6 8; do echo -n "$CT pt=$PT -> "; (for I in `seq 1 10`; do BIN=/tmp/dbbench${SUFFIX}.bin; rm -f $BIN; cp db_bench $BIN; /usr/bin/time $BIN -db=/dev/shm/dbbench$SUFFIX --benchmarks=fillseq -num=10000000 -compaction_style=2 -fifo_compaction_max_table_files_size_mb=1000 -fifo_compaction_allow_compaction=0 -disable_wal -write_buffer_size=12000000 -format_version=7 -compression_type=$CT -compression_parallel_threads=$PT 2>&1; done) | awk '/micros.op/ {n++; sum += $5;} /system / { cpu += $1 + $2; } END { print "ops/s: " int(sum/n) " cpu*s: " cpu; }'; done; done ``` Before this change: ``` none pt=1 -> ops/s: 1999603 cpu*s: 72.08 none pt=2 -> ops/s: 1871094 cpu*s: 148.3 none pt=3 -> ops/s: 1882907 cpu*s: 147.7 lz4 pt=1 -> ops/s: 1987858 cpu*s: 94.74 lz4 pt=2 -> ops/s: 1590192 cpu*s: 182.65 lz4 pt=3 -> ops/s: 1896294 cpu*s: 174.7 lz4 pt=4 -> ops/s: 1949174 cpu*s: 172.26 lz4 pt=6 -> ops/s: 1912517 cpu*s: 175.91 lz4 pt=8 -> ops/s: 1930585 cpu*s: 176.71 zstd pt=1 -> ops/s: 1239379 cpu*s: 129.85 zstd pt=2 -> ops/s: 1171742 cpu*s: 226.12 zstd pt=3 -> ops/s: 1832574 cpu*s: 214.21 zstd pt=4 -> ops/s: 1887124 cpu*s: 212.51 zstd pt=6 -> ops/s: 1920936 cpu*s: 211.7 zstd pt=8 -> ops/s: 1885544 cpu*s: 214.87 ``` After this change: ``` none pt=1 -> ops/s: 1964361 cpu*s: 72.66 none pt=2 -> ops/s: 1914033 cpu*s: 104.95 none pt=3 -> ops/s: 1978567 cpu*s: 100.24 lz4 pt=1 -> ops/s: 2041703 cpu*s: 92.88 lz4 pt=2 -> ops/s: 1903210 cpu*s: 121.64 lz4 pt=3 -> ops/s: 1973906 cpu*s: 122.22 lz4 pt=4 -> ops/s: 1952605 cpu*s: 123.05 lz4 pt=6 -> ops/s: 1957524 cpu*s: 124.31 lz4 pt=8 -> ops/s: 1986274 cpu*s: 129.06 zstd pt=1 -> ops/s: 1233748 cpu*s: 130.43 zstd pt=2 -> ops/s: 1675226 cpu*s: 158.41 zstd pt=3 -> ops/s: 1929878 cpu*s: 159.77 zstd pt=4 -> ops/s: 1916403 cpu*s: 160.99 zstd pt=6 -> ops/s: 1942526 cpu*s: 166.21 zstd pt=8 -> ops/s: 1966704 cpu*s: 171.56 ``` For parallel_threads=1, results are very similar, as expected. For parallel_threads>1, throughput is usually improved a bit, but cpu consumption is dramatically reduced. For zstd, maximum throughput is essentially achieved with pt=3 rather than the previous roughly pt=4 to 6. And the old used about 30% more CPU. We can also compare with more expensive compression by raising the compression level. ``` SUFFIX=`tty | sed 's|/|_|g'`; CT=zstd; for CL in 4 6 8; do for PT in 1 4 8; do echo -n "$CT@$CL pt=$PT -> "; (for I in `seq 1 10`; do BIN=/tmp/dbbench${SUFFIX}.bin; rm -f $BIN; cp db_bench $BIN; /usr/bin/time $BIN -db=/dev/shm/dbbench$SUFFIX --benchmarks=fillseq -num=10000000 -compaction_style=2 -fifo_compaction_max_table_files_size_mb=1000 -fifo_compaction_allow_compaction=0 -disable_wal -write_buffer_size=12000000 -format_version=7 -compression_type=$CT -compression_parallel_threads=$PT -compression_level=$CL 2>&1; done) | awk '/micros.op/ {n++; sum += $5;} /system / { cpu += $1 + $2; } END { print "ops/s: " int(sum/n) " cpu*s: " cpu; }'; done; done ``` Before: ``` zstd@4 pt=1 -> ops/s: 883630 cpu*s: 161.12 zstd@4 pt=4 -> ops/s: 1878206 cpu*s: 243.25 zstd@4 pt=8 -> ops/s: 1885002 cpu*s: 245.89 zstd@6 pt=1 -> ops/s: 710767 cpu*s: 189.44 zstd@6 pt=4 -> ops/s: 1706377 cpu*s: 277.29 zstd@6 pt=8 -> ops/s: 1866736 cpu*s: 275.07 zstd@8 pt=1 -> ops/s: 529047 cpu*s: 237.87 zstd@8 pt=4 -> ops/s: 1401379 cpu*s: 330.61 zstd@8 pt=8 -> ops/s: 1895601 cpu*s: 321.59 ``` After: ``` zstd@4 pt=1 -> ops/s: 889905 cpu*s: 161.03 zstd@4 pt=4 -> ops/s: 1942240 cpu*s: 193.18 zstd@4 pt=8 -> ops/s: 1922367 cpu*s: 205.21 zstd@6 pt=1 -> ops/s: 713870 cpu*s: 188.91 zstd@6 pt=4 -> ops/s: 1832314 cpu*s: 219.66 zstd@6 pt=8 -> ops/s: 1949631 cpu*s: 229.34 zstd@8 pt=1 -> ops/s: 530324 cpu*s: 238.02 zstd@8 pt=4 -> ops/s: 1479767 cpu*s: 271.65 zstd@8 pt=8 -> ops/s: 1949631 cpu*s: 275.6 ``` And we can also look at the cumulative effect of this change and https://github.com/facebook/rocksdb/pull/13850 that will combine for the parallel compression improvements in the upcoming 10.7 release: Before both: ``` lz4 pt=1 -> ops/s: 1954445 cpu*s: 95.14 lz4 pt=3 -> ops/s: 1687043 cpu*s: 186.62 lz4 pt=5 -> ops/s: 1708196 cpu*s: 188.33 zstd pt=1 -> ops/s: 1220649 cpu*s: 131.2 zstd pt=3 -> ops/s: 1658100 cpu*s: 227.08 zstd pt=5 -> ops/s: 1685074 cpu*s: 226.08 ``` After: ``` lz4 pt=1 -> ops/s: 2048214 cpu*s: 93.24 lz4 pt=3 -> ops/s: 1922049 cpu*s: 122.9 lz4 pt=5 -> ops/s: 1980165 cpu*s: 122.49 zstd pt=1 -> ops/s: 1245165 cpu*s: 128.84 zstd pt=3 -> ops/s: 1956961 cpu*s: 158.73 zstd pt=5 -> ops/s: 1970458 cpu*s: 161.02 ``` In summary, before with zstd default level, you could see only * about 38% increase in throughput for about 73% increase in CPU usage Now you can get * about 58% increase in throughput for about 25% increase in CPU usage ## Performance, high load To validate this for usage on remote compaction workers, we also need to test whether it falls over at high load or anything concerning like that. For this I did a lot of testing with concurrent db_bench and zstd compression_level=8 and parallel_thread (PT) in {1,8} trying to observe "bad" behaviors such as stalls due to preempted threads and such. On a 166 core machine where a "job" is a db_bench process running a fillseq benchmark similar to above in parallel with others, I could summarize the results like this: 10 jobs PT=8 vs. PT=1 -> 12% more CPU usage, 75% reduction in wall time, 1.9 jobs/sec (vs. 0.5) 50 jobs PT=8 vs. PT=1 -> 89% more CPU usage, 27% reduction in wall time, 3.1 jobs/sec (vs. 2.3) 100 jobs PT=8 vs. PT=1 -> 24% more CPU usage, 5% reduction in wall time, 3.25 jobs/sec (vs. 3.1) 150 jobs PT=8 vs. PT=1 -> 4% more CPU usage, 2% increase in wall time, 3.3 jobs/sec (vs. 3.4) 500 jobs PT=8 vs. PT=1 -> 1% more CPU usage, insignificant difference in wall time, 3.3 jobs/sec Even when there are 4000 threads potentially competing for 166 cores, the throughput (3.3 jobs / sec) is still very close to maximum (3.4). Enabling parallel compression didn't result in notably less throughput (based on wall clock time for all jobs to complete) in any case tested above, and much higher throughput for many cases. If parallel compression causes us to tip from comfortably under-saturating to over-saturating the cores (as in the 50 jobs case), the overall CPU usage can be much higher, presumably due to lower CPU cache hit rates and maybe clock throttling, but parallel compression still has the throughput advantage in those cases. In other words, what would we stand to gain from being able to intelligently share worker threads between compaction jobs? It doesn't seem that much. Reviewed By: xingbowang Differential Revision: D81365623 Pulled By: pdillinger fbshipit-source-id: 5db5151a959b5d25b84dbe185bc208bd188f2d1c |
||
|
Peter Dillinger
|
2950e99219 |
Require C++20 (#13904)
Summary: I am wanting to use std::counting_semaphore for something and the timing seems good to require C++20 support. The internets suggest: * GCC >= 10 is adequate, >= 11 preferred * Clang >= 10 is needed * Visual Studio >= 2019 is adquate And popular linux distributions look like this: * CentOS Stream 9 -> GCC 11.2 (CentOS 8 is EOL) * Ubuntu 22.04 LTS -> GCC 11.x (Ubuntu 20 just ended standard support) * Debian 12 (oldstable) -> GCC 12.2 * (Debian 11 has ended security updates, uses GCC 10.2) This required generating a new docker image based on Ubuntu 22 for CI using gcc. The existing Ubuntu 20 image works for covering appropriate clang versions (though we should maybe add a much later version as well, in the next increment of our Ubuntu 22 image; however the minimum available clang build from apt.llvm.org for Ubuntu 22 is clang 13). Update to SetDumpFilter is to quiet a mysterious gcc-13 warning-as-error. Removed --compile-no-warning-as-error from a cmake command line because cmake in the new docker image is too old for this option. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13904 Test Plan: CI, one minor unit test added to verify std::counting_semaphor works Reviewed By: xingbowang Differential Revision: D81266435 Pulled By: pdillinger fbshipit-source-id: 26040eeccca7004416e29a6ff4f6ea93f2052684 |
||
|
Peter Dillinger
|
78c83ac1ec |
Publish/support format_version=7, related enhancements (#13713)
Summary: * Make new format_version=7 a supported setting. * Fix a bug in compressed_secondary_cache.cc that is newly exercised by custom compression types and showing up in crash test with tiered secondary cache * Small change to handling of disabled compression in fv=7: use empty compression manager compatibility name. * Get rid of GetDefaultBuiltinCompressionManager() in public API because it could cause unexpected+unsafe schema change on a user's CompressionManager if built upon the default built-in manager and we add a new built-in schema. Now must be referenced by explicit compression schema version in the public API. (That notion was already exposed in compressed secondary cache API, for better or worse.) * Improve some error messages for compression misconfiguration * Improve testing with ObjectLibrary and CompressionManagers * Improve testing of compression_name table property in BlockBasedTableTest.BlockBasedTableProperties2 * Improve some comments Pull Request resolved: https://github.com/facebook/rocksdb/pull/13713 Test Plan: existing and updated tests. Notably, the crash test has already been running with (unpublished) format_version=7 Reviewed By: mszeszko-meta, hx235 Differential Revision: D77035482 Pulled By: pdillinger fbshipit-source-id: 95278de8734a79706a22361bff2184b1edb230ca |
||
|
Peter Dillinger
|
09175119d2 |
Allow SmallEnumSet on larger enum types (#13657)
Summary: ... to support SmallEnumSet over CompressionType with allowed custom compression types using most of the available byte. This is accomplished using an std::array<uint64_t> in place of just uint64_t. Also adds an std::bitset-like count() operation. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13657 Test Plan: unit tests included Reviewed By: hx235 Differential Revision: D75827601 Pulled By: pdillinger fbshipit-source-id: 519ae97ac671fd9885d6485976abbd969d1392d3 |
||
|
Peter Dillinger
|
c3c0cfc3a8 |
Create an UnownedPtr type (#12447)
Summary: ... that is more hygienic as an "optional reference" than a raw pointer, and likely more efficient than std::optional<std::reference_wrapper<T>>. Pull Request resolved: https://github.com/facebook/rocksdb/pull/12447 Test Plan: unit test included (with manual verification that "must not compile" sections currently do not) Reviewed By: jowlyzhang Differential Revision: D54957917 Pulled By: pdillinger fbshipit-source-id: bbd89218df803617b1a170ebddc9e56c9b52bf93 |
||
yuzhangyu@fb.com
|
1cfdece85d |
Run internal cpp modernizer on RocksDB repo (#12398)
Summary: When internal cpp modernizer attempts to format rocksdb code, it will replace macro `ROCKSDB_NAMESPACE` with its default definition `rocksdb` when collapsing nested namespace. We filed a feedback for the tool T180254030 and the team filed a bug for this: https://github.com/llvm/llvm-project/issues/83452. At the same time, they suggested us to run the modernizer tool ourselves so future auto codemod attempts will be smaller. This diff contains: Running `xplat/scripts/codemod_service/cpp_modernizer.sh` in fbcode/internal_repo_rocksdb/repo (excluding some directories in utilities/transactions/lock/range/range_tree/lib that has a non meta copyright comment) without swapping out the namespace macro `ROCKSDB_NAMESPACE` Followed by RocksDB's own `make format` Pull Request resolved: https://github.com/facebook/rocksdb/pull/12398 Test Plan: Auto tests Reviewed By: hx235 Differential Revision: D54382532 Pulled By: jowlyzhang fbshipit-source-id: e7d5b40f9b113b60e5a503558c181f080b9d02fa |
||
|
Peter Dillinger
|
54cb9c77d9 |
Prefer static_cast in place of most reinterpret_cast (#12308)
Summary: The following are risks associated with pointer-to-pointer reinterpret_cast: * Can produce the "wrong result" (crash or memory corruption). IIRC, in theory this can happen for any up-cast or down-cast for a non-standard-layout type, though in practice would only happen for multiple inheritance cases (where the base class pointer might be "inside" the derived object). We don't use multiple inheritance a lot, but we do. * Can mask useful compiler errors upon code change, including converting between unrelated pointer types that you are expecting to be related, and converting between pointer and scalar types unintentionally. I can only think of some obscure cases where static_cast could be troublesome when it compiles as a replacement: * Going through `void*` could plausibly cause unnecessary or broken pointer arithmetic. Suppose we have `struct Derived: public Base1, public Base2`. If we have `Derived*` -> `void*` -> `Base2*` -> `Derived*` through reinterpret casts, this could plausibly work (though technical UB) assuming the `Base2*` is not dereferenced. Changing to static cast could introduce breaking pointer arithmetic. * Unnecessary (but safe) pointer arithmetic could arise in a case like `Derived*` -> `Base2*` -> `Derived*` where before the Base2 pointer might not have been dereferenced. This could potentially affect performance. With some light scripting, I tried replacing pointer-to-pointer reinterpret_casts with static_cast and kept the cases that still compile. Most occurrences of reinterpret_cast have successfully been changed (except for java/ and third-party/). 294 changed, 257 remain. A couple of related interventions included here: * Previously Cache::Handle was not actually derived from in the implementations and just used as a `void*` stand-in with reinterpret_cast. Now there is a relationship to allow static_cast. In theory, this could introduce pointer arithmetic (as described above) but is unlikely without multiple inheritance AND non-empty Cache::Handle. * Remove some unnecessary casts to void* as this is allowed to be implicit (for better or worse). Most of the remaining reinterpret_casts are for converting to/from raw bytes of objects. We could consider better idioms for these patterns in follow-up work. I wish there were a way to implement a template variant of static_cast that would only compile if no pointer arithmetic is generated, but best I can tell, this is not possible. AFAIK the best you could do is a dynamic check that the void* conversion after the static cast is unchanged. Pull Request resolved: https://github.com/facebook/rocksdb/pull/12308 Test Plan: existing tests, CI Reviewed By: ltamasi Differential Revision: D53204947 Pulled By: pdillinger fbshipit-source-id: 9de23e618263b0d5b9820f4e15966876888a16e2 |
||
|
Peter Dillinger
|
141b872bd4 |
Improve efficiency of create_missing_column_families, light refactor (#11920)
Summary: In preparing some seqno_to_time_mapping improvements, I found that some of the wrap-up work for creating column families was unnecessarily repeated in the case of DB::Open with create_missing_column_families. This change fixes that (`CreateColumnFamily()` -> `CreateColumnFamilyImpl()` in `DBImpl::Open()`), motivated by avoiding repeated calls to `RegisterRecordSeqnoTimeWorker()` but with the side benefit of avoiding repeated calls to `WriteOptionsFile()` for each CF. Also in this change: * Add a `Status::UpdateIfOk()` function for combining statuses in a common pattern * Rename `max_time_duration` -> `min_preserve_seconds` (include units as much as possible) * Improved comments in several places Pull Request resolved: https://github.com/facebook/rocksdb/pull/11920 Test Plan: tests added / updated Reviewed By: jaykorean Differential Revision: D49919147 Pulled By: pdillinger fbshipit-source-id: 3d0318c1d070c842c5331da0a5b415caedc104f1 |
||
Peter Dillinger
|
34bb3ddc43 |
Improve SmallEnumSet (#11178)
Summary: In anticipation of using this to represent sets of CacheEntryRole for including or excluding kinds of blocks in block cache tiers, add significant new features to SmallEnumSet, including at least: * List initialization * Applicative constexpr operations * copy/move/equality ops * begin/end/const_iterator for iteration * Better comments Pull Request resolved: https://github.com/facebook/rocksdb/pull/11178 Test Plan: unit tests added/expanded Reviewed By: ltamasi Differential Revision: D42973723 Pulled By: pdillinger fbshipit-source-id: 40783486feda931c3f7c6fcc9a300acd6a4b0a0a |
||
|
Peter Dillinger
|
7fff38b1fe |
clang-format cache/ and util/ directories (#10867)
Summary: This is purely the result of running `clang-format -i` on files, except some files have been excluded for manual intervention in a separate PR Pull Request resolved: https://github.com/facebook/rocksdb/pull/10867 Test Plan: `make check`, `make check-headers`, `make format` Reviewed By: jay-zhuang Differential Revision: D40682086 Pulled By: pdillinger fbshipit-source-id: 8673d978553ab99b516da7fb63ba0b82523337f8 |
||
|
Peter Dillinger
|
fd3e0f43b3 |
Require C++17 (#9481)
Summary: Drop support for some old compilers by requiring C++17 standard (or higher). See https://github.com/facebook/rocksdb/issues/9388 First modification based on this is to remove some conditional compilation in slice.h (also better for ODR) Also in this PR: * Fix some Makefile formatting that seems to affect ASSERT_STATUS_CHECKED config in some cases * Add c_test to NON_PARALLEL_TEST in Makefile * Fix a clang-analyze reported "potential leak" in lru_cache_test * Better "compatibility" definition of DEFINE_uint32 for old versions of gflags * Fix a linking problem with shared libraries in Makefile (`./random_test: error while loading shared libraries: librocksdb.so.6.29: cannot open shared object file: No such file or directory`) * Always set ROCKSDB_SUPPORT_THREAD_LOCAL and use thread_local (from C++11) * TODO in later PR: clean up that obsolete flag * Fix a cosmetic typo in c.h (https://github.com/facebook/rocksdb/issues/9488) Pull Request resolved: https://github.com/facebook/rocksdb/pull/9481 Test Plan: CircleCI config substantially updated. * Upgrade to latest Ubuntu images for each release * Generally prefer Ubuntu 20, but keep a couple Ubuntu 16 builds with oldest supported compilers, to ensure compatibility * Remove .circleci/cat_ignore_eagain except for Ubuntu 16 builds, because this is to work around a kernel bug that should not affect anything but Ubuntu 16. * Remove designated gcc-9 build, because the default linux build now uses GCC 9 from Ubuntu 20. * Add some `apt-key add` to fix some apt "couldn't be verified" errors * Generally drop SKIP_LINK=1; work-around no longer needed * Generally `add-apt-repository` before `apt-get update` as manual testing indicated the reverse might not work. Travis: * Use gcc-7 by default (remove specific gcc-7 and gcc-4.8 builds) * TODO in later PR: fix s390x "Assembler messages: Error: invalid switch -march=z14" failure AppVeyor: * Completely dropped because we are dropping VS2015 support and CircleCI covers VS >= 2017 Also local testing with old gflags (out of necessity when using ROCKSDB_NO_FBCODE=1). Reviewed By: mrambacher Differential Revision: D33946377 Pulled By: pdillinger fbshipit-source-id: ae077c823905b45370a26c0103ada119459da6c1 |
||
|
Peter Dillinger
|
449029f865 |
Remove deprecated ObjectLibrary::Register() (and Regex public API) (#9439)
Summary: Regexes are considered potentially problematic for use in registering RocksDB extensions, so we are removing ObjectLibrary::Register() and the Regex public API it depended on (now unused). In reference to https://github.com/facebook/rocksdb/issues/9389 Why? * The power of Regexes can make it hard to reason about which extension will match what. (The replacement API isn't perfect, but we are at least "holding the line" on patterns we have seen in practice.) * It is easy to make regexes that don't quite mean what you think they mean, such as forgetting that the `.` in `foo.bar` can match any character or that matching is nondeterministic, as in `a:b:42` matching `.*:[0-9]+`. * Some regexes and implementations can have disastrously bad performance. This might not be much practical concern for ObjectLibray here, but we don't want to encourage potentially dangerous further use in production code. (Testing code is fine. See TestRegex.) Pull Request resolved: https://github.com/facebook/rocksdb/pull/9439 Test Plan: CI Reviewed By: mrambacher Differential Revision: D33792342 Pulled By: pdillinger fbshipit-source-id: 4f64dcb04764e639162c8977a5fa196f67754cec |
||
|
Peter Dillinger
|
0ef88538c6 |
Improve support for using regexes (#8740)
Summary: * Consolidate use of std::regex for testing to testharness.cc, to minimize Facebook linters constantly flagging uses in non-production code. * Improve syntax and error messages for asserting some string matches a regex in tests. * Add a public Regex wrapper class to encapsulate existing usage in ObjectRegistry. * Remove unnecessary include <regex> * Put warnings that use of Regex in production code could cause bad performance or stack overflow. Intended follow-up work: * Replace std::regex with another underlying implementation like RE2 * Improve ObjectRegistry interface in terms of possibly confusing literal string matching vs. regex and in terms of reporting invalid regex. Pull Request resolved: https://github.com/facebook/rocksdb/pull/8740 Test Plan: tests updated, basic unit test for public Regex, and some manual testing of temporary changes to see example error messages: utilities/backupable/backupable_db_test.cc:917: Failure 000010_1162373755_138626.blob (child.name) does not match regex [0-9]+_[0-9]+_[0-9]+[.]blobHAHAHA (pattern) db/db_basic_test.cc:74: Failure R3SHSBA8C4U0CIMV2ZB0 (sid3) does not match regex [0-9A-Z]{20}HAHAHA Reviewed By: mrambacher Differential Revision: D30706246 Pulled By: pdillinger fbshipit-source-id: ba845e8f563ccad39bdb58f44f04e9da8f78c3fd |
||
Zhichao Cao
|
d1c510baec |
Handoff checksum Implementation (#7523)
Summary: in PR https://github.com/facebook/rocksdb/issues/7419 , we introduce the new Append and PositionedAppend APIs to WritableFile at File System, which enable RocksDB to pass the data verification information (e.g., checksum of the data) to the lower layer. In this PR, we use the new API in WritableFileWriter, such that the file created via WritableFileWrite can pass the checksum to the storage layer. To control which types file should apply the checksum handoff, we add checksum_handoff_file_types to DBOptions. User can use this option to control which file types (Currently supported file tyes: kLogFile, kTableFile, kDescriptorFile.) should use the new Append and PositionedAppend APIs to handoff the verification information. Pull Request resolved: https://github.com/facebook/rocksdb/pull/7523 Test Plan: add new unit test, pass make check/ make asan_check Reviewed By: pdillinger Differential Revision: D24313271 Pulled By: zhichao-cao fbshipit-source-id: aafd69091ae85c3318e3e17cbb96fe7338da11d0 |
||
sdong
|
fdf882ded2 |
Replace namespace name "rocksdb" with ROCKSDB_NAMESPACE (#6433)
Summary: When dynamically linking two binaries together, different builds of RocksDB from two sources might cause errors. To provide a tool for user to solve the problem, the RocksDB namespace is changed to a flag which can be overridden in build time. Pull Request resolved: https://github.com/facebook/rocksdb/pull/6433 Test Plan: Build release, all and jtest. Try to build with ROCKSDB_NAMESPACE with another flag. Differential Revision: D19977691 fbshipit-source-id: aa7f2d0972e1c31d75339ac48478f34f6cfcfb3e |
||
Cheng Chang
|
d3ba398bbd |
Update unit tests for PinnableSlice (#6399)
Summary: 1. remove AssertEmpty because calling methods on moved objects is discouraged. 2. add a test to assert that the internal buffer is moved instead of being copied. Pull Request resolved: https://github.com/facebook/rocksdb/pull/6399 Test Plan: make slice_test && ./slice_test USE_CLANG=1 make analyze Differential Revision: D19825372 Pulled By: cheng-chang fbshipit-source-id: 2e26f8ce5ec3edbfce067db045e80bd433e704f4 |
||
|
Cheng Chang
|
b42fa1497f |
Support move semantics for PinnableSlice (#6374)
Summary: It's logically correct for PinnableSlice to support move semantics to transfer ownership of the pinned memory region. This PR adds both move constructor and move assignment to PinnableSlice. Pull Request resolved: https://github.com/facebook/rocksdb/pull/6374 Test Plan: A set of unit tests for the move semantics are added in slice_test. So `make slice_test && ./slice_test`. Differential Revision: D19739254 Pulled By: cheng-chang fbshipit-source-id: f898bd811bb05b2d87384ec58b645e9915e8e0b1 |
||
Yi Wu
|
42564ada53 |
Blob DB: not using PinnableSlice move assignment
Summary: The current implementation of PinnableSlice move assignment have an issue #3163. We are moving away from it instead of try to get the move assignment right, since it is too tricky. Closes https://github.com/facebook/rocksdb/pull/3164 Differential Revision: D6319201 Pulled By: yiwu-arbug fbshipit-source-id: 8f3279021f3710da4a4caa14fd238ed2df902c48 |
||
|
Yi Wu
|
be410dede8 |
Fix PinnableSlice move assignment
Summary: After move assignment, we need to re-initialized the moved PinnableSlice. Also update blob_db_impl.cc to not reuse the moved PinnableSlice since it is supposed to be in an undefined state after move. Closes https://github.com/facebook/rocksdb/pull/3127 Differential Revision: D6238585 Pulled By: yiwu-arbug fbshipit-source-id: bd99f2e37406c4f7de160c7dee6a2e8126bc224e |
||
|
Yi Wu
|
31d3e41810 |
PinnableSlice move assignment
Summary: Allow `std::move(pinnable_slice)`. Closes https://github.com/facebook/rocksdb/pull/2997 Differential Revision: D6036782 Pulled By: yiwu-arbug fbshipit-source-id: 583fb0419a97e437ff530f4305822341cd3381fa |