Summary:
Pull Request resolved: https://github.com/facebook/rocksdb/pull/15387
## Summary
RocksDB's `.clang-tidy` enables `cppcoreguidelines-special-member-functions`,
`cppcoreguidelines-virtual-class-destructor` and the noexcept-move checks,
and clang warns `-Wdeprecated-copy-with-user-provided-dtor`. A full scan
reports 476 findings. In most of them, a class declares a destructor (or
copy operations) but leaves the rest of copy/move implicit. That has two
costs. Readers can't tell whether a class may be copied or moved. And
resource-owning classes keep an implicit copy that would double-free if
anyone ever used it. This diff resolves 453 of the 476.
Approach, picked per class from least to most invasive:
1. An empty user-provided destructor becomes `= default`. This covers most
interface classes, such as `virtual ~X() {}` -> `= default`. If the
class is still copied, that would only rename the deprecated-copy
warning, so the destructor is removed instead when nothing needs it
(`Customizable`, `TableFactory` and `InternalKeyComparator` inherit a
virtual one; `IteratorWrapperBase` isn't polymorphic), and
`MemTableRep::KeyComparator` gets defaulted copy and move.
2. Classes whose copy is already deleted get explicitly deleted move
operations too. Move was already unavailable, so this only documents
it.
3. Resource-owning classes that are never copied get deleted copy and move
operations. Examples: the C API wrapper structs, which run a
`destructor_` callback (edited in `tools/c_api_gen/c_base.cc` and
regenerated into `db/c.cc`); `ShardedCache`, which frees its shards;
iterator wrappers; and RAII handles.
4. Value types that are copied or moved get the missing operations as
`= default`, which keeps their current behavior.
5. Exception specifications of existing functions are unchanged, except
for `WriteBatch`'s move assignment (below): no other `noexcept` is
added or removed. Only the move operations this diff
newly declares (`Comparator`, `FileSampledStats` and the thread-local
`Entry`) are `noexcept`; they copy only scalars or atomics.
`WriteBatch`'s copy and move assignment destroyed `*this` and
placement-new'ed a new `WriteBatch` over it. That is undefined behavior
when `*this` is a subclass, and if the copy constructor threw, `*this`
was left destroyed and was later destroyed again. Move assignment now
assigns member-wise and ends in the same state as the move constructor,
which leaves `has_key_with_ts_`, the timestamp-size tracking and the
other fields it doesn't copy at their defaults. `rep_` is swapped through
a temporary, because a plain string move-assignment can keep the old
buffer (e.g. a large batch reset with `batch = WriteBatch()` would keep
its memory). Nothing in it can throw, so it is now `noexcept`. Copy
assignment copies into a temporary and moves it in, so a throwing copy
leaves `*this` unchanged.
Bugs this exposed and fixed:
- `RestoreAfterCopyOrCreateWorkItem` (backup engine): the hand-written
move constructor and assignment moved only some fields, so `from_file`
and `to_file` were empty after `push_back(std::move(...))`. A restore
that hit a checksum mismatch then reported "While restoring -> : ..."
without file names. It now uses the compiler-generated moves, which
are still `noexcept`.
- `ExternalSstFileIngestionJob` lived in a `std::vector`, and its
destructor frees raw-owned pointers, so the implicit copy used on
vector reallocation would double-free. Its file batches also point into
its own inline file storage, so neither a copy nor a move is safe after
`Prepare()`. Copy and move are now deleted, and `DBImpl` holds jobs
through `std::unique_ptr`.
Compatibility: no public class loses an operation users can call. Public
interfaces mostly only get `virtual ~X() = default;`.
`WriteBatch`'s move assignment becomes `noexcept`, which is not part of
the mangled name, so the ABI doesn't change.
`SubcompactionJobInfo` and `CompactionJobInfo` intentionally stay C++20
aggregates, so users' aggregate and designated initializers keep
compiling. `Configurable`, `CompareInterface`, `WriteBatchBase`,
`ObjectLibrary::Entry` and `MemTableRep::KeyComparator` declare defaulted
copy and move, and `Customizable` no longer declares a destructor. As a
result, `Configurable` and `Customizable` subclasses that declare no
destructor or copy operations of their own now move their
registered-options list instead of copying it. In-tree, these are the
mutable-options adapters, the table factories, the memory allocators and
test classes; all are held through smart pointers or used in place, and
`Clone()` copies, so none is moved by value.
Not changed (23): `IODebugContext` (custom deep copy plus a
`std::shared_mutex`), `DBWithColumnFamilies` in db_bench (stored in a
`std::vector` with non-movable members), `Snapshot`'s protected virtual
destructor (intended), `ManagedSnapshot`, the `SubcompactionJobInfo` and
`CompactionJobInfo` aggregates, and the 16 noexcept-move-constructor and
noexcept-swap findings on existing functions, including `autovector`'s
move assignment (adding `noexcept` would change their exception
specifications). Copying `ManagedSnapshot` already double-releases the
snapshot. Fixing that needs a real move implementation and is a behavior
change, so it is left for a separate diff.
Reviewed By: rban1
Differential Revision: D123186309
fbshipit-source-id: 6f8e57e233e12199a007407130bb9a1f6d52e8d1
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
Summary:
... which is the default for CentOS 9 and Ubuntu 24, the latter of which is now available in GitHub Actions. Relevant CI job updated.
Re-formatted all cc|c|h files except in third-party/, using
```
clang-format -i `git ls-files | grep -E '[.](cc|c|h)$' | grep -v third-party/`
```
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13233
Test Plan: CI
Reviewed By: jaykorean, archang19
Differential Revision: D67461638
Pulled By: pdillinger
fbshipit-source-id: 0c9ac21a3f5eea6f5ade68bb6af7b6ba16c8b301
Summary:
We haven't been actively mantaining RocksDB LITE recently and the size must have been gone up significantly. We are removing the support.
Most of changes were done through following comments:
unifdef -m -UROCKSDB_LITE `git grep -l ROCKSDB_LITE | egrep '[.](cc|h)'`
by Peter Dillinger. Others changes were manually applied to build scripts, CircleCI manifests, ROCKSDB_LITE is used in an expression and file db_stress_test_base.cc.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/11147
Test Plan: See CI
Reviewed By: pdillinger
Differential Revision: D42796341
fbshipit-source-id: 4920e15fc2060c2cd2221330a6d0e5e65d4b7fe2
Summary:
Old typedef syntax is confusing
Most but not all changes with
perl -pi -e 's/typedef (.*) ([a-zA-Z0-9_]+);/using $2 = $1;/g' list_of_files
make format
Pull Request resolved: https://github.com/facebook/rocksdb/pull/8751
Test Plan: existing
Reviewed By: zhichao-cao
Differential Revision: D30745277
Pulled By: pdillinger
fbshipit-source-id: 6f65f0631c3563382d43347896020413cc2366d9
Summary:
For performance purposes, the lower level routines were changed to use a SystemClock* instead of a std::shared_ptr<SystemClock>. The shared ptr has some performance degradation on certain hardware classes.
For most of the system, there is no risk of the pointer being deleted/invalid because the shared_ptr will be stored elsewhere. For example, the ImmutableDBOptions stores the Env which has a std::shared_ptr<SystemClock> in it. The SystemClock* within the ImmutableDBOptions is essentially a "short cut" to gain access to this constant resource.
There were a few classes (PeriodicWorkScheduler?) where the "short cut" property did not hold. In those cases, the shared pointer was preserved.
Using db_bench readrandom perf_level=3 on my EC2 box, this change performed as well or better than 6.17:
6.17: readrandom : 28.046 micros/op 854902 ops/sec; 61.3 MB/s (355999 of 355999 found)
6.18: readrandom : 32.615 micros/op 735306 ops/sec; 52.7 MB/s (290999 of 290999 found)
PR: readrandom : 27.500 micros/op 871909 ops/sec; 62.5 MB/s (367999 of 367999 found)
(Note that the times for 6.18 are prior to revert of the SystemClock).
Pull Request resolved: https://github.com/facebook/rocksdb/pull/8033
Reviewed By: pdillinger
Differential Revision: D27014563
Pulled By: mrambacher
fbshipit-source-id: ad0459eba03182e454391b5926bf5cdd45657b67
Summary:
Introduces and uses a SystemClock class to RocksDB. This class contains the time-related functions of an Env and these functions can be redirected from the Env to the SystemClock.
Many of the places that used an Env (Timer, PerfStepTimer, RepeatableThread, RateLimiter, WriteController) for time-related functions have been changed to use SystemClock instead. There are likely more places that can be changed, but this is a start to show what can/should be done. Over time it would be nice to migrate most (if not all) of the uses of the time functions from the Env to the SystemClock.
There are several Env classes that implement these functions. Most of these have not been converted yet to SystemClock implementations; that will come in a subsequent PR. It would be good to unify many of the Mock Timer implementations, so that they behave similarly and be tested similarly (some override Sleep, some use a MockSleep, etc).
Additionally, this change will allow new methods to be introduced to the SystemClock (like https://github.com/facebook/rocksdb/issues/7101 WaitFor) in a consistent manner across a smaller number of classes.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/7858
Reviewed By: pdillinger
Differential Revision: D26006406
Pulled By: mrambacher
fbshipit-source-id: ed10a8abbdab7ff2e23d69d85bd25b3e7e899e90
Summary:
Persistent cache feature caused rocks db crash on windows. I posted a issue for it, https://github.com/facebook/rocksdb/issues/6919. I found this is because no "persistent_cache_key_prefix" is generated for persistent cache. Looking repo history, "GetUniqueIdFromFile" is not implemented on Windows. So my fix is adding "NewId()" function in "persistent_cache" and using it to generate prefix for persistent cache. In this PR, i also re-enable related test cases defined in "db_test2" and "persistent_cache_test" for windows.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6932
Test Plan:
1. run related test cases in "db_test2" and "persistent_cache_test" on windows and see it passed.
2. manually run db_bench.exe with "read_cache_path" and verified.
Reviewed By: riversand963
Differential Revision: D21911608
Pulled By: cheng-chang
fbshipit-source-id: cdfd938d54a385edbb2836b13aaa1d39b0a6f1c2
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
Summary:
Move some files under util/ to new directories env/, monitoring/ options/ and cache/
Closes https://github.com/facebook/rocksdb/pull/2090
Differential Revision: D4833681
Pulled By: siying
fbshipit-source-id: 2fd8bef
Summary:
Exposing persistent cache stats (counters) to the user via public API.
Closes https://github.com/facebook/rocksdb/pull/1485
Differential Revision: D4155274
Pulled By: siying
fbshipit-source-id: 30a9f50
Summary: Fix failed persistent cache test introduced by recent PR to support cross-platform build.
Test Plan: Run persistent_cache_test (on multiple platforms)
Reviewers: sdong
Reviewed By: sdong
Subscribers: andrewkr, dhruba, leveldb
Differential Revision: https://reviews.facebook.net/D61467
Summary:
The patch is a continuation of part 5. It glues the abstraction for
file layout and metadata, and flush out the implementation of the API. It
adds unit tests for the implementation.
Test Plan: Run unit tests
Subscribers: andrewkr, dhruba, leveldb
Differential Revision: https://reviews.facebook.net/D57549
Summary:
PersistentCacheOptions class in persistent_cache_tier.h is not used any where yet in the code base
but it break the unity build because it have the same name as PersistentCacheOptions in table/persistent_cache_helper.h
Remove it temporarily, and the @krad can add it again with a different name when we start using it
Test Plan:
make unity_test -j64
make check -j64
Reviewers: kradhakrishnan, sdong
Reviewed By: sdong
Subscribers: andrewkr, dhruba
Differential Revision: https://reviews.facebook.net/D59349
Summary:
This provides provides an implementation of PersistentCacheTier that is
specialized for RAM. This tier does not persist data though.
Why do we need this tier ?
This is ideal as tier 0. This tier can host data that is too hot.
Why can't we use Cache variants ?
Yes you can use them instead. This tier can potentially outperform BlockCache
in RAW mode by virtue of compression and compressed cache in block cache doesn't
seem very popular. Potentially this tier can be modified to under stand the
disadvantage of the tier below and retain data that the tier below is bad at
handling (for example index and bloom data that is huge in size)
Test Plan: Run unit tests added
Subscribers: andrewkr, dhruba, leveldb
Differential Revision: https://reviews.facebook.net/D57069
Summary:
Fix 2 issues that was breaking Windows build
1) double to size_t potential downcast warning
2) port_posix is not ready for windows, avoiding building hash_table_bench to
avoid build break
Test Plan: compile in Windoes and make check
Reviewers: sdong, andrewkr
Subscribers: andrewkr, dhruba, leveldb
Differential Revision: https://reviews.facebook.net/D59265
Summary:
This diff provides the basic interface definitions of persistent read
cache system
PersistentCacheOptions captures the persistent read cache options used to
configure and control the system
PersistentCacheTier provides the basic building block for constructing tiered
cache
PersistentTieredCache provides a logical abstraction of tiers of cache layered
over one another
Test Plan: Compile
Reviewers: sdong
Subscribers: andrewkr, dhruba, leveldb
Differential Revision: https://reviews.facebook.net/D57051
