Simplify TRACE logging in MagmaKVStore::compactionCallBack()

Before this patch trace logging in MagmaKVStore::compactionCallBack() was fairly complex using a reusable std::stringstream stored in MagmaKVStore::MagmaCompactionCB. Instead introduce a string inside the MagmaKVStore::compactionCallBack(), which can be used to store the formatted sanitized key and meta data. These means we'll only ever allocate one string when not using TRACE level logging meaning we only use 8 bytes instead of 368. Then when we're using TRACE level logging we're still only generating one string per call to MagmaKVStore::compactionCallBack(). Change-Id: Ibf7924ab2f383ca02096e8a61044958f0e0d80d9 Reviewed-on: http://review.couchbase.org/c/kv_engine/+/160181 Reviewed-by: Dave Rigby <daver@couchbase.com> Tested-by: Build Bot <build@couchbase.com>
couchbase · Aug 27, 2021 · 29dde61 · 29dde61
1 parent 9da45b1
commit 29dde61
Show file tree

Hide file tree

Showing 2 changed files with 14 additions and 23 deletions.
diff --git a/engines/ep/src/kvstore/magma-kvstore/magma-kvstore.cc b/engines/ep/src/kvstore/magma-kvstore/magma-kvstore.cc
@@ -235,16 +235,14 @@ bool MagmaKVStore::compactionCallBack(MagmaKVStore::MagmaCompactionCB& cbCtx,
     }
 
     auto vbid = magmakv::getVbid(metaSlice);
-    auto& itemString = cbCtx.itemKeyBuf;
+    std::string userSanitizedItemStr;
     if (logger->should_log(spdlog::level::TRACE)) {
-        itemString.str(std::string());
-        itemString << "key:"
-                   << cb::UserData{makeDiskDocKey(keySlice).to_string()};
-        itemString << " ";
-        itemString << magmakv::getDocMeta(metaSlice).to_string();
-        logger->TRACE("MagmaCompactionCB: {} {}",
-                      vbid,
-                      cb::UserData(itemString.str()));
+        userSanitizedItemStr =
+                "key:" +
+                cb::UserData{makeDiskDocKey(keySlice).to_string()}
+                        .getSanitizedValue() +
+                " " + magmakv::getDocMeta(metaSlice).to_string();
+        logger->TRACE("MagmaCompactionCB: {} {}", vbid, userSanitizedItemStr);
     }
 
     if (!cbCtx.ctx) {
@@ -301,7 +299,7 @@ bool MagmaKVStore::compactionCallBack(MagmaKVStore::MagmaCompactionCB& cbCtx,
                         "MagmaKVStore::MagmaCompactionCallback: DROP "
                         "collections "
                         "{}",
-                        itemString.str());
+                        userSanitizedItemStr);
             }
             return true;
         }
@@ -323,7 +321,7 @@ bool MagmaKVStore::compactionCallBack(MagmaKVStore::MagmaCompactionCB& cbCtx,
                 if (logger->should_log(spdlog::level::TRACE)) {
                     logger->TRACE("MagmaCompactionCB: {} DROP drop_deletes {}",
                                   vbid,
-                                  cb::UserData(itemString.str()));
+                                  userSanitizedItemStr);
                 }
                 drop = true;
             }
@@ -333,7 +331,7 @@ bool MagmaKVStore::compactionCallBack(MagmaKVStore::MagmaCompactionCB& cbCtx,
                     logger->TRACE(
                             "MagmaCompactionCB: {} DROP expired tombstone {}",
                             vbid,
-                            cb::UserData(itemString.str()));
+                            userSanitizedItemStr);
                 }
                 drop = true;
             }
@@ -363,7 +361,7 @@ bool MagmaKVStore::compactionCallBack(MagmaKVStore::MagmaCompactionCB& cbCtx,
                     logger->TRACE(
                             "MagmaKVStore::MagmaCompactionCallback: "
                             "DROP prepare {}",
-                            itemString.str());
+                            userSanitizedItemStr);
                 }
                 return true;
             }
@@ -392,15 +390,14 @@ bool MagmaKVStore::compactionCallBack(MagmaKVStore::MagmaCompactionCB& cbCtx,
             if (logger->should_log(spdlog::level::TRACE)) {
                 logger->TRACE("MagmaCompactionCB: {} expiry callback {}",
                               vbid,
-                              cb::UserData(itemString.str()));
+                              userSanitizedItemStr);
             }
         }
     }
 
     if (logger->should_log(spdlog::level::TRACE)) {
-        logger->TRACE("MagmaCompactionCB: {} KEEP {}",
-                      vbid,
-                      cb::UserData(itemString.str()));
+        logger->TRACE(
+                "MagmaCompactionCB: {} KEEP {}", vbid, userSanitizedItemStr);
     }
     return false;
 }

diff --git a/engines/ep/src/kvstore/magma-kvstore/magma-kvstore.h b/engines/ep/src/kvstore/magma-kvstore/magma-kvstore.h
@@ -753,12 +753,6 @@ class MagmaKVStore : public KVStore {
         }
         MagmaKVStore& magmaKVStore;
 
-        // The only usage of this is is in compactionCallback and it could be a
-        // local variable instead but it's less expensive to reset the contents
-        // of the stringstream than it is to destroy/recreate the stringstream
-        // for each key we visit.
-        std::stringstream itemKeyBuf;
-
         std::shared_ptr<CompactionContext> ctx;
 
         MagmaDbStats magmaDbStats;