Fix redundant sampleKey() in NvmCache event tracking

Summary:
NvmCache::recordEvent() calls sampleKey(), then routes through CacheAPIWrapperForNvm::recordEvent() → CacheAllocator::recordEvent() which calls sampleKey() again on the same key. Since sampleKey() is deterministic for a given key, the second call always produces the same result as the first making it redundant CPU work (hashing + comparison) on every NVM event.

Fix by adding recordEventWithoutSampling() to CacheAllocator and its CacheApiWrapper, which skips the redundant sampleKey() check and goes directly to eventTracker->recordWithoutSampling(). NvmCache::recordEvent() now calls sampleKey() once, then uses the no-sampling path downstream.

Reviewed By: rlyerly

Differential Revision: D100874852

fbshipit-source-id: 60205bb38eb086e82beed82edb7a5797243ba35d

Author: pbhandar2

cachelib/allocator/CacheAllocator.h

@@ -1876,32 +1876,45 @@ class CacheAllocator : public CacheBase {
                    AllocatorApiResult result,
                    EventRecordParams params = {}) const {
     if (auto* eventTracker = getEventTracker()) {
-      if (eventTracker->sampleKey(key)) {
-        EventInfo eventInfo;
-        eventInfo.eventTimestamp = util::getCurrentTimeSec();
-        eventInfo.event = event;
-        eventInfo.result = result;
-        eventInfo.key = key;
-        if (params.size) {
-          eventInfo.size = *params.size;
-        }
-        if (params.expiryTime) {
-          eventInfo.expiryTime = *params.expiryTime;
-          eventInfo.timeToExpire = calculateTimeToExpire(
-              *params.expiryTime, eventInfo.eventTimestamp);
-        }
-        if (params.ttlSecs && *params.ttlSecs > 0) {
-          eventInfo.ttlSecs = *params.ttlSecs;
-        }
-        if (params.allocSize) {
-          eventInfo.allocSize = *params.allocSize;
-        }
-        if (params.poolId) {
-          eventInfo.poolId = *params.poolId;
-        }
+      if (!eventTracker->sampleKey(key)) {
+        return;
+      }
+    }
+    recordEventWithoutSampling(event, key, result, std::move(params));
+  }
 
-        eventTracker->recordWithoutSampling(eventInfo);
+  // Record event without calling sampleKey(). Use when the caller has already
+  // determined that the key should be sampled (e.g., NvmCache::recordEvent()
+  // calls sampleKey() once and then uses this to avoid double-sampling).
+  void recordEventWithoutSampling(AllocatorApiEvent event,
+                                  Key key,
+                                  AllocatorApiResult result,
+                                  EventRecordParams params = {}) const {
+    if (auto* eventTracker = getEventTracker()) {
+      EventInfo eventInfo;
+      eventInfo.eventTimestamp = util::getCurrentTimeSec();
+      eventInfo.event = event;
+      eventInfo.result = result;
+      eventInfo.key = key;
+      if (params.size) {
+        eventInfo.size = *params.size;
+      }
+      if (params.expiryTime) {
+        eventInfo.expiryTime = *params.expiryTime;
+        eventInfo.timeToExpire =
+            calculateTimeToExpire(*params.expiryTime, eventInfo.eventTimestamp);
+      }
+      if (params.ttlSecs && *params.ttlSecs > 0) {
+        eventInfo.ttlSecs = *params.ttlSecs;
       }
+      if (params.allocSize) {
+        eventInfo.allocSize = *params.allocSize;
+      }
+      if (params.poolId) {
+        eventInfo.poolId = *params.poolId;
+      }
+
+      eventTracker->recordWithoutSampling(eventInfo);
     } else if (auto legacyEventTracker = getLegacyEventTracker()) {
       folly::Optional<uint32_t> size =
           params.size

cachelib/allocator/nvmcache/CacheApiWrapper.h

@@ -172,6 +172,17 @@ class CacheAPIWrapperForNvm {
                           typename C::EventRecordParams params = {}) {
     cache.recordEvent(event, key, result, params);
   }
+
+  // Record event without sampling. Use when the caller has already called
+  // sampleKey() and determined the key should be sampled.
+  static void recordEventWithoutSampling(
+      C& cache,
+      AllocatorApiEvent event,
+      Key key,
+      AllocatorApiResult result,
+      typename C::EventRecordParams params = {}) {
+    cache.recordEventWithoutSampling(event, key, result, std::move(params));
+  }
 };
 
 } // namespace cachelib

cachelib/allocator/nvmcache/NvmCache.h

@@ -321,6 +321,8 @@ class NvmCache {
                    folly::StringPiece key,
                    AllocatorApiResult result,
                    const NvmItem* nvmItem = nullptr) {
+    // Sample the key once here. Use recordEventWithoutSampling() downstream
+    // to avoid double/triple-sampling through CacheAllocator::recordEvent().
     if (auto eventTracker = CacheAPIWrapperForNvm<C>::getEventTracker(cache_)) {
       if (!eventTracker->sampleKey(key)) {
         return;
@@ -330,7 +332,8 @@ class NvmCache {
     }
 
     if (!nvmItem) {
-      CacheAPIWrapperForNvm<C>::recordEvent(cache_, event, key, result);
+      CacheAPIWrapperForNvm<C>::recordEventWithoutSampling(cache_, event, key,
+                                                           result);
       return;
     }
 
@@ -343,7 +346,7 @@ class NvmCache {
       ttlSecs = expiryTime - nvmItem->getCreationTime();
     }
 
-    CacheAPIWrapperForNvm<C>::recordEvent(
+    CacheAPIWrapperForNvm<C>::recordEventWithoutSampling(
         cache_, event, key, result,
         typename C::EventRecordParams{.size = itemSize,
                                       .ttlSecs = ttlSecs,