From: Piotr Dałek <piotr.dalek@ts.fujitsu.com>
Date: Thu, 15 Oct 2015 13:40:06 +0000 (+0200)
Subject: os/KeyValueDB: reduce malloc/free/string copy count
X-Git-Tag: v10.0.1~116^2~21
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=709b111ceafe3893e82e3d576aa7b2d58bd16516;p=ceph.git

os/KeyValueDB: reduce malloc/free/string copy count

In RocksDB, LevelDB and Kinetic wrapper code, there is unnecessary
string copying. In particular, split_key makes an extra copy of string
(as a basis for extracted key and/or value), which can be easily omitted.
Also, checking for iterator validity generates std::pair with prefix
and key and checks prefix with the one from pair, wasting memory for
key.

Signed-off-by: Piotr Dałek <piotr.dalek@ts.fujitsu.com>
---

diff --git a/src/kv/KeyValueDB.h b/src/kv/KeyValueDB.h
index 755a627f8f9f..8691694ef3c5 100644
--- a/src/kv/KeyValueDB.h
+++ b/src/kv/KeyValueDB.h
@@ -128,6 +128,7 @@ public:
     virtual int prev() = 0;
     virtual std::string key() = 0;
     virtual std::pair<std::string,std::string> raw_key() = 0;
+    virtual bool raw_key_is_prefixed(const std::string &prefix) = 0;
     virtual bufferlist value() = 0;
     virtual int status() = 0;
     virtual ~WholeSpaceIteratorImpl() { }
@@ -157,8 +158,7 @@ public:
     bool valid() {
       if (!generic_iter->valid())
 	return false;
-      std::pair<std::string,std::string> raw_key = generic_iter->raw_key();
-      return (raw_key.first.compare(0, prefix.length(), prefix) == 0);
+      return generic_iter->raw_key_is_prefixed(prefix);
     }
     int next() {
       if (valid())
diff --git a/src/kv/KineticStore.cc b/src/kv/KineticStore.cc
index fb6e2bfe002e..b79ab37314be 100644
--- a/src/kv/KineticStore.cc
+++ b/src/kv/KineticStore.cc
@@ -205,14 +205,22 @@ bufferlist KineticStore::to_bufferlist(const kinetic::KineticRecord &record)
 
 int KineticStore::split_key(string in_prefix, string *prefix, string *key)
 {
-  size_t prefix_len = in_prefix.find('\1');
+  size_t prefix_len = 0;
+  char* in_data = in.c_str();
+  
+  // Find separator inside Slice
+  char* separator = (char*) memchr((void*)in_data, 1, in.size());
+  if (separator == NULL)
+     return -EINVAL;
+  prefix_len = size_t(separator - in_data);
   if (prefix_len >= in_prefix.size())
     return -EINVAL;
 
+  // Fetch prefix and/or key directly from Slice
   if (prefix)
-    *prefix = string(in_prefix, 0, prefix_len);
+    *prefix = string(in_data, 0, prefix_len);
   if (key)
-    *key= string(in_prefix, prefix_len + 1);
+    *key = string(separator+1, 0, in_prefix.size()-prefix_len-1);
   return 0;
 }
 
@@ -316,6 +324,17 @@ pair<string,string> KineticStore::KineticWholeSpaceIteratorImpl::raw_key() {
   return make_pair(prefix, key);
 }
 
+bool KineticStore::KineticWholeSpaceIteratorImpl::raw_key_is_prefixed(const string &prefix) {
+  // Look for "prefix\1" right in *keys_iter without making a copy
+  string key = *keys_iter;
+  if ((key.size() > prefix.length()) && (key[prefix.length()] == '\1')) {
+    return memcmp(key.c_str(), prefix.c_str(), prefix.length()) == 0;
+  } else {
+    return false;
+  }
+}
+
+
 bufferlist KineticStore::KineticWholeSpaceIteratorImpl::value() {
   dout(30) << "kinetic iterator value()" << dendl;
   unique_ptr<kinetic::KineticRecord> record;
diff --git a/src/kv/KineticStore.h b/src/kv/KineticStore.h
index cbb7633466b1..14c35d771225 100644
--- a/src/kv/KineticStore.h
+++ b/src/kv/KineticStore.h
@@ -130,6 +130,7 @@ public:
     int prev();
     string key();
     pair<string,string> raw_key();
+    bool raw_key_is_prefixed(const string &prefix);
     bufferlist value();
     int status();
   };
diff --git a/src/kv/LevelDBStore.cc b/src/kv/LevelDBStore.cc
index 6a71aef9ec34..92c915810f68 100644
--- a/src/kv/LevelDBStore.cc
+++ b/src/kv/LevelDBStore.cc
@@ -228,15 +228,21 @@ bufferlist LevelDBStore::to_bufferlist(leveldb::Slice in)
 
 int LevelDBStore::split_key(leveldb::Slice in, string *prefix, string *key)
 {
-  string in_prefix = in.ToString();
-  size_t prefix_len = in_prefix.find('\0');
-  if (prefix_len >= in_prefix.size())
+  size_t prefix_len = 0;
+  
+  // Find separator inside Slice
+  char* separator = (char*) memchr(in.data(), 0, in.size());
+  if (separator == NULL)
+     return -EINVAL;
+  prefix_len = size_t(separator - in.data());
+  if (prefix_len >= in.size())
     return -EINVAL;
 
+  // Fetch prefix and/or key directly from Slice
   if (prefix)
-    *prefix = string(in_prefix, 0, prefix_len);
+    *prefix = string(in.data(), 0, prefix_len);
   if (key)
-    *key= string(in_prefix, prefix_len + 1);
+    *key = string(separator+1, 0, in.size()-prefix_len-1);
   return 0;
 }
 
diff --git a/src/kv/LevelDBStore.h b/src/kv/LevelDBStore.h
index a42f2f067c7e..f450046d38d7 100644
--- a/src/kv/LevelDBStore.h
+++ b/src/kv/LevelDBStore.h
@@ -277,6 +277,14 @@ public:
       split_key(dbiter->key(), &prefix, &key);
       return make_pair(prefix, key);
     }
