Optimize reading data from leveldb with iterator. (#862)

Andrei Popescu · web-flow · commit 2a96485c14cd · 2020-05-25T17:34:11.000+02:00
When reading data from leveldb::Get() we need to use a string which
is poorly initialized and data is copied slowly and inefficient to it
which then again is memcpy-ied to a shared vector which adds another
layer of memcpy and slow initialization. The overhead for frequent data
reads is more then enough to consider an alternative.
This alternative consist in reading the data directly from the DB into
the resulting shared vector without any intermediate steps by using
iterators. The leveldb iterator gives us the possibility to access the
raw DB data and size without passing it into a inefficient string
before, and gives a huge performance bump.

Relates-To: OLPEDGE-1983

Signed-off-by: Andrei Popescu &lt;andrei.popescu@here.com&gt;
diff --git a/olp-cpp-sdk-core/src/cache/DefaultCacheImpl.cpp b/olp-cpp-sdk-core/src/cache/DefaultCacheImpl.cpp
@@ -21,6 +21,9 @@
 
 #include <algorithm>
 #include <chrono>
+#include <memory>
+#include <string>
+#include <utility>
 
 #include "olp/core/logging/Log.h"
 #include "olp/core/porting/make_unique.h"
@@ -231,24 +234,22 @@ KeyValueCache::ValueTypePtr DefaultCacheImpl::Get(const std::string& key) {
 
   if (memory_cache_) {
     auto value = memory_cache_->Get(key);
-
     if (!value.empty()) {
       PromoteKeyLru(key);
       return boost::any_cast<KeyValueCache::ValueTypePtr>(value);
     }
   }
 
-  auto disc_cache = GetFromDiscCache(key);
-  if (disc_cache) {
-    const std::string& cached_data = disc_cache->first;
-    auto data = std::make_shared<KeyValueCache::ValueType>(cached_data.size());
-
-    std::memcpy(&data->front(), cached_data.data(), cached_data.size());
+  KeyValueCache::ValueTypePtr value = nullptr;
+  time_t expiry = KeyValueCache::kDefaultExpiry;
 
+  auto result = GetFromDiskCache(key, value, expiry);
+  if (result && value) {
     if (memory_cache_) {
-      memory_cache_->Put(key, data, disc_cache->second, data->size());
+      memory_cache_->Put(key, value, expiry, value->size());
     }
-    return data;
+
+    return value;
   }
 
   return nullptr;
@@ -515,38 +516,54 @@ DefaultCache::StorageOpenResult DefaultCacheImpl::SetupStorage() {
   return result;
 }
 
-boost::optional<std::pair<std::string, time_t>>
-DefaultCacheImpl::GetFromDiscCache(const std::string& key) {
+bool DefaultCacheImpl::GetFromDiskCache(const std::string& key,
+                                        KeyValueCache::ValueTypePtr& value,
+                                        time_t& expiry) {
+  // Make sure we do not get a dirty entry
+  value = nullptr;
+  expiry = KeyValueCache::kDefaultExpiry;
+
   if (protected_cache_) {
-    auto result = protected_cache_->Get(key);
-    if (result) {
-      auto default_expiry = KeyValueCache::kDefaultExpiry;
-      return std::make_pair(std::move(result.value()), default_expiry);
+    auto result = protected_cache_->Get(key, value);
+    if (result && value && !value->empty()) {
+      return true;
     }
   }
 
   if (mutable_cache_) {
     auto expiry = GetRemainingExpiryTime(key, *mutable_cache_);
-    if (expiry <= 0) {
-      uint64_t removed_data_size = 0u;
-      PurgeDiskItem(key, *mutable_cache_, removed_data_size);
-      mutable_cache_data_size_ -= removed_data_size;
-
-      RemoveKeyLru(key);
-    } else {
+    if (expiry > 0) {
+      // Entry didn't expire yet, we can still use it
       if (!PromoteKeyLru(key)) {
         // If not found in LRU no need to look in disk cache either.
-        OLP_SDK_LOG_DEBUG_F(
-            kLogTag, "Key is not found LRU mutable cache: key %s", key.c_str());
-        return boost::none;
+        OLP_SDK_LOG_DEBUG_F(kLogTag, "Key not found in LRU, key='%s'",
+                            key.c_str());
+        return false;
       }
 
-      auto result = mutable_cache_->Get(key);
-      if (result) {
-        return std::make_pair(std::move(result.value()), expiry);
-      }
+      auto result = mutable_cache_->Get(key, value);
+      return result && value;
     }
+
+    // Data expired in cache -> remove
+    uint64_t removed_data_size = 0u;
+    PurgeDiskItem(key, *mutable_cache_, removed_data_size);
+    mutable_cache_data_size_ -= removed_data_size;
+    RemoveKeyLru(key);
+  }
+
+  return false;
+}
+
+boost::optional<std::pair<std::string, time_t>>
+DefaultCacheImpl::GetFromDiscCache(const std::string& key) {
+  KeyValueCache::ValueTypePtr value = nullptr;
+  time_t expiry = KeyValueCache::kDefaultExpiry;
+  auto result = GetFromDiskCache(key, value, expiry);
+  if (result && value) {
+    return std::make_pair(std::string(value->begin(), value->end()), expiry);
   }
+
   return boost::none;
 }
 
diff --git a/olp-cpp-sdk-core/src/cache/DefaultCacheImpl.h b/olp-cpp-sdk-core/src/cache/DefaultCacheImpl.h
@@ -21,6 +21,10 @@
 
 #include "olp/core/cache/DefaultCache.h"
 
+#include <memory>
+#include <string>
+#include <utility>
+
 #include "DiskCache.h"
 #include "InMemoryCache.h"
 
@@ -52,24 +56,24 @@ class DefaultCacheImpl {
   bool RemoveKeysWithPrefix(const std::string& key);
 
  protected:
-  /// The LRU cache definition using the leveldb keys as key and the value size
+  /// Alias for the LRU cache using the leveldb keys as key and the value size
   /// as value.
   using DiskLruCache = utils::LruCache<std::string, size_t>;
 
   /// Returns LRU mutable cache, used for tests.
   const std::unique_ptr<DiskLruCache>& GetMutableCacheLru() const {
     return mutable_cache_lru_;
-  };
+  }
 
   /// Returns mutable cache, used for tests.
   const std::unique_ptr<DiskCache>& GetMutableCache() const {
     return mutable_cache_;
-  };
+  }
 
   /// Returns memory cache, used for tests.
   const std::unique_ptr<InMemoryCache>& GetMemoryCache() const {
     return memory_cache_;
-  };
+  }
 
   /// Returns mutable cache size, used for tests.
   uint64_t GetMutableCacheSize() const { return mutable_cache_data_size_; }
@@ -99,6 +103,9 @@ class DefaultCacheImpl {
 
   DefaultCache::StorageOpenResult SetupStorage();
 
+  bool GetFromDiskCache(const std::string& key,
+                        KeyValueCache::ValueTypePtr& value, time_t& expiry);
+
   boost::optional<std::pair<std::string, time_t>> GetFromDiscCache(
       const std::string& key);
 
diff --git a/olp-cpp-sdk-core/src/cache/DiskCache.cpp b/olp-cpp-sdk-core/src/cache/DiskCache.cpp
@@ -24,6 +24,7 @@
 #include <memory>
 #include <regex>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include <leveldb/db.h>
@@ -272,6 +273,30 @@ boost::optional<std::string> DiskCache::Get(const std::string& key) {
              : boost::none;
 }
 
+bool DiskCache::Get(const std::string& key,
+                    KeyValueCache::ValueTypePtr& value) {
+  if (!database_) {
+    OLP_SDK_LOG_ERROR(kLogTag, "Get: Database not initialized");
+    return false;
+  }
+
+  value = nullptr;
+  leveldb::ReadOptions options;
+  options.verify_checksums = check_crc_;
+  auto iterator = NewIterator(options);
+
+  iterator->Seek(key);
+  if (iterator->Valid() && iterator->key() == key) {
+    auto slice_value = iterator->value();
+    if (!slice_value.empty()) {
+      value = std::make_shared<KeyValueCache::ValueType>(
+          slice_value.data(), slice_value.data() + slice_value.size());
+    }
+  }
+
+  return true;
+}
+
 bool DiskCache::Remove(const std::string& key, uint64_t& removed_data_size) {
   if (!database_) {
     removed_data_size = 0;
diff --git a/olp-cpp-sdk-core/src/cache/DiskCache.h b/olp-cpp-sdk-core/src/cache/DiskCache.h
@@ -21,6 +21,7 @@
 
 #include <time.h>
 #include <functional>
+#include <limits>
 #include <map>
 #include <memory>
 #include <mutex>
@@ -33,6 +34,7 @@
 #include <leveldb/options.h>
 #include <leveldb/write_batch.h>
 #include <olp/core/cache/CacheSettings.h>
+#include <olp/core/cache/KeyValueCache.h>
 #include <olp/core/client/ApiError.h>
 #include <olp/core/client/ApiResponse.h>
 
@@ -110,8 +112,12 @@ class DiskCache {
 
   bool Put(const std::string& key, leveldb::Slice slice);
 
+  /// @deprecated Please use Get(const std::string&,
+  /// KeyValueCache::ValueTypePtr&) instead.
   boost::optional<std::string> Get(const std::string& key);
 
+  bool Get(const std::string& key, KeyValueCache::ValueTypePtr& value);
+
   /// Remove single key/value from DB.
   bool Remove(const std::string& key, uint64_t& removed_data_size);
 
diff --git a/olp-cpp-sdk-core/tests/cache/DefaultCacheImplTest.cpp b/olp-cpp-sdk-core/tests/cache/DefaultCacheImplTest.cpp
@@ -22,13 +22,13 @@
 #include <cache/DefaultCacheImpl.h>
 #include <olp/core/utils/Dir.h>
 
-using namespace olp::cache;
+namespace cache = olp::cache;
 
 namespace {
-class DefaultCacheImplHelper : public DefaultCacheImpl {
+class DefaultCacheImplHelper : public cache::DefaultCacheImpl {
  public:
-  DefaultCacheImplHelper(const CacheSettings& settings)
-      : DefaultCacheImpl(settings){};
+  explicit DefaultCacheImplHelper(const cache::CacheSettings& settings)
+      : DefaultCacheImpl(settings) {}
 
   bool HasLruCache() const { return GetMutableCacheLru().get() != nullptr; }
 
@@ -114,7 +114,7 @@ TEST(DefaultCacheImplTest, LruCache) {
 
     olp::cache::CacheSettings settings;
     settings.disk_path_mutable = cache_path;
-    settings.eviction_policy = EvictionPolicy::kLeastRecentlyUsed;
+    settings.eviction_policy = cache::EvictionPolicy::kLeastRecentlyUsed;
     DefaultCacheImplHelper cache(settings);
     cache.Open();
 
@@ -170,7 +170,7 @@ TEST(DefaultCacheImplTest, LruCache) {
 
     olp::cache::CacheSettings settings;
     settings.disk_path_mutable = cache_path;
-    settings.eviction_policy = EvictionPolicy::kNone;
+    settings.eviction_policy = cache::EvictionPolicy::kNone;
     DefaultCacheImplHelper cache(settings);
     cache.Open();
 
@@ -340,20 +340,18 @@ TEST(DefaultCacheImplTest, MutableCacheSize) {
   {
     SCOPED_TRACE("Put decode");
 
-    settings.eviction_policy = EvictionPolicy::kNone;
+    settings.eviction_policy = cache::EvictionPolicy::kNone;
     DefaultCacheImplHelper cache(settings);
     cache.Open();
     cache.Clear();
 
-    cache.Put(
-        key1, data_string, [=]() { return data_string; },
-        (std::numeric_limits<time_t>::max)());
+    cache.Put(key1, data_string, [=]() { return data_string; },
+              (std::numeric_limits<time_t>::max)());
     auto data_size = key1.size() + data_string.size();
 
     EXPECT_EQ(data_size, cache.Size());
 
-    cache.Put(
-        key2, data_string, [=]() { return data_string; }, expiry);
+    cache.Put(key2, data_string, [=]() { return data_string; }, expiry);
     data_size +=
         key2.size() + data_string.size() + cache.CalculateExpirySize(key2);
 
@@ -363,7 +361,7 @@ TEST(DefaultCacheImplTest, MutableCacheSize) {
   {
     SCOPED_TRACE("Put binary");
 
-    settings.eviction_policy = EvictionPolicy::kNone;
+    settings.eviction_policy = cache::EvictionPolicy::kNone;
     DefaultCacheImplHelper cache(settings);
     cache.Open();
     cache.Clear();
@@ -388,9 +386,8 @@ TEST(DefaultCacheImplTest, MutableCacheSize) {
     cache.Clear();
 
     cache.Put(key1, data_ptr, (std::numeric_limits<time_t>::max)());
-    cache.Put(
-        key2, data_string, [=]() { return data_string; },
-        (std::numeric_limits<time_t>::max)());
+    cache.Put(key2, data_string, [=]() { return data_string; },
+              (std::numeric_limits<time_t>::max)());
 
     cache.Remove(key1);
     cache.Remove(key2);
@@ -407,8 +404,7 @@ TEST(DefaultCacheImplTest, MutableCacheSize) {
     cache.Clear();
 
     cache.Put(key1, data_ptr, expiry);
-    cache.Put(
-        key2, data_string, [=]() { return data_string; }, expiry);
+    cache.Put(key2, data_string, [=]() { return data_string; }, expiry);
 
     cache.Remove(key1);
     cache.Remove(key2);
@@ -426,8 +422,7 @@ TEST(DefaultCacheImplTest, MutableCacheSize) {
 
     cache.Put(key1, data_ptr, (std::numeric_limits<time_t>::max)());
     cache.Put(key2, data_ptr, expiry);
-    cache.Put(
-        key3, data_string, [=]() { return data_string; }, expiry);
+    cache.Put(key3, data_string, [=]() { return data_string; }, expiry);
     const auto data_size =
         key3.size() + data_string.size() + cache.CalculateExpirySize(key3);
 
@@ -445,8 +440,7 @@ TEST(DefaultCacheImplTest, MutableCacheSize) {
     cache.Clear();
 
     cache.Put(key1, data_ptr, -1);
-    cache.Put(
-        key2, data_string, [=]() { return data_string; }, -1);
+    cache.Put(key2, data_string, [=]() { return data_string; }, -1);
 
     cache.Get(key1);
     cache.Get(key2, [](const std::string& value) { return value; });
@@ -459,15 +453,14 @@ TEST(DefaultCacheImplTest, MutableCacheSize) {
     const std::string data_string{"this is key's data"};
     const std::string key{"somekey"};
 
-    settings.eviction_policy = EvictionPolicy::kNone;
+    settings.eviction_policy = cache::EvictionPolicy::kNone;
     DefaultCacheImplHelper cache(settings);
 
     cache.Open();
     cache.Clear();
 
-    cache.Put(
-        key, data_string, [=]() { return data_string; },
-        (std::numeric_limits<time_t>::max)());
+    cache.Put(key, data_string, [=]() { return data_string; },
+              (std::numeric_limits<time_t>::max)());
     const auto data_size = key.size() + data_string.size();
     cache.Close();
     EXPECT_EQ(0u, cache.Size());
@@ -487,7 +480,7 @@ TEST(DefaultCacheImplTest, MutableCacheSize) {
     std::vector<unsigned char> binary_data(data_size);
     olp::cache::CacheSettings settings;
     settings.disk_path_mutable = cache_path;
-    settings.eviction_policy = EvictionPolicy::kNone;
+    settings.eviction_policy = cache::EvictionPolicy::kNone;
     settings.max_disk_storage = 2u * 1024u * 1024u;
     DefaultCacheImplHelper cache(settings);
 
@@ -543,7 +536,7 @@ TEST(DefaultCacheImplTest, LruCacheEviction) {
     std::vector<unsigned char> binary_data(data_size);
     olp::cache::CacheSettings settings;
     settings.disk_path_mutable = cache_path;
-    settings.eviction_policy = EvictionPolicy::kNone;
+    settings.eviction_policy = cache::EvictionPolicy::kNone;
     settings.max_disk_storage = 2u * 1024u * 1024u;
     DefaultCacheImplHelper cache(settings);
 
@@ -590,7 +583,7 @@ TEST(DefaultCacheImplTest, LruCacheEviction) {
     std::vector<unsigned char> binary_data(data_size);
     olp::cache::CacheSettings settings;
     settings.disk_path_mutable = cache_path;
-    settings.eviction_policy = EvictionPolicy::kLeastRecentlyUsed;
+    settings.eviction_policy = cache::EvictionPolicy::kLeastRecentlyUsed;
     settings.max_disk_storage = 2u * 1024u * 1024u;
     DefaultCacheImplHelper cache(settings);
 
