From c0fd93869cb7660acae4dd2f9c6d389675e224b8 Mon Sep 17 00:00:00 2001 From: sfc-gh-tclinkenbeard Date: Thu, 15 Sep 2022 11:48:04 -0700 Subject: [PATCH 1/3] Add probe::assert::rocksDB code probe annotation --- fdbserver/SimulatedCluster.actor.cpp | 4 ++-- flow/CodeProbe.cpp | 8 ++++++++ flow/include/flow/CodeProbe.h | 5 +++++ 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/fdbserver/SimulatedCluster.actor.cpp b/fdbserver/SimulatedCluster.actor.cpp index 513990c581..c223f22c0d 100644 --- a/fdbserver/SimulatedCluster.actor.cpp +++ b/fdbserver/SimulatedCluster.actor.cpp @@ -1461,7 +1461,7 @@ void SimulationConfig::setStorageEngine(const TestConfig& testConfig) { break; } case 4: { - CODE_PROBE(true, "Simulated cluster using RocksDB storage engine"); + CODE_PROBE(true, "Simulated cluster using RocksDB storage engine", probe::assert::rocksDB); set_config("ssd-rocksdb-v1"); // Tests using the RocksDB engine are necessarily non-deterministic because of RocksDB // background threads. @@ -1471,7 +1471,7 @@ void SimulationConfig::setStorageEngine(const TestConfig& testConfig) { break; } case 5: { - CODE_PROBE(true, "Simulated cluster using Sharded RocksDB storage engine"); + CODE_PROBE(true, "Simulated cluster using Sharded RocksDB storage engine", probe::assert::rocksDB); set_config("ssd-sharded-rocksdb"); // Tests using the RocksDB engine are necessarily non-deterministic because of RocksDB // background threads. diff --git a/flow/CodeProbe.cpp b/flow/CodeProbe.cpp index b5c1c9d2ea..425aa8929d 100644 --- a/flow/CodeProbe.cpp +++ b/flow/CodeProbe.cpp @@ -288,6 +288,14 @@ bool SimOnly::operator()(ICodeProbe* self) const { return g_network->isSimulated(); } +bool RocksDB::operator()(ICodeProbe* self) const { +#ifdef SSD_ROCKSDB_EXPERIMENTAL + return true; +#else + return false; +#endif +} + } // namespace assert } // namespace probe diff --git a/flow/include/flow/CodeProbe.h b/flow/include/flow/CodeProbe.h index 4c41470238..cf0ba436f5 100644 --- a/flow/include/flow/CodeProbe.h +++ b/flow/include/flow/CodeProbe.h @@ -70,6 +70,10 @@ struct SimOnly { constexpr static AnnotationType type = AnnotationType::Assertion; bool operator()(ICodeProbe* self) const; }; +struct RocksDB { + constexpr static AnnotationType type = AnnotationType::Assertion; + bool operator()(ICodeProbe* self) const; +}; template struct AssertOr { @@ -111,6 +115,7 @@ constexpr std::enable_if_t> o constexpr SimOnly simOnly; constexpr auto noSim = !simOnly; +constexpr RocksDB rocksDB; } // namespace assert From b273ad83ff0bb53fa68012ca38085ecb0eff9749 Mon Sep 17 00:00:00 2001 From: sfc-gh-tclinkenbeard Date: Thu, 15 Sep 2022 16:12:51 -0700 Subject: [PATCH 2/3] Avoid tracing unexpected code probes --- flow/CodeProbe.cpp | 14 +++++++------- flow/include/flow/CodeProbe.h | 21 ++++++++++++++++++--- 2 files changed, 25 insertions(+), 10 deletions(-) diff --git a/flow/CodeProbe.cpp b/flow/CodeProbe.cpp index 425aa8929d..38961acbd6 100644 --- a/flow/CodeProbe.cpp +++ b/flow/CodeProbe.cpp @@ -228,12 +228,12 @@ void CodeProbes::traceMissedProbes(Optional context) const { std::tie(iter, std::ignore) = locations.emplace(probe.first, false); iter->second = iter->second || probe.second->wasHit(); } - for (auto probe : codeProbes) { - auto iter = locations.find(probe.first); + for (const auto& [loc, probe] : codeProbes) { + auto iter = locations.find(loc); ASSERT(iter != locations.end()); - if (!iter->second) { + if (!iter->second && probe->shouldTrace()) { iter->second = true; - probe.second->trace(false); + probe->trace(false); } } } @@ -280,15 +280,15 @@ void ICodeProbe::printProbesJSON(std::vector const& ctxs) { // annotations namespace assert { -bool NoSim::operator()(ICodeProbe* self) const { +bool NoSim::operator()(ICodeProbe const* self) const { return !g_network->isSimulated(); } -bool SimOnly::operator()(ICodeProbe* self) const { +bool SimOnly::operator()(ICodeProbe const* self) const { return g_network->isSimulated(); } -bool RocksDB::operator()(ICodeProbe* self) const { +bool RocksDB::operator()(ICodeProbe const* self) const { #ifdef SSD_ROCKSDB_EXPERIMENTAL return true; #else diff --git a/flow/include/flow/CodeProbe.h b/flow/include/flow/CodeProbe.h index cf0ba436f5..e4c4e3ea38 100644 --- a/flow/include/flow/CodeProbe.h +++ b/flow/include/flow/CodeProbe.h @@ -64,15 +64,15 @@ operator|(Left const& lhs, Right const& rhs) { namespace assert { struct NoSim { constexpr static AnnotationType type = AnnotationType::Assertion; - bool operator()(ICodeProbe* self) const; + bool operator()(ICodeProbe const* self) const; }; struct SimOnly { constexpr static AnnotationType type = AnnotationType::Assertion; - bool operator()(ICodeProbe* self) const; + bool operator()(ICodeProbe const* self) const; }; struct RocksDB { constexpr static AnnotationType type = AnnotationType::Assertion; - bool operator()(ICodeProbe* self) const; + bool operator()(ICodeProbe const* self) const; }; template @@ -140,6 +140,7 @@ struct CodeProbeAnnotations<> { constexpr bool expectContext(ExecutionContext context, bool prevHadSomeContext = false) const { return !prevHadSomeContext; } + constexpr bool shouldTrace(ICodeProbe const*) const { return true; } constexpr bool deduplicate() const { return false; } }; @@ -161,6 +162,16 @@ struct CodeProbeAnnotations { } tail.hit(self); } + + bool shouldTrace(ICodeProbe const* self) const { + if constexpr (Head::type == AnnotationType::Assertion) { + if (!head(self)) { + return false; + } + } + return tail.shouldTrace(self); + } + void trace(const ICodeProbe* self, BaseTraceEvent& evt, bool condition) const { if constexpr (Head::type == AnnotationType::Decoration) { head.trace(self, evt, condition); @@ -204,6 +215,7 @@ struct ICodeProbe { virtual const char* condition() const = 0; virtual const char* compilationUnit() const = 0; virtual void trace(bool) const = 0; + virtual bool shouldTrace() const = 0; virtual bool wasHit() const = 0; virtual unsigned hitCount() const = 0; virtual bool expectInContext(ExecutionContext context) const = 0; @@ -237,6 +249,9 @@ struct CodeProbeImpl : ICodeProbe { .detail("Comment", Comment::value()); annotations.trace(this, evt, condition); } + + bool shouldTrace() const override { return annotations.shouldTrace(this); } + bool wasHit() const override { return _hitCount > 0; } unsigned hitCount() const override { return _hitCount; } From c503b2dc53c1af3ab1f4bb8334aebae33515ef6a Mon Sep 17 00:00:00 2001 From: Markus Pilman Date: Mon, 26 Sep 2022 15:03:35 -0600 Subject: [PATCH 3/3] fix probe::assert::RocksDB --- fdbserver/CMakeLists.txt | 3 +-- fdbserver/SimulatedCluster.actor.cpp | 25 +++++++++++++++++++++++-- flow/CodeProbe.cpp | 8 -------- flow/include/flow/CodeProbe.h | 5 ----- 4 files changed, 24 insertions(+), 17 deletions(-) diff --git a/fdbserver/CMakeLists.txt b/fdbserver/CMakeLists.txt index 4386b84b5e..ddc6089855 100644 --- a/fdbserver/CMakeLists.txt +++ b/fdbserver/CMakeLists.txt @@ -7,8 +7,6 @@ else() endif() if (WITH_ROCKSDB_EXPERIMENTAL) - add_definitions(-DSSD_ROCKSDB_EXPERIMENTAL) - include(CompileRocksDB) # CompileRocksDB sets `lz4_LIBRARIES` to be the shared lib, we want to link # statically, so find the static library here. @@ -44,6 +42,7 @@ else() endif() target_link_libraries(fdbserver PRIVATE toml11_target jemalloc rapidjson) +target_compile_definitions(fdbserver PRIVATE SSD_ROCKSDB_EXPERIMENTAL) # target_compile_definitions(fdbserver PRIVATE -DENABLE_SAMPLING) if(GPERFTOOLS_FOUND) diff --git a/fdbserver/SimulatedCluster.actor.cpp b/fdbserver/SimulatedCluster.actor.cpp index c223f22c0d..4763bc28a8 100644 --- a/fdbserver/SimulatedCluster.actor.cpp +++ b/fdbserver/SimulatedCluster.actor.cpp @@ -59,6 +59,27 @@ extern const char* getSourceVersion(); using namespace std::literals; +namespace probe { + +namespace assert { + +struct HasRocksDB { + constexpr static AnnotationType type = AnnotationType::Assertion; + bool operator()(ICodeProbe const* self) const { +#ifdef SSD_ROCKSDB_EXPERIMENTAL + return true; +#else + return false; +#endif + } +}; + +constexpr HasRocksDB hasRocksDB; + +} // namespace assert + +} // namespace probe + // TODO: Defining these here is just asking for ODR violations. template <> std::string describe(bool const& val) { @@ -1461,7 +1482,7 @@ void SimulationConfig::setStorageEngine(const TestConfig& testConfig) { break; } case 4: { - CODE_PROBE(true, "Simulated cluster using RocksDB storage engine", probe::assert::rocksDB); + CODE_PROBE(true, "Simulated cluster using RocksDB storage engine", probe::assert::hasRocksDB); set_config("ssd-rocksdb-v1"); // Tests using the RocksDB engine are necessarily non-deterministic because of RocksDB // background threads. @@ -1471,7 +1492,7 @@ void SimulationConfig::setStorageEngine(const TestConfig& testConfig) { break; } case 5: { - CODE_PROBE(true, "Simulated cluster using Sharded RocksDB storage engine", probe::assert::rocksDB); + CODE_PROBE(true, "Simulated cluster using Sharded RocksDB storage engine", probe::assert::hasRocksDB); set_config("ssd-sharded-rocksdb"); // Tests using the RocksDB engine are necessarily non-deterministic because of RocksDB // background threads. diff --git a/flow/CodeProbe.cpp b/flow/CodeProbe.cpp index 38961acbd6..0a4d391cd9 100644 --- a/flow/CodeProbe.cpp +++ b/flow/CodeProbe.cpp @@ -288,14 +288,6 @@ bool SimOnly::operator()(ICodeProbe const* self) const { return g_network->isSimulated(); } -bool RocksDB::operator()(ICodeProbe const* self) const { -#ifdef SSD_ROCKSDB_EXPERIMENTAL - return true; -#else - return false; -#endif -} - } // namespace assert } // namespace probe diff --git a/flow/include/flow/CodeProbe.h b/flow/include/flow/CodeProbe.h index e4c4e3ea38..b39e4221da 100644 --- a/flow/include/flow/CodeProbe.h +++ b/flow/include/flow/CodeProbe.h @@ -70,10 +70,6 @@ struct SimOnly { constexpr static AnnotationType type = AnnotationType::Assertion; bool operator()(ICodeProbe const* self) const; }; -struct RocksDB { - constexpr static AnnotationType type = AnnotationType::Assertion; - bool operator()(ICodeProbe const* self) const; -}; template struct AssertOr { @@ -115,7 +111,6 @@ constexpr std::enable_if_t> o constexpr SimOnly simOnly; constexpr auto noSim = !simOnly; -constexpr RocksDB rocksDB; } // namespace assert