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 755a627f8f9f62dbdc3d1a2efdb02a827c925bbb..8691694ef3c5d46eba785e6c83770ca1cfb2611c 100644 (file)
--- 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 fb6e2bfe002e4f40626f0ef73913f5e5e1d5a07a..b79ab37314bed0228fb728008d8eae434f687bec 100644 (file)
--- 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 cbb7633466b1003153ff185182e520111ac379f0..14c35d771225e298c693b5456b4710dc19565435 100644 (file)
--- 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 6a71aef9ec343dac9ecfbb22a121207117963521..92c915810f68cadaae6800f5fc0997cdeefd6038 100644 (file)
--- 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 a42f2f067c7e0b926ff70749983e13b67368328c..f450046d38d764b54d5f805fab5147df27d85997 100644 (file)
--- 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 a75274328601466582d31eec82a3b0947cc85f5e..ce46b93d0fc8e49c07ff83e8dc9ffd5f388baccf 100644 (file)
--- 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 c03b20c86b3fb7e522dc6015c0dbed7d3a447c5c..9ea125fcf872819c2d24546cb16830883cf4fb30 100644 (file)
--- 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();
   };