micro-optimizations for cache performance (#19132)

Co-authored-by: Manuel Pöter <manuel@arangodb.com>
arangodb · Jun 2, 2023 · 8ca5e2b · 8ca5e2b
1 parent c7d6154
commit 8ca5e2b
Show file tree

Hide file tree

Showing 22 changed files with 433 additions and 372 deletions.
diff --git a/arangod/Cache/BucketState.cpp b/arangod/Cache/BucketState.cpp
@@ -45,18 +45,19 @@ BucketState& BucketState::operator=(BucketState const& other) noexcept {
 }
 
 bool BucketState::isLocked() const noexcept {
-  return ((_state.load() & static_cast<std::uint32_t>(Flag::locked)) > 0);
+  return (_state.load() & static_cast<FlagType>(Flag::locked)) != 0;
 }
 
 bool BucketState::lock(std::uint64_t maxTries) noexcept {
-  uint64_t attempt = 0;
-  while (attempt < maxTries) {
+  uint64_t attempts = 0;
+  do {
     // expect unlocked, but need to preserve migrating status
-    std::uint32_t current = _state.load(std::memory_order_relaxed);
-    std::uint32_t expected =
-        current & (~static_cast<std::uint32_t>(Flag::locked));
+    FlagType current = _state.load(std::memory_order_relaxed);
+    FlagType expected =
+        static_cast<FlagType>(current & (~static_cast<FlagType>(Flag::locked)));
     if (current == expected) {
-      uint32_t desired = expected | static_cast<std::uint32_t>(Flag::locked);
+      FlagType desired =
+          static_cast<FlagType>(expected | static_cast<FlagType>(Flag::locked));
       // try to lock
       bool success = _state.compare_exchange_strong(expected, desired,
                                                     std::memory_order_acq_rel,
@@ -65,39 +66,32 @@ bool BucketState::lock(std::uint64_t maxTries) noexcept {
         return true;
       }
     }
-    attempt++;
     basics::cpu_relax();
     // TODO: exponential back-off for failure?
-  }
+  } while (++attempts < maxTries);
 
   return false;
 }
 
 void BucketState::unlock() noexcept {
   TRI_ASSERT(isLocked());
-  _state.fetch_and(~static_cast<std::uint32_t>(Flag::locked),
+  _state.fetch_and(static_cast<FlagType>(~static_cast<FlagType>(Flag::locked)),
                    std::memory_order_release);
 }
 
 bool BucketState::isSet(BucketState::Flag flag) const noexcept {
   TRI_ASSERT(isLocked());
-  return ((_state.load() & static_cast<std::uint32_t>(flag)) > 0);
-}
-
-bool BucketState::isSet(BucketState::Flag flag1,
-                        BucketState::Flag flag2) const noexcept {
-  TRI_ASSERT(isLocked());
-  return ((_state.load() & (static_cast<std::uint32_t>(flag1) |
-                            static_cast<std::uint32_t>(flag2))) > 0);
+  return static_cast<FlagType>(_state.load() & static_cast<FlagType>(flag)) !=
+         0;
 }
 
 void BucketState::toggleFlag(BucketState::Flag flag) noexcept {
   TRI_ASSERT(isLocked());
-  _state ^= static_cast<std::uint32_t>(flag);
+  _state ^= static_cast<FlagType>(flag);
 }
 
 void BucketState::clear() noexcept {
   TRI_ASSERT(isLocked());
-  _state = static_cast<std::uint32_t>(Flag::locked);
+  _state = static_cast<FlagType>(Flag::locked);
 }
 }  // namespace arangodb::cache
diff --git a/arangod/Cache/BucketState.h b/arangod/Cache/BucketState.h
@@ -29,13 +29,12 @@
 #include <limits>
 #include <type_traits>
 
-namespace arangodb {
-namespace cache {
+namespace arangodb::cache {
 
 ////////////////////////////////////////////////////////////////////////////////
 /// @brief Simple state class with a small footprint.
 ///
-/// Underlying store is simply a std::atomic<uint32_t>, and each bit corresponds
+/// Underlying store is simply a std::atomic<uint16_t>, and each bit corresponds
 /// to a flag that can be set. The lowest bit is special and is designated as
 /// the locking flag. Any access (read or modify) to the state must occur when
 /// the state is already locked; the two exceptions are to check whether the
@@ -47,25 +46,20 @@ struct BucketState {
   //////////////////////////////////////////////////////////////////////////////
   /// @brief Flags which can be queried or toggled to reflect state.
   ///
-  /// Each flag must have exactly one bit set, fit in uint32_t. The 'locked'
+  /// Each flag must have exactly one bit set, fit in uint16_t. The 'locked'
   /// flag is special and should remain the least-significant bit. When other
   /// flags are added,they should be kept in alphabetical order for readability,
   /// and all flag values should be adjusted to keep bit-significance in
   /// ascending order.
   //////////////////////////////////////////////////////////////////////////////
-  enum class Flag : std::uint32_t {
-    locked = 0x00000001,
-    banished = 0x00000002,
-    disabled = 0x00000004,
-    evictions = 0x00000008,
-    migrated = 0x00000010,
-    migrating = 0x00000020,
-    rebalancing = 0x00000040,
-    resizing = 0x00000080,
-    shutdown = 0x00000100,
-    shuttingDown = 0x00000200,
+  enum class Flag : std::uint16_t {
+    locked = 0x0001,
+    banished = 0x0002,
+    migrated = 0x0004,
   };
 
+  using FlagType = std::underlying_type<Flag>::type;
+
   //////////////////////////////////////////////////////////////////////////////
   /// @brief Initializes state with no flags set and unlocked
   //////////////////////////////////////////////////////////////////////////////
@@ -117,7 +111,6 @@ struct BucketState {
   /// @brief Checks whether the given flag is set. Requires state to be locked.
   //////////////////////////////////////////////////////////////////////////////
   bool isSet(BucketState::Flag flag) const noexcept;
-  bool isSet(BucketState::Flag flag1, BucketState::Flag flag2) const noexcept;
 
   //////////////////////////////////////////////////////////////////////////////
   /// @brief Toggles the given flag. Requires state to be locked.
@@ -130,12 +123,10 @@ struct BucketState {
   void clear() noexcept;
 
  private:
-  std::atomic<std::uint32_t> _state;
+  std::atomic<FlagType> _state;
 };
 
-// ensure that state is exactly the size of uint32_t
-static_assert(sizeof(BucketState) == sizeof(std::uint32_t),
-              "Expected sizeof(BucketState) == sizeof(uint32_t).");
+// ensure that state is exactly the size of uint16_t
+static_assert(sizeof(BucketState) == sizeof(std::uint16_t));
 
-};  // end namespace cache
-};  // end namespace arangodb
+};  // end namespace arangodb::cache
diff --git a/arangod/Cache/Cache.h b/arangod/Cache/Cache.h
@@ -23,8 +23,8 @@
 
 #pragma once
 
+#include "Basics/ErrorCode.h"
 #include "Basics/ReadWriteSpinLock.h"
-#include "Basics/Result.h"
 #include "Basics/SharedCounter.h"
 #include "Cache/CachedValue.h"
 #include "Cache/Common.h"
@@ -87,9 +87,9 @@ class Cache : public std::enable_shared_from_this<Cache> {
 
   // primary functionality; documented in derived classes
   virtual Finding find(void const* key, std::uint32_t keySize) = 0;
-  virtual Result insert(CachedValue* value) = 0;
-  virtual Result remove(void const* key, std::uint32_t keySize) = 0;
-  virtual Result banish(void const* key, std::uint32_t keySize) = 0;
+  virtual ::ErrorCode insert(CachedValue* value) = 0;
+  virtual ::ErrorCode remove(void const* key, std::uint32_t keySize) = 0;
+  virtual ::ErrorCode banish(void const* key, std::uint32_t keySize) = 0;
 
   //////////////////////////////////////////////////////////////////////////////
   /// @brief Returns the ID for this cache.
@@ -181,18 +181,18 @@ class Cache : public std::enable_shared_from_this<Cache> {
           CachedValue::construct(key, keySize, value, valueSize)};
       if (ADB_LIKELY(cv)) {
         status = cache.insert(cv.get());
-        if (status.ok()) {
+        if (status == TRI_ERROR_NO_ERROR) {
           cv.release();
         }
       } else {
-        status.reset(TRI_ERROR_OUT_OF_MEMORY);
+        status = TRI_ERROR_OUT_OF_MEMORY;
       }
     }
 
     Inserter(Inserter const& other) = delete;
     Inserter& operator=(Inserter const& other) = delete;
 
-    Result status;
+    ::ErrorCode status = TRI_ERROR_NO_ERROR;
   };
 
   // same as Cache::Inserter, but more lightweight. Does not provide
@@ -203,7 +203,7 @@ class Cache : public std::enable_shared_from_this<Cache> {
                    void const* value, std::size_t valueSize) {
       std::unique_ptr<CachedValue> cv{
           CachedValue::construct(key, keySize, value, valueSize)};
-      if (ADB_LIKELY(cv) && cache.insert(cv.get()).ok()) {
+      if (ADB_LIKELY(cv) && cache.insert(cv.get()) == TRI_ERROR_NO_ERROR) {
         cv.release();
       }
     }