+    bool raw_key_is_prefixed(const string &prefix) {
+      leveldb::Slice key = dbiter->key();
+      if ((key.size() > prefix.length()) && (key[prefix.length()] == '\0')) {
+        return memcmp(key.data(), prefix.c_str(), prefix.length()) == 0;
+      } else {
+        return false;
+      }
+    }
     bufferlist value() {
       return to_bufferlist(dbiter->value());
     }
diff --git a/src/kv/RocksDBStore.cc b/src/kv/RocksDBStore.cc
index a75274328601..ce46b93d0fc8 100644
--- a/src/kv/RocksDBStore.cc
+++ b/src/kv/RocksDBStore.cc
@@ -303,15 +303,21 @@ bufferlist RocksDBStore::to_bufferlist(rocksdb::Slice in)
 
 int RocksDBStore::split_key(rocksdb::Slice in, string *prefix, string *key)
 {
-  string in_prefix = in.ToString();
-  size_t prefix_len = in_prefix.find('\0');
-  if (prefix_len >= in_prefix.size())
+  size_t prefix_len = 0;
+  
+  // Find separator inside Slice
+  char* separator = (char*) memchr(in.data(), 0, in.size());
+  if (separator == NULL)
+     return -EINVAL;
+  prefix_len = size_t(separator - in.data());
+  if (prefix_len >= in.size())
     return -EINVAL;
 
+  // Fetch prefix and/or key directly from Slice
   if (prefix)
-    *prefix = string(in_prefix, 0, prefix_len);
+    *prefix = string(in.data(), 0, prefix_len);
   if (key)
-    *key= string(in_prefix, prefix_len + 1);
+    *key = string(separator+1, 0, in.size()-prefix_len-1);
   return 0;
 }
 
@@ -473,6 +479,17 @@ pair<string,string> RocksDBStore::RocksDBWholeSpaceIteratorImpl::raw_key()
   split_key(dbiter->key(), &prefix, &key);
   return make_pair(prefix, key);
 }
+
+bool RocksDBStore::RocksDBWholeSpaceIteratorImpl::raw_key_is_prefixed(const string &prefix) {
+  // Look for "prefix\0" right in rocksb::Slice
+  rocksdb::Slice key = dbiter->key();
+  if ((key.size() > prefix.length()) && (key[prefix.length()] == '\0')) {
+    return memcmp(key.data(), prefix.c_str(), prefix.length()) == 0;
+  } else {
+    return false;
+  }
+}
+
 bufferlist RocksDBStore::RocksDBWholeSpaceIteratorImpl::value()
 {
   return to_bufferlist(dbiter->value());
diff --git a/src/kv/RocksDBStore.h b/src/kv/RocksDBStore.h
index c03b20c86b3f..9ea125fcf872 100644
--- a/src/kv/RocksDBStore.h
+++ b/src/kv/RocksDBStore.h
@@ -183,6 +183,7 @@ public:
     int prev();
     string key();
     pair<string,string> raw_key();
+    bool raw_key_is_prefixed(const string &prefix);
     bufferlist value();
     int status();
   };