1
0
mirror of https://github.com/facebook/rocksdb.git synced 2026-10-23 08:09:02 +03:00
Files
rocksdb/utilities/persistent_cache/volatile_tier_impl.cc
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
2026-10-08 17:19:47 -07:00

138 lines
4.0 KiB
C++

// Copyright (c) 2013, Facebook, Inc. All rights reserved.
// This source code is licensed under both the GPLv2 (found in the
// COPYING file in the root directory) and Apache 2.0 License
// (found in the LICENSE.Apache file in the root directory).
//
#include "utilities/persistent_cache/volatile_tier_impl.h"
#include <string>
namespace ROCKSDB_NAMESPACE {
void VolatileCacheTier::DeleteCacheData(VolatileCacheTier::CacheData* data) {
assert(data);
delete data;
}
VolatileCacheTier::~VolatileCacheTier() { index_.Clear(&DeleteCacheData); }
PersistentCache::StatsType VolatileCacheTier::Stats() {
std::map<std::string, double> stat;
stat.insert({"persistent_cache.volatile_cache.hits",
static_cast<double>(stats_.cache_hits_)});
stat.insert({"persistent_cache.volatile_cache.misses",
static_cast<double>(stats_.cache_misses_)});
stat.insert({"persistent_cache.volatile_cache.inserts",
static_cast<double>(stats_.cache_inserts_)});
stat.insert({"persistent_cache.volatile_cache.evicts",
static_cast<double>(stats_.cache_evicts_)});
stat.insert({"persistent_cache.volatile_cache.hit_pct",
static_cast<double>(stats_.CacheHitPct())});
stat.insert({"persistent_cache.volatile_cache.miss_pct",
static_cast<double>(stats_.CacheMissPct())});
auto out = PersistentCacheTier::Stats();
out.push_back(stat);
return out;
}
Status VolatileCacheTier::Insert(const Slice& page_key, const char* data,
const size_t size) {
// precondition
assert(data);
assert(size);
// increment the size
size_ += size;
// check if we have overshot the limit, if so evict some space
while (size_ > max_size_) {
if (!Evict()) {
// unable to evict data, we give up so we don't spike read
// latency
assert(size_ >= size);
size_ -= size;
return Status::TryAgain("Unable to evict any data");
}
}
assert(size_ >= size);
// insert order: LRU, followed by index
std::string key(page_key.data(), page_key.size());
std::string value(data, size);
std::unique_ptr<CacheData> cache_data(
new CacheData(std::move(key), std::move(value)));
bool ok = index_.Insert(cache_data.get());
if (!ok) {
// decrement the size that we incremented ahead of time
assert(size_ >= size);
size_ -= size;
// failed to insert to cache, block already in cache
return Status::TryAgain("key already exists in volatile cache");
}
[[maybe_unused]] CacheData* released_data = cache_data.release();
stats_.cache_inserts_++;
return Status::OK();
}
Status VolatileCacheTier::Lookup(const Slice& page_key,
std::unique_ptr<char[]>* result,
size_t* size) {
CacheData key(std::move(page_key.ToString()));
CacheData* kv;
bool ok = index_.Find(&key, &kv);
if (ok) {
// set return data
result->reset(new char[kv->value.size()]);
memcpy(result->get(), kv->value.c_str(), kv->value.size());
*size = kv->value.size();
// drop the reference on cache data
kv->refs_--;
// update stats
stats_.cache_hits_++;
return Status::OK();
}
stats_.cache_misses_++;
if (next_tier()) {
return next_tier()->Lookup(page_key, result, size);
}
return Status::NotFound("key not found in volatile cache");
}
bool VolatileCacheTier::Erase(const Slice& /*key*/) {
assert(false && "not supported");
return true;
}
bool VolatileCacheTier::Evict() {
CacheData* edata = index_.Evict();
if (!edata) {
// not able to evict any object
return false;
}
stats_.cache_evicts_++;
// push the evicted object to the next level
if (next_tier()) {
// TODO: Should the insert error be ignored?
Status s = next_tier()->Insert(Slice(edata->key), edata->value.c_str(),
edata->value.size());
s.PermitUncheckedError();
}
// adjust size and destroy data
size_ -= edata->value.size();
delete edata;
return true;
}
} // namespace ROCKSDB_NAMESPACE