chore: add hdr_histogram to the project (#5330)

* chore: add hdr_histogram to the project

Mostly a boilerplate PR, should not change functionality.

1. reduce number of absl::GetCurrentTimeNanos() calls by one during the command execution.
2. add hdr_histogram object to CommandId but do not yet record anything into it.

First part to address #5092
---------

Signed-off-by: Roman Gershman <[email protected]>

Author: romange

src/CMakeLists.txt

@@ -125,6 +125,17 @@ add_third_party(
                     -DFLATBUFFERS_BUILD_FLATC=OFF"
 )
 
+add_third_party(
+  hdr_histogram
+  GIT_REPOSITORY https://github.com/HdrHistogram/HdrHistogram_c/
+  GIT_TAG 652d51bcc36744fd1a6debfeb1a8a5f58b14022c
+  GIT_SHALLOW 1
+  CMAKE_PASS_FLAGS "-DHDR_LOG_REQUIRED=OFF -DHDR_HISTOGRAM_BUILD_PROGRAMS=OFF
+                    -DHDR_HISTOGRAM_INSTALL_SHARED=OFF"
+  LIB libhdr_histogram_static.a
+)
+
+
 add_library(TRDP::jsoncons INTERFACE IMPORTED)
 add_dependencies(TRDP::jsoncons jsoncons_project)
 set_target_properties(TRDP::jsoncons PROPERTIES

src/server/CMakeLists.txt

@@ -82,7 +82,7 @@ if (WITH_AWS)
   add_definitions(-DWITH_AWS)
 endif()
 
-cxx_link(dfly_transaction dfly_core strings_lib TRDP::fast_float)
+cxx_link(dfly_transaction dfly_core strings_lib TRDP::fast_float TRDP::hdr_histogram)
 cxx_link(dragonfly_lib dfly_transaction dfly_facade redis_lib ${AWS_LIB} jsonpath
          strings_lib html_lib gcp_lib azure_lib
          http_client_lib absl::random_random TRDP::jsoncons TRDP::zstd TRDP::lz4

src/server/acl/acl_family.cc

@@ -1186,12 +1186,13 @@ std::variant<User::UpdateRequest, ErrorReply> AclFamily::ParseAclSetUser(
 }
 
 void AclFamily::BuildIndexers(RevCommandsIndexStore families) {
-  acl::NumberOfFamilies(families.size());
+  size_t family_count = acl::NumberOfFamilies(families.size());
   CommandsRevIndexer(std::move(families));
   CategoryToCommandsIndexStore index;
   cmd_registry_->Traverse([&](std::string_view, auto& cid) {
     const uint32_t cat = cid.acl_categories();
     const size_t family = cid.GetFamily();
+    DCHECK_LT(family, family_count);
     const uint64_t bit_index = cid.GetBitIndex();
     for (size_t i = 0; i < 32; ++i) {
       if (cat & 1 << i) {

src/server/command_registry.cc

@@ -121,13 +121,27 @@ CmdLineMapping OriginalToAliasMap() {
   return original_to_alias;
 }
 
+constexpr int64_t kLatencyHistogramMinValue = 1;        // Minimum value in usec
+constexpr int64_t kLatencyHistogramMaxValue = 1000000;  // Maximum value in usec (1s)
+constexpr int32_t kLatencyHistogramPrecision = 2;
+
 }  // namespace
 
 CommandId::CommandId(const char* name, uint32_t mask, int8_t arity, int8_t first_key,
                      int8_t last_key, std::optional<uint32_t> acl_categories)
     : facade::CommandId(name, ImplicitCategories(mask), arity, first_key, last_key,
                         acl_categories.value_or(ImplicitAclCategories(mask))) {
   implicit_acl_ = !acl_categories.has_value();
+  hdr_histogram* hist = nullptr;
+  hdr_init(kLatencyHistogramMinValue, kLatencyHistogramMaxValue, kLatencyHistogramPrecision, &hist);
+  latency_histogram_ = hist;
+}
+
+CommandId::~CommandId() {
+  // Aliases share the same latency histogram, so we only close it if this is not an alias.
+  if (latency_histogram_ && !is_alias_) {
+    hdr_close(latency_histogram_);
+  }
 }
 
 CommandId CommandId::Clone(const std::string_view name) const {
@@ -138,6 +152,11 @@ CommandId CommandId::Clone(const std::string_view name) const {
   cloned.acl_categories_ = acl_categories_;
   cloned.implicit_acl_ = implicit_acl_;
   cloned.is_alias_ = true;
+
+  // explicit sharing of the object since it's an alias we can do that.
+  // I am assuming that the source object lifetime is at least as of the cloned object.
+  hdr_close(cloned.latency_histogram_);  // Free the histogram in the cloned object.
+  cloned.latency_histogram_ = static_cast<hdr_histogram*>(latency_histogram_);
   return cloned;
 }
 
@@ -157,7 +176,8 @@ bool CommandId::IsMultiTransactional() const {
 }
 
 uint64_t CommandId::Invoke(CmdArgList args, const CommandContext& cmd_cntx) const {
-  int64_t before = absl::GetCurrentTimeNanos();
+  uint64_t before = cmd_cntx.conn_cntx->conn_state.cmd_start_time_ns;
+  DCHECK_GT(before, 0u);
   handler_(args, cmd_cntx);
   int64_t after = absl::GetCurrentTimeNanos();
 

src/server/command_registry.h

@@ -7,6 +7,7 @@
 #include <absl/container/flat_hash_map.h>
 #include <absl/container/flat_hash_set.h>
 #include <absl/types/span.h>
+#include <hdr/hdr_histogram.h>
 
 #include <functional>
 #include <optional>
@@ -84,14 +85,41 @@ struct CommandContext {
   ConnectionContext* conn_cntx;
 };
 
+// TODO: move it to helio
+// Makes sure that the POD T that is passed to the constructor is reset to default state
+template <typename T> class MoveOnly {
+ public:
+  MoveOnly() = default;
+
+  MoveOnly(const MoveOnly&) = delete;
+  MoveOnly& operator=(const MoveOnly&) = delete;
+
+  MoveOnly(MoveOnly&& t) noexcept : value_(std::move(t.value_)) {
+    t.value_ = T{};  // Reset the passed value to default state
+  }
+
+  MoveOnly& operator=(const T& t) noexcept {
+    value_ = t;
+    return *this;
+  }
+
+  operator const T&() const {  // NOLINT
+    return value_;
+  }
+
+ private:
+  T value_;
+};
+
 class CommandId : public facade::CommandId {
  public:
   // NOTICE: name must be a literal string, otherwise metrics break! (see cmd_stats_map in
   // server_state.h)
   CommandId(const char* name, uint32_t mask, int8_t arity, int8_t first_key, int8_t last_key,
             std::optional<uint32_t> acl_categories = std::nullopt);
+  CommandId(CommandId&& o) = default;
 
-  CommandId(CommandId&&) = default;
+  ~CommandId();
 
   [[nodiscard]] CommandId Clone(std::string_view name) const;
 
@@ -160,11 +188,13 @@ class CommandId : public facade::CommandId {
   }
 
  private:
+  // The following fields must copy manually in the move constructor.
   bool implicit_acl_;
+  bool is_alias_{false};
   std::unique_ptr<CmdCallStats[]> command_stats_;
   Handler3 handler_;
   ArgValidator validator_;
-  bool is_alias_{false};
+  MoveOnly<hdr_histogram*> latency_histogram_;  // Histogram for command latency in usec
 };
 
 class CommandRegistry {

src/server/conn_context.h

@@ -147,7 +147,7 @@ struct ConnectionState {
     const ConnectionContext* owner = nullptr;
   };
 
-  enum MCGetMask { FETCH_CAS_VER = 1 };
+  enum MCGetMask : uint8_t { FETCH_CAS_VER = 1 };
 
   size_t UsedMemory() const;
 
@@ -265,6 +265,7 @@ struct ConnectionState {
   std::unique_ptr<ScriptInfo> script_info;
   std::unique_ptr<SubscribeInfo> subscribe_info;
   ClientTracking tracking_info_;
+  uint64_t cmd_start_time_ns = 0;  // time when the last command started executing
 };
 
 class ConnectionContext : public facade::ConnectionContext {

src/server/main_service.cc

@@ -1044,11 +1044,10 @@ static optional<ErrorReply> VerifyConnectionAclStatus(const CommandId* cid,
   return nullopt;
 }
 
-bool ShouldDenyOnOOM(const CommandId* cid) {
+bool ShouldDenyOnOOM(const CommandId* cid, uint64_t curr_time_ns) {
   ServerState& etl = *ServerState::tlocal();
   if ((cid->opt_mask() & CO::DENYOOM) && etl.is_master) {
-    uint64_t start_ns = absl::GetCurrentTimeNanos();
-    auto memory_stats = etl.GetMemoryUsage(start_ns);
+    auto memory_stats = etl.GetMemoryUsage(curr_time_ns);
 
     if (memory_stats.used_mem > max_memory_limit ||
         (etl.rss_oom_deny_ratio > 0 &&
@@ -1062,14 +1061,14 @@ bool ShouldDenyOnOOM(const CommandId* cid) {
   return false;
 }
 
-optional<ErrorReply> Service::VerifyCommandExecution(const CommandId* cid,
-                                                     const ConnectionContext* cntx,
+optional<ErrorReply> Service::VerifyCommandExecution(const ConnectionContext* cntx,
                                                      CmdArgList tail_args) {
-  if (ShouldDenyOnOOM(cid)) {
+  DCHECK(cntx->conn_state.cmd_start_time_ns);
+  if (ShouldDenyOnOOM(cntx->cid, cntx->conn_state.cmd_start_time_ns)) {
     return facade::ErrorReply{OpStatus::OUT_OF_MEMORY};
   }
 
-  return VerifyConnectionAclStatus(cid, cntx, "ACL rules changed between the MULTI and EXEC",
+  return VerifyConnectionAclStatus(cntx->cid, cntx, "ACL rules changed between the MULTI and EXEC",
                                    tail_args);
 }
 
@@ -1361,11 +1360,12 @@ DispatchResult Service::InvokeCmd(const CommandId* cid, CmdArgList tail_args,
 
   ConnectionContext* cntx = cmd_cntx.conn_cntx;
   cntx->cid = cid;
+  cntx->conn_state.cmd_start_time_ns = absl::GetCurrentTimeNanos();
   auto* builder = cmd_cntx.rb;
   DCHECK(builder);
   DCHECK(cntx);
 
-  if (auto err = VerifyCommandExecution(cid, cntx, tail_args); err) {
+  if (auto err = VerifyCommandExecution(cntx, tail_args); err) {
     // We need to skip this because ACK's should not be replied to
     // Bonus points because this allows to continue replication with ACL users who got
     // their access revoked and reinstated

src/server/main_service.h

@@ -49,8 +49,7 @@ class Service : public facade::ServiceInterface {
 
   // Verify command can be executed now (check out of memory), always called immediately before
   // execution
-  std::optional<facade::ErrorReply> VerifyCommandExecution(const CommandId* cid,
-                                                           const ConnectionContext* cntx,
+  std::optional<facade::ErrorReply> VerifyCommandExecution(const ConnectionContext* cntx,
                                                            CmdArgList tail_args);
 
   // Verify command prepares excution in correct state.