MB-21650: Don't copy key when servicing memcached get() request 23/70023/5
authorDave Rigby <daver@couchbase.com>
Wed, 16 Nov 2016 17:37:13 +0000 (09:37 -0800)
committerDave Rigby <daver@couchbase.com>
Fri, 18 Nov 2016 13:14:28 +0000 (13:14 +0000)
In the get request path (EvpGet() and onwards) we take a copy of the
key from the request packet into a std::string. This is then passed
around ep-engine (read-only) to retrieve the document to return to the
user.

This incurs the cost of creating a std::string object per get request,
along with the related memory tracking overheads which are not
insigificant (see ObjectRegistry).

To reduce this cost, use a const_sized_buffer (essentially a pair of
{const char*, size_t}) for the key, relying on the fact that the
client's request is always available in the request packet owned by
memcached.

On the 2-node 'hera' cluster this improves the op/s of the following
pillowfight benchmark, from 1.8M op/s total to 2.3M op/s:

    cbc-pillowfight --spec couchbase://172.23.96.117:12000/default \
        --batch-size 1000 --num-items 1000000 --num-threads 50 \
         --min-size 512 --max-size 512 --set-pct 20

(client running on 3rd hera-client node).

Change-Id: I0371fb5ef9bdcdc6f92bb941926e8af80cf5e24f
Reviewed-on: http://review.couchbase.org/70023
Reviewed-by: Daniel Owen <owend@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
src/ep.cc
src/ep.h
src/ep_engine.h
src/stored-value.cc
src/stored-value.h
src/tasks.h
src/vbucket.cc
src/vbucket.h
tests/module_tests/evp_store_rollback_test.cc
tests/module_tests/hash_table_test.cc

index deef2bb..d48485b 100644 (file)
--- a/src/ep.cc
+++ b/src/ep.cc
@@ -649,7 +649,7 @@ EventuallyPersistentStore::deleteExpiredItems(std::list<std::pair<uint16_t,
 }
 
 StoredValue *EventuallyPersistentStore::fetchValidValue(RCPtr<VBucket> &vb,
-                                                        const std::string &key,
+                                                        const const_sized_buffer key,
                                                         int bucket_num,
                                                         bool wantDeleted,
                                                         bool trackReference,
@@ -745,7 +745,7 @@ protocol_binary_response_status EventuallyPersistentStore::evictKey(
 ENGINE_ERROR_CODE EventuallyPersistentStore::addTempItemForBgFetch(
                                                         LockHolder &lock,
                                                         int bucket_num,
-                                                        const std::string &key,
+                                                        const const_sized_buffer key,
                                                         RCPtr<VBucket> &vb,
                                                         const void *cookie,
                                                         bool metadataOnly,
@@ -1855,7 +1855,7 @@ void EventuallyPersistentStore::completeBGFetchMulti(uint16_t vbId,
         uint64_t(fetchedItems.size()), vbId, gethrtime()/1000000);
 }
 
-void EventuallyPersistentStore::bgFetch(const std::string &key,
+void EventuallyPersistentStore::bgFetch(const const_sized_buffer key,
                                         uint16_t vbucket,
                                         const void *cookie,
                                         bool isMeta) {
@@ -1890,7 +1890,7 @@ void EventuallyPersistentStore::bgFetch(const std::string &key,
     }
 }
 
-GetValue EventuallyPersistentStore::getInternal(const std::string &key,
+GetValue EventuallyPersistentStore::getInternal(const const_sized_buffer key,
                                                 uint16_t vbucket,
                                                 const void *cookie,
                                                 vbucket_state_t allowedState,
index b556961..0aa8c2e 100644 (file)
--- a/src/ep.h
+++ b/src/ep.h
@@ -262,7 +262,7 @@ public:
      *
      * @return a GetValue representing the result of the request
      */
-    GetValue get(const std::string &key, uint16_t vbucket,
+    GetValue get(const const_sized_buffer key, uint16_t vbucket,
                  const void *cookie, get_options_t options) {
         return getInternal(key, vbucket, cookie, vbucket_state_active,
                            options);
@@ -280,7 +280,7 @@ public:
      *
      * @return a GetValue representing the result of the request
      */
-    GetValue getReplica(const std::string &key, uint16_t vbucket,
+    GetValue getReplica(const const_sized_buffer key, uint16_t vbucket,
                         const void *cookie,
                         get_options_t options = static_cast<get_options_t>(
                                                         QUEUE_BG_FETCH |
@@ -456,7 +456,7 @@ public:
      * @param type whether the fetch is for a non-resident value or metadata of
      *             a (possibly) deleted item
      */
-    void bgFetch(const std::string &key,
+    void bgFetch(const const_sized_buffer key,
                  uint16_t vbucket,
                  const void *cookie,
                  bool isMeta = false);
@@ -993,17 +993,17 @@ protected:
     PersistenceCallback* flushOneDelOrSet(const queued_item &qi,
                                           RCPtr<VBucket> &vb);
 
-    StoredValue *fetchValidValue(RCPtr<VBucket> &vb, const std::string &key,
+    StoredValue *fetchValidValue(RCPtr<VBucket> &vb, const const_sized_buffer key,
                                  int bucket_num, bool wantsDeleted=false,
                                  bool trackReference=true, bool queueExpired=true);
 
-    GetValue getInternal(const std::string &key, uint16_t vbucket,
+    GetValue getInternal(const const_sized_buffer key, uint16_t vbucket,
                          const void *cookie,
                          vbucket_state_t allowedState,
                          get_options_t options = TRACK_REFERENCE);
 
     ENGINE_ERROR_CODE addTempItemForBgFetch(LockHolder &lock, int bucket_num,
-                                            const std::string &key, RCPtr<VBucket> &vb,
+                                            const const_sized_buffer key, RCPtr<VBucket> &vb,
                                             const void *cookie, bool metadataOnly,
                                             bool isReplication = false);
 
index 01a91c3..1fea559 100644 (file)
@@ -302,7 +302,7 @@ public:
                           get_options_t options)
     {
         BlockTimer timer(&stats.getCmdHisto);
-        std::string k(static_cast<const char*>(key), nkey);
+        const const_sized_buffer k(static_cast<const char*>(key), nkey);
 
         GetValue gv(epstore->get(k, vbucket, cookie, options));
         ENGINE_ERROR_CODE ret = gv.getStatus();
index cbde24d..11d4182 100644 (file)
@@ -681,7 +681,7 @@ add_type_t HashTable::unlocked_add(int &bucket_num,
 }
 
 add_type_t HashTable::unlocked_addTempItem(int &bucket_num,
-                                           const std::string &key,
+                                           const const_sized_buffer key,
                                            item_eviction_policy_t policy,
                                            bool isReplication) {
 
@@ -692,7 +692,7 @@ add_type_t HashTable::unlocked_addTempItem(int &bucket_num,
     uint8_t ext_meta[1];
     uint8_t ext_len = EXT_META_LEN;
     *(ext_meta) = PROTOCOL_BINARY_RAW_BYTES;
-    Item itm(key.c_str(), key.length(), /*flags*/0, /*exp*/0, /*data*/NULL,
+    Item itm(key.data(), key.size(), /*flags*/0, /*exp*/0, /*data*/NULL,
              /*size*/0, ext_meta, ext_len, 0, StoredValue::state_temp_init);
 
     // if a temp item for a possibly deleted, set it non-resident by resetting
index a324686..8ecebf7 100644 (file)
@@ -23,6 +23,7 @@
 #include "item_pager.h"
 #include "utility.h"
 
+#include "daemon/buffer.h"
 #include <platform/cb_malloc.h>
 
 // Forward declaration for StoredValue
@@ -132,8 +133,8 @@ public:
      * @param k the key we're checking
      * @return true if this item's key is equal to k
      */
-    bool hasKey(const std::string &k) const {
-        return k.length() == getKeyLen()
+    bool hasKey(const const_sized_buffer k) const {
+        return k.size() == getKeyLen()
             && (std::memcmp(k.data(), getKeyBytes(), getKeyLen()) == 0);
     }
 
@@ -966,7 +967,7 @@ public:
      * @param key the key to find
      * @return a pointer to a StoredValue -- NULL if not found
      */
-    StoredValue *find(const std::string &key, bool trackReference=true) {
+    StoredValue *find(const const_sized_buffer key, bool trackReference=true) {
         if (!isActive()) {
             throw std::logic_error("HashTable::find: Cannot call on a "
                     "non-active object");
@@ -1197,7 +1198,7 @@ public:
      * @return an indication of what happened
      */
     add_type_t unlocked_addTempItem(int &bucket_num,
-                                    const std::string &key,
+                                    const const_sized_buffer key,
                                     item_eviction_policy_t policy,
                                     bool isReplication = false);
 
@@ -1209,7 +1210,7 @@ public:
      * @param policy item eviction policy
      * @return an indicator of what the deletion did
      */
-    mutation_type_t softDelete(const std::string &key, uint64_t cas,
+    mutation_type_t softDelete(const const_sized_buffer key, uint64_t cas,
                                item_eviction_policy_t policy = VALUE_ONLY) {
         if (!isActive()) {
             throw std::logic_error("HashTable::softDelete: Cannot call on a "
@@ -1303,7 +1304,7 @@ public:
      *
      * @return a pointer to a StoredValue -- NULL if not found
      */
-    StoredValue *unlocked_find(const std::string &key, int bucket_num,
+    StoredValue *unlocked_find(const const_sized_buffer key, int bucket_num,
                                bool wantsDeleted=false, bool trackReference=true) {
         StoredValue *v = values[bucket_num];
         while (v) {
@@ -1412,6 +1413,10 @@ public:
         return getLockedBucket(hash(s.data(), s.size()), bucket);
     }
 
+    inline LockHolder getLockedBucket(const const_sized_buffer key, int *bucket) {
+        return getLockedBucket(hash(key.data(), key.size()), bucket);
+    }
+
     /**
      * Delete a key from the cache without trying to lock the cache first
      * (Please note that you <b>MUST</b> acquire the mutex before calling
@@ -1421,7 +1426,7 @@ public:
      * @param bucket_num the bucket to look in (must already be locked)
      * @return true if an object was deleted, false otherwise
      */
-    bool unlocked_del(const std::string &key, int bucket_num) {
+    bool unlocked_del(const const_sized_buffer key, int bucket_num) {
         if (!isActive()) {
             throw std::logic_error("HashTable::unlocked_del: Cannot call on a "
                     "non-active object");
@@ -1484,7 +1489,7 @@ public:
      * @param key the key to delete
      * @return true if the item existed before this call
      */
-    bool del(const std::string &key) {
+    bool del(const const_sized_buffer key) {
         int bucket_num(0);
         LockHolder lh = getLockedBucket(key, &bucket_num);
         return unlocked_del(key, bucket_num);
index 6d49af6..968e214 100644 (file)
@@ -27,6 +27,7 @@
 #include <string>
 #include "atomic.h"
 #include "kvstore.h"
+#include "daemon/buffer.h"
 
 enum task_state_t {
     TASK_RUNNING,
@@ -382,11 +383,11 @@ private:
  */
 class SingleBGFetcherTask : public GlobalTask {
 public:
-    SingleBGFetcherTask(EventuallyPersistentEngine *e, const std::string &k,
+    SingleBGFetcherTask(EventuallyPersistentEngine *e, const const_sized_buffer k,
                        uint16_t vbid, const void *c, bool isMeta,
                        int sleeptime = 0, bool completeBeforeShutdown = false)
         : GlobalTask(e, TaskId::SingleBGFetcherTask, sleeptime, completeBeforeShutdown),
-          key(k),
+          key(k.data(), k.size()),
           vbucket(vbid),
           cookie(c),
           metaFetch(isMeta),
index b12d5ba..a532c6f 100644 (file)
@@ -287,11 +287,12 @@ void VBucket::addStat(const char *nm, const T &val, ADD_STAT add_stat,
     }
 }
 
-size_t VBucket::queueBGFetchItem(const std::string &key,
+size_t VBucket::queueBGFetchItem(const const_sized_buffer key,
                                  VBucketBGFetchItem *fetch,
                                  BgFetcher *bgFetcher) {
     LockHolder lh(pendingBGFetchesLock);
-    vb_bgfetch_item_ctx_t& bgfetch_itm_ctx = pendingBGFetches[key];
+    vb_bgfetch_item_ctx_t& bgfetch_itm_ctx =
+        pendingBGFetches[std::string(key.data(), key.size())];
 
     if (bgfetch_itm_ctx.bgfetched_list.empty()) {
         bgfetch_itm_ctx.isMetaOnly = true;
@@ -515,10 +516,10 @@ void VBucket::addToFilter(const std::string &key) {
     }
 }
 
-bool VBucket::maybeKeyExistsInFilter(const std::string &key) {
+bool VBucket::maybeKeyExistsInFilter(const const_sized_buffer key) {
     LockHolder lh(bfMutex);
     if (bFilter) {
-        return bFilter->maybeKeyExists(key.c_str(), key.length());
+        return bFilter->maybeKeyExists(key.data(), key.size());
     } else {
         // If filter doesn't exist, allow the BgFetch to go through.
         return true;
index 522fd04..04c2fdd 100644 (file)
@@ -373,7 +373,8 @@ public:
      * Returns the number of pending background fetches after
      * adding the specified item.
      **/
-    size_t queueBGFetchItem(const std::string &key, VBucketBGFetchItem *fetch,
+    size_t queueBGFetchItem(const const_sized_buffer key,
+                            VBucketBGFetchItem *fetch,
                             BgFetcher *bgFetcher);
 
     bool hasPendingBGFetchItems(void) {
@@ -417,7 +418,7 @@ public:
     void createFilter(size_t key_count, double probability);
     void initTempFilter(size_t key_count, double probability);
     void addToFilter(const std::string &key);
-    bool maybeKeyExistsInFilter(const std::string &key);
+    bool maybeKeyExistsInFilter(const const_sized_buffer key);
     bool isTempFilterAvailable();
     void addToTempFilter(const std::string &key);
     void swapFilter();
index 3aa7424..9faedcd 100644 (file)
@@ -56,13 +56,14 @@ protected:
     void rollback_after_deletion_test(bool flush_before_rollback) {
         // Setup: Store an item then flush the vBucket (creating a checkpoint);
         // then delete the item and create a second checkpoint.
-        auto item_v1 = store_item(vbid, "a", "1");
+        std::string a("a");
+        auto item_v1 = store_item(vbid, a, "1");
         ASSERT_EQ(initial_seqno + 1, item_v1.getBySeqno());
         ASSERT_EQ(1, store->flushVBucket(vbid));
         uint64_t cas = item_v1.getCas();
         mutation_descr_t mut_info;
         ASSERT_EQ(ENGINE_SUCCESS,
-                  store->deleteItem("a", &cas, vbid, /*cookie*/nullptr,
+                  store->deleteItem(a, &cas, vbid, /*cookie*/nullptr,
                                     /*force*/false, /*itemMeta*/nullptr,
                                     &mut_info));
         if (flush_before_rollback) {
@@ -70,13 +71,13 @@ protected:
         }
         // Sanity-check - item should no longer exist.
         EXPECT_EQ(ENGINE_KEY_ENOENT,
-                  store->get("a", vbid, nullptr, {}).getStatus());
+                  store->get(a, vbid, nullptr, {}).getStatus());
 
         // Test - rollback to seqno of item_v1 and verify that the previous value
         // of the item has been restored.
         store->setVBucketState(vbid, vbucket_state_replica, false);
         ASSERT_EQ(ENGINE_SUCCESS, store->rollback(vbid, item_v1.getBySeqno()));
-        auto result = store->public_getInternal("a", vbid, /*cookie*/nullptr,
+        auto result = store->public_getInternal(a, vbid, /*cookie*/nullptr,
                                                 vbucket_state_replica, {});
         ASSERT_EQ(ENGINE_SUCCESS, result.getStatus());
         EXPECT_EQ(item_v1, *result.getValue())
@@ -92,14 +93,16 @@ protected:
     void rollback_after_mutation_test(bool flush_before_rollback) {
         // Setup: Store an item then flush the vBucket (creating a checkpoint);
         // then update the item with a new value and create a second checkpoint.
-        auto item_v1 = store_item(vbid, "a", "old");
+        std::string a("a");
+        auto item_v1 = store_item(vbid, a, "old");
         ASSERT_EQ(initial_seqno + 1, item_v1.getBySeqno());
         ASSERT_EQ(1, store->flushVBucket(vbid));
 
-        auto item2 = store_item(vbid, "a", "new");
+        auto item2 = store_item(vbid, a, "new");
         ASSERT_EQ(initial_seqno + 2, item2.getBySeqno());
 
-        store_item(vbid, "key", "meh");
+        std::string key("key");
+        store_item(vbid, key, "meh");
 
         if (flush_before_rollback) {
             EXPECT_EQ(2, store->flushVBucket(vbid));
@@ -113,7 +116,7 @@ protected:
 
         // a should have the value of 'old'
         {
-            auto result = store->get("a", vbid, nullptr, {});
+            auto result = store->get(a, vbid, nullptr, {});
             ASSERT_EQ(ENGINE_SUCCESS, result.getStatus());
             EXPECT_EQ(item_v1, *result.getValue())
                 << "Fetched item after rollback should match item_v1";
@@ -122,7 +125,7 @@ protected:
 
         // key should be gone
         {
-            auto result = store->get("key", vbid, nullptr, {});
+            auto result = store->get(key, vbid, nullptr, {});
             EXPECT_EQ(ENGINE_KEY_ENOENT, result.getStatus())
                 << "A key set after the rollback point was found";
         }
@@ -172,7 +175,7 @@ protected:
         // These keys should be gone after the rollback
         for (int i = 0; i < 3; i++) {
             std::string key = "rollback-cp-" + std::to_string(i);
-            auto result = store->get(key.c_str(), vbid, nullptr, {});
+            auto result = store->get(key, vbid, nullptr, {});
             EXPECT_EQ(ENGINE_KEY_ENOENT, result.getStatus())
                 << "A key set after the rollback point was found";
         }
@@ -180,7 +183,7 @@ protected:
         // These keys should be gone after the rollback
         for (int i = 0; i < 3; i++) {
             std::string key = "anotherkey_" + std::to_string(i);
-            auto result = store->get(key.c_str(), vbid, nullptr, {});
+            auto result = store->get(key, vbid, nullptr, {});
             EXPECT_EQ(ENGINE_KEY_ENOENT, result.getStatus())
                 << "A key set after the rollback point was found";
         }
index e85fff2..4420f80 100644 (file)
@@ -595,7 +595,7 @@ TEST_F(HashTableTest, MB21448_UnlockedSetWithCASDeleted) {
     std::string key("key");
     Item item(key.data(), key.length(), 0, 0, "deleted", strlen("deleted"));
     ASSERT_EQ(WAS_CLEAN, ht.set(item));
-    ASSERT_EQ(WAS_DIRTY, ht.softDelete("key", 0));
+    ASSERT_EQ(WAS_DIRTY, ht.softDelete(key, 0));
 
     // Attempt to perform a set on a deleted key with a CAS.
     Item replacement(key.data(), key.length(), 0, 0, "value", strlen("value"));