[#25602] DocDB: Use ScopedStatistics during write operations

Summary: `StatisticsMetricImpl` performance simnifically degraded after https://phorge.dev.yugabyte.com/D39667. Good news that we use `ScopedStatistics` during read and merge it once with `StatisticsMetricImpl` during read operation. Could do the same to write operations. Performance comparison, PgSingleTServerTest.Scan TServer insert time decreased from 12.96s to 12.53s. Also did the following small improvements: Removed unnecessary includes of "yb/docdb/docdb_statistics.h", it helped to use `ScopedStatistics` directly in `DocDBStatistics` instead of std::unique_ptr. Fixed VLOG in ScopedStats, that were collecting dump even when VLOG is off. Pass operation statistics to `NonBlockBasedFilterKeyMayMatch`, so it works with correct statistics object. Simplified `FullFilterBitsReader` code. Jira: DB-14850 Test Plan: Jenkins Reviewers: esheng Reviewed By: esheng Subscribers: ybase, yql Tags: #jenkins-ready Differential Revision: https://phorge.dev.yugabyte.com/D41179
yugabyte · Jan 14, 2025 · 3724517 · 3724517
1 parent 25a62a8
commit 3724517
Show file tree

Hide file tree

Showing 33 changed files with 204 additions and 254 deletions.
diff --git a/src/yb/docdb/cql_operation.h b/src/yb/docdb/cql_operation.h
@@ -19,7 +19,6 @@
 #include "yb/docdb/doc_expr.h"
 #include "yb/dockv/doc_key.h"
 #include "yb/docdb/doc_operation.h"
-#include "yb/docdb/docdb_statistics.h"
 #include "yb/docdb/intent_aware_iterator.h"
 
 #include "yb/util/operation_counter.h"

diff --git a/src/yb/docdb/doc_rowwise_iterator.h b/src/yb/docdb/doc_rowwise_iterator.h
@@ -27,7 +27,6 @@
 #include "yb/docdb/doc_read_context.h"
 #include "yb/docdb/doc_reader.h"
 #include "yb/docdb/doc_rowwise_iterator_base.h"
-#include "yb/docdb/docdb_statistics.h"
 #include "yb/docdb/intent_aware_iterator.h"
 #include "yb/docdb/key_bounds.h"
 #include "yb/docdb/ql_rowwise_iterator_interface.h"

diff --git a/src/yb/docdb/docdb_rocksdb_util.cc b/src/yb/docdb/docdb_rocksdb_util.cc
@@ -27,6 +27,7 @@
 #include "yb/docdb/consensus_frontier.h"
 #include "yb/docdb/doc_ql_filefilter.h"
 #include "yb/docdb/docdb_filter_policy.h"
+#include "yb/docdb/docdb_statistics.h"
 #include "yb/docdb/intent_aware_iterator.h"
 #include "yb/docdb/key_bounds.h"
 #include "yb/docdb/read_operation_data.h"
@@ -284,7 +285,7 @@ rocksdb::ReadOptions PrepareReadOptions(
   return read_opts;
 }
 
-rocksdb::Statistics* GetRegularDBStatistics(const DocDBStatistics* statistics) {
+rocksdb::Statistics* GetRegularDBStatistics(DocDBStatistics* statistics) {
   return statistics ? statistics->RegularDBStatistics() : nullptr;
 }
 

diff --git a/src/yb/docdb/docdb_rocksdb_util.h b/src/yb/docdb/docdb_rocksdb_util.h
@@ -17,7 +17,6 @@
 
 #include "yb/docdb/bounded_rocksdb_iterator.h"
 #include "yb/docdb/docdb_fwd.h"
-#include "yb/docdb/docdb_statistics.h"
 
 #include "yb/rocksdb/cache.h"
 #include "yb/rocksdb/db.h"

diff --git a/src/yb/docdb/docdb_statistics.cc b/src/yb/docdb/docdb_statistics.cc
@@ -379,46 +379,40 @@ size_t DumpRocksDBStatistics(
 
 } // namespace
 
-DocDBStatistics::DocDBStatistics():
-    regulardb_statistics_(std::make_unique<rocksdb::ScopedStatistics>()),
-    intentsdb_statistics_(std::make_unique<rocksdb::ScopedStatistics>()) {}
-
-DocDBStatistics::~DocDBStatistics() {}
-
-rocksdb::Statistics* DocDBStatistics::RegularDBStatistics() const {
-  return regulardb_statistics_.get();
+rocksdb::Statistics* DocDBStatistics::RegularDBStatistics() {
+  return &regulardb_statistics_;
 }
 
-rocksdb::Statistics* DocDBStatistics::IntentsDBStatistics() const {
-  return intentsdb_statistics_.get();
+rocksdb::Statistics* DocDBStatistics::IntentsDBStatistics() {
+  return &intentsdb_statistics_;
 }
 
 void DocDBStatistics::MergeAndClear(
     rocksdb::Statistics* regulardb_statistics,
     rocksdb::Statistics* intentsdb_statistics) {
-  regulardb_statistics_->MergeAndClear(regulardb_statistics);
-  intentsdb_statistics_->MergeAndClear(intentsdb_statistics);
+  regulardb_statistics_.MergeAndClear(regulardb_statistics);
+  intentsdb_statistics_.MergeAndClear(intentsdb_statistics);
 }
 
 size_t DocDBStatistics::Dump(std::stringstream* out) const {
   size_t dumped = 0;
   dumped += DumpRocksDBStatistics(
-      *regulardb_statistics_, std::span{kRegularDBTickers}, std::span{kRegularDBEventStats},
+      regulardb_statistics_, std::span{kRegularDBTickers}, std::span{kRegularDBEventStats},
       "" /* name_prefix */, out);
   dumped += DumpRocksDBStatistics(
-      *intentsdb_statistics_, std::span{kIntentsDBTickers}, std::span{kIntentsDBEventStats},
+      intentsdb_statistics_, std::span{kIntentsDBTickers}, std::span{kIntentsDBEventStats},
       "intentsdb_" /* name_prefix */, out);
   return dumped;
 }
 
 void DocDBStatistics::CopyToPgsqlResponse(PgsqlResponsePB* response) const {
   auto* metrics = response->mutable_metrics();
   CopyRocksDBStatisticsToPgsqlResponse(
-      *regulardb_statistics_, std::span{kRegularDBTickers}, std::span{kRegularDBEventStats},
+      regulardb_statistics_, std::span{kRegularDBTickers}, std::span{kRegularDBEventStats},
       metrics);
   if (GetAtomicFlag(&FLAGS_ysql_analyze_dump_intentsdb_metrics)) {
     CopyRocksDBStatisticsToPgsqlResponse(
-        *intentsdb_statistics_, std::span{kIntentsDBTickers}, std::span{kIntentsDBEventStats},
+        intentsdb_statistics_, std::span{kIntentsDBTickers}, std::span{kIntentsDBEventStats},
         metrics);
   }
 }

diff --git a/src/yb/docdb/docdb_statistics.h b/src/yb/docdb/docdb_statistics.h
@@ -15,12 +15,7 @@
 
 #include <memory>
 
-namespace rocksdb {
-
-class Statistics;
-class ScopedStatistics;
-
-} // namespace rocksdb
+#include "yb/rocksdb/util/statistics.h"
 
 namespace yb {
 
@@ -30,11 +25,8 @@ namespace docdb {
 
 class DocDBStatistics {
  public:
-  DocDBStatistics();
-  ~DocDBStatistics();
-
-  rocksdb::Statistics* RegularDBStatistics() const;
-  rocksdb::Statistics* IntentsDBStatistics() const;
+  rocksdb::Statistics* RegularDBStatistics();
+  rocksdb::Statistics* IntentsDBStatistics();
 
   void MergeAndClear(
       rocksdb::Statistics* regulardb_statistics,
@@ -46,8 +38,8 @@ class DocDBStatistics {
   void CopyToPgsqlResponse(PgsqlResponsePB* response) const;
 
  private:
-  std::unique_ptr<rocksdb::ScopedStatistics> regulardb_statistics_;
-  std::unique_ptr<rocksdb::ScopedStatistics> intentsdb_statistics_;
+  rocksdb::ScopedStatistics regulardb_statistics_;
+  rocksdb::ScopedStatistics intentsdb_statistics_;
 };
 
 } // namespace docdb

diff --git a/src/yb/docdb/intent_aware_iterator.cc b/src/yb/docdb/intent_aware_iterator.cc
@@ -176,7 +176,7 @@ inline bool IsKeyOrderedBefore(Slice key, Slice other_key) {
   }
 }
 
-rocksdb::Statistics* GetIntentsDBStatistics(const DocDBStatistics* statistics) {
+rocksdb::Statistics* GetIntentsDBStatistics(DocDBStatistics* statistics) {
   return statistics ? statistics->IntentsDBStatistics() : nullptr;
 }
 

diff --git a/src/yb/docdb/pgsql_operation.cc b/src/yb/docdb/pgsql_operation.cc
@@ -41,6 +41,7 @@
 #include "yb/docdb/docdb_debug.h"
 #include "yb/docdb/docdb_pgapi.h"
 #include "yb/docdb/docdb_rocksdb_util.h"
+#include "yb/docdb/docdb_statistics.h"
 #include "yb/docdb/intent_aware_iterator.h"
 #include "yb/docdb/ql_storage_interface.h"
 #include "yb/docdb/vector_index.h"

diff --git a/src/yb/docdb/pgsql_operation.h b/src/yb/docdb/pgsql_operation.h
@@ -23,7 +23,6 @@
 #include "yb/docdb/doc_expr.h"
 #include "yb/docdb/doc_operation.h"
 #include "yb/docdb/doc_read_context.h"
-#include "yb/docdb/docdb_statistics.h"
 #include "yb/docdb/intent_aware_iterator.h"
 #include "yb/docdb/ql_rowwise_iterator_interface.h"
 

diff --git a/src/yb/docdb/ql_rocksdb_storage.cc b/src/yb/docdb/ql_rocksdb_storage.cc
@@ -26,6 +26,7 @@
 #include "yb/docdb/doc_read_context.h"
 #include "yb/docdb/doc_rowwise_iterator.h"
 #include "yb/docdb/doc_ql_scanspec.h"
+#include "yb/docdb/docdb_statistics.h"
 
 #include "yb/rocksdb/util/statistics.h"
 
@@ -664,9 +665,11 @@ Result<YQLStorageIf::SampleBlocksData> QLRocksDBStorage::GetSampleBlocks(
     ScopedStats(const DocDB& doc_db_, int vlog_level_) : doc_db(doc_db_), vlog_level(vlog_level_) {}
 
     ~ScopedStats() {
-      std::stringstream ss;
-      docdb_stats.Dump(&ss);
-      VLOG(vlog_level) << "DocDB stats:\n" << ss.str();
+      if (VLOG_IS_ON(vlog_level)) {
+        std::stringstream ss;
+        docdb_stats.Dump(&ss);
+        VLOG(vlog_level) << "DocDB stats:\n" << ss.str();
+      }
       docdb_stats.MergeAndClear(
           doc_db.regular->GetOptions().statistics.get(),
           doc_db.intents->GetOptions().statistics.get());

diff --git a/src/yb/docdb/ql_storage_interface.h b/src/yb/docdb/ql_storage_interface.h
@@ -23,7 +23,6 @@
 #include "yb/common/read_hybrid_time.h"
 
 #include "yb/docdb/docdb_fwd.h"
-#include "yb/docdb/docdb_statistics.h"
 #include "yb/docdb/ql_rowwise_iterator_interface.h"
 
 #include "yb/util/kv_util.h"

diff --git a/src/yb/docdb/read_operation_data.h b/src/yb/docdb/read_operation_data.h
@@ -25,7 +25,7 @@ namespace yb::docdb {
 struct ReadOperationData {
   CoarseTimePoint deadline = CoarseTimePoint::max();
   ReadHybridTime read_time = ReadHybridTime::Max();
-  const DocDBStatistics* statistics = nullptr;
+  DocDBStatistics* statistics = nullptr;
 
   std::string ToString() const {
     return YB_STRUCT_TO_STRING(deadline, read_time);
@@ -37,7 +37,7 @@ struct ReadOperationData {
     return result;
   }
 
-  ReadOperationData WithStatistics(const DocDBStatistics* statistics_) const {
+  ReadOperationData WithStatistics(DocDBStatistics* statistics_) const {
     auto result = *this;
     result.statistics = statistics_;
     return result;

diff --git a/src/yb/docdb/redis_operation.cc b/src/yb/docdb/redis_operation.cc
@@ -28,6 +28,7 @@
 #include "yb/server/hybrid_clock.h"
 
 #include "yb/util/redis_util.h"
+#include "yb/util/scope_exit.h"
 #include "yb/util/status_format.h"
 #include "yb/util/stol_utils.h"
 #include "yb/util/flags.h"
@@ -557,6 +558,9 @@ void RedisWriteOperation::InitializeIterator(const DocOperationApplyData& data)
 }
 
 Status RedisWriteOperation::Apply(const DocOperationApplyData& data) {
+  auto se = ScopeExit([this] {
+    iterator_.reset();
+  });
   switch (request_.request_case()) {
     case RedisWriteRequestPB::kSetRequest:
       return ApplySet(data);

diff --git a/src/yb/rocksdb/filter_policy.h b/src/yb/rocksdb/filter_policy.h
@@ -67,7 +67,7 @@ class FilterBitsReader {
   virtual ~FilterBitsReader() {}
 
   // Check if the entry match the bits in filter
-  virtual bool MayMatch(const Slice& entry) = 0;
+  virtual bool MayMatch(Slice entry) = 0;
 };
 
 // We add a new format of filter block called full filter block

diff --git a/src/yb/rocksdb/statistics.h b/src/yb/rocksdb/statistics.h
@@ -285,7 +285,7 @@ class Statistics {
   virtual void histogramData(uint32_t type,
                              HistogramData* const data) const = 0;
   virtual const yb::AggregateStats& getAggregateStats(uint32_t histogramType) const = 0;
-  virtual void recordTick(uint32_t tickerType, uint64_t count = 0) = 0;
+  virtual void recordTick(uint32_t tickerType, uint64_t count = 1) = 0;
   virtual void setTickerCount(uint32_t tickerType, uint64_t count) = 0;
   virtual void measureTime(uint32_t histogramType, uint64_t time) = 0;
   virtual void addHistogram(uint32_t histogramType, const yb::AggregateStats& stats) = 0;

diff --git a/src/yb/rocksdb/table/block_based_filter_block.cc b/src/yb/rocksdb/table/block_based_filter_block.cc
@@ -196,8 +196,7 @@ BlockBasedFilterBlockReader::BlockBasedFilterBlockReader(
   num_ = (n - 5 - last_word) / 4;
 }
 
-bool BlockBasedFilterBlockReader::KeyMayMatch(const Slice& key,
-                                              uint64_t block_offset) {
+bool BlockBasedFilterBlockReader::KeyMayMatch(Slice key, uint64_t block_offset) {
   assert(block_offset != kNotValid);
   if (!whole_key_filtering_) {
     return true;
@@ -214,8 +213,7 @@ bool BlockBasedFilterBlockReader::PrefixMayMatch(const Slice& prefix,
   return MayMatch(prefix, block_offset);
 }
 
-bool BlockBasedFilterBlockReader::MayMatch(const Slice& entry,
-                                           uint64_t block_offset) {
+bool BlockBasedFilterBlockReader::MayMatch(Slice entry, uint64_t block_offset) {
   uint64_t index = block_offset >> base_lg_;
   if (index < num_) {
     uint32_t start = DecodeFixed32(offset_ + index * 4);

diff --git a/src/yb/rocksdb/table/block_based_filter_block.h b/src/yb/rocksdb/table/block_based_filter_block.h
@@ -97,11 +97,9 @@ class BlockBasedFilterBlockReader : public FilterBlockReader {
                               const BlockBasedTableOptions& table_opt,
                               bool whole_key_filtering,
                               BlockContents&& contents);
-  virtual bool KeyMayMatch(const Slice& key,
-                           uint64_t block_offset = kNotValid) override;
-  virtual bool PrefixMayMatch(const Slice& prefix,
-                              uint64_t block_offset = kNotValid) override;
-  virtual size_t ApproximateMemoryUsage() const override;
+  bool KeyMayMatch(Slice key, uint64_t block_offset = kNotValid) override;
+  bool PrefixMayMatch(const Slice& prefix, uint64_t block_offset = kNotValid) override;
+  size_t ApproximateMemoryUsage() const override;
 
   // convert this object to a human readable form
   std::string ToString() const override;
@@ -116,7 +114,7 @@ class BlockBasedFilterBlockReader : public FilterBlockReader {
   size_t base_lg_;      // Encoding parameter (see kFilterBaseLg in .cc file)
   BlockContents contents_;
 
-  bool MayMatch(const Slice& entry, uint64_t block_offset);
+  bool MayMatch(Slice entry, uint64_t block_offset);
 
   // No copying allowed
   BlockBasedFilterBlockReader(const BlockBasedFilterBlockReader&);
-Original file line number
+Diff line change
@@ Expand Up @@
       }
     }
-    rocksdb::Statistics* GetIntentsDBStatistics(const DocDBStatistics* statistics) {
+    rocksdb::Statistics* GetIntentsDBStatistics(DocDBStatistics* statistics) {
       return statistics ? statistics->IntentsDBStatistics() : nullptr;
     }
@@ Expand Down @@