Switch to a QueueMap implementation with less indirections (directly store elements instead of pointers)

implementations/caching/Cache.cpp

@@ -27,7 +27,7 @@ Cache::~Cache() {
 unique_ptr<Block> Cache::pop(const Key &key) {
   lock_guard<mutex> lock(_mutex);
   auto found = _cachedBlocks.pop(key);
-  if (found.get() == nullptr) {
+  if (!found) {
     return nullptr;
   }
   auto block = found->releaseBlock();
@@ -42,7 +42,7 @@ void Cache::push(unique_ptr<Block> block) {
     assert(_cachedBlocks.size() == MAX_ENTRIES-1);
   }
   Key key = block->key();
-  _cachedBlocks.push(key, make_unique<CacheEntry>(std::move(block)));
+  _cachedBlocks.push(key, std::move(block));
 }
 
 void Cache::_popOldEntries() {

implementations/caching/Cache.h

@@ -31,7 +31,6 @@ public:
 private:
   void _popOldEntries();
 
-
   mutable std::mutex _mutex;
   QueueMap<Key, CacheEntry> _cachedBlocks;
   std::unique_ptr<PeriodicTask> _timeoutFlusher;

implementations/caching/QueueMap.h

@@ -5,6 +5,8 @@
 #include <memory>
 #include <unordered_map>
 #include <cassert>
+#include <boost/optional.hpp>
+#include <messmer/cpp-utils/macros.h>
 
 namespace blockstore {
 namespace caching {
@@ -14,35 +16,43 @@ namespace caching {
 template<class Key, class Value>
 class QueueMap {
 public:
-  QueueMap(): _entries(), _sentinel(nullptr, nullptr, &_sentinel, &_sentinel) {
+  QueueMap(): _entries(), _sentinel(&_sentinel, &_sentinel) {
+  }
+  virtual ~QueueMap() {
+    for (auto &entry : _entries) {
+      entry.second.release();
     }
-  virtual ~QueueMap() {}
-
-  void push(const Key &key, std::unique_ptr<Value> value) {
-    auto newEntry = std::make_unique<Entry>(&key, std::move(value), _sentinel.prev, &_sentinel);
-    _sentinel.prev->next = newEntry.get();
-    _sentinel.prev = newEntry.get();
-    auto insertResult = _entries.emplace(key, std::move(newEntry));
-    assert(insertResult.second == true);
   }
 
-  std::unique_ptr<Value> pop(const Key &key) {
+  void push(const Key &key, Value value) {
+    auto newEntry = _entries.emplace(std::piecewise_construct, std::forward_as_tuple(key), std::forward_as_tuple(_sentinel.prev, &_sentinel));
+    assert(newEntry.second == true);
+    newEntry.first->second.init(&newEntry.first->first, std::move(value));
+    //The following is ok, because std::unordered_map never invalidates pointers to its entries
+    _sentinel.prev->next = &newEntry.first->second;
+    _sentinel.prev = &newEntry.first->second;
+  }
+
+  boost::optional<Value> pop(const Key &key) {
     auto found = _entries.find(key);
     if (found == _entries.end()) {
-      return nullptr;
+      return boost::none;
     }
-    _removeFromQueue(found->second.get());
-    auto value = std::move(found->second->value);
+    _removeFromQueue(found->second);
+    auto value = found->second.release();
     _entries.erase(found);
-    return value;
+    return std::move(value);
   }
 
-  std::unique_ptr<Value> pop() {
+  boost::optional<Value> pop() {
+    if(_sentinel.next == &_sentinel) {
+      return boost::none;
+    }
     return pop(*_sentinel.next->key);
   }
 
   const Value &peek() {
-	return *_sentinel.next->value;
+    return _sentinel.next->value();
   }
 
   uint32_t size() {
@@ -50,26 +60,39 @@ public:
   }
 
 private:
-  struct Entry {
-    Entry(const Key *key_, std::unique_ptr<Value> value_, Entry *prev_, Entry *next_): key(nullptr), value(std::move(value_)), prev(prev_), next(next_) {
-      if (key_ != nullptr) {
-        key = std::make_unique<Key>(*key_);
+  class Entry {
+  public:
+    Entry(Entry *prev_, Entry *next_): prev(prev_), next(next_), key(nullptr), _value() {
     }
+    void init(const Key *key_, Value value_) {
+      key = key_;
+      new(_value) Value(std::move(value_));
+    }
+    Value release() {
+      Value value = std::move(*reinterpret_cast<Value*>(_value));
+      reinterpret_cast<Value*>(_value)->~Value();
+      return value;
+    }
+    const Value &value() {
+      return *reinterpret_cast<Value*>(_value);
     }
-    std::unique_ptr<Key> key;
-    std::unique_ptr<Value> value;
     Entry *prev;
     Entry *next;
+    const Key *key;
+  private:
+    alignas(Value) char _value[sizeof(Value)];
+    DISALLOW_COPY_AND_ASSIGN(Entry);
   };
 
-  void _removeFromQueue(Entry *entry) {
-    entry->prev->next = entry->next;
-    entry->next->prev = entry->prev;
+  void _removeFromQueue(const Entry &entry) {
+    entry.prev->next = entry.next;
+    entry.next->prev = entry.prev;
   }
 
-  //TODO Double indirection unique_ptr<Entry> and Entry has unique_ptr<Value>. Necessary?
-  std::unordered_map<Key, std::unique_ptr<Entry>> _entries;
+  std::unordered_map<Key, Entry> _entries;
   Entry _sentinel;
+
+  DISALLOW_COPY_AND_ASSIGN(QueueMap);
 };
 
 }

test/implementations/caching/QueueMapTest.cpp

@@ -0,0 +1,137 @@
+#include <google/gtest/gtest.h>
+#include <messmer/cpp-utils/macros.h>
+#include "../../../implementations/caching/QueueMap.h"
+
+using ::testing::Test;
+
+using namespace blockstore::caching;
+
+// This is a not-default-constructible Key type
+class MinimalKeyType {
+public:
+  static MinimalKeyType create() {
+    return MinimalKeyType();
+  }
+  bool operator==(const MinimalKeyType &rhs) const {
+    return true;
+  }
+private:
+  MinimalKeyType() {
+  }
+};
+namespace std {
+template <> struct hash<MinimalKeyType> {
+  size_t operator()(const MinimalKeyType &obj) const {
+    return 0;
+  }
+};
+}
+// This is a not-default-constructible non-copyable but moveable Value type
+class MinimalValueType {
+public:
+  static MinimalValueType create() {
+    return MinimalValueType();
+  }
+  MinimalValueType(MinimalValueType &&rhs) = default;
+private:
+  MinimalValueType() {
+  }
+  DISALLOW_COPY_AND_ASSIGN(MinimalValueType);
+};
+
+class QueueMapTest: public Test {
+public:
+  QueueMap<int, int> map;
+};
+
+TEST_F(QueueMapTest, TypeConstraints) {
+  QueueMap<MinimalKeyType, MinimalValueType> obj;
+  //Call all functions to ensure they still work
+  obj.push(MinimalKeyType::create(), MinimalValueType::create());
+  obj.peek();
+  obj.pop(MinimalKeyType::create());
+  obj.push(MinimalKeyType::create(), MinimalValueType::create());
+  obj.pop();
+  obj.size();
+}
+
+TEST_F(QueueMapTest, Size_Empty) {
+  EXPECT_EQ(0, map.size());
+}
+
+TEST_F(QueueMapTest, Size_AfterPushingOne) {
+  map.push(2, 3);
+  EXPECT_EQ(1, map.size());
+}
+
+TEST_F(QueueMapTest, Size_AfterPushingTwo) {
+  map.push(2, 3);
+  map.push(3, 4);
+  EXPECT_EQ(2, map.size());
+}
+
+TEST_F(QueueMapTest, Size_AfterPushingTwoAndPoppingOldest) {
+  map.push(2, 3);
+  map.push(3, 4);
+  map.pop();
+  EXPECT_EQ(1, map.size());
+}
+
+TEST_F(QueueMapTest, Size_AfterPushingTwoAndPoppingFirst) {
+  map.push(2, 3);
+  map.push(3, 4);
+  map.pop(2);
+  EXPECT_EQ(1, map.size());
+}
+
+TEST_F(QueueMapTest, Size_AfterPushingTwoAndPoppingLast) {
+  map.push(2, 3);
+  map.push(3, 4);
+  map.pop(3);
+  EXPECT_EQ(1, map.size());
+}
+
+TEST_F(QueueMapTest, Size_AfterPushingOnePoppingOne) {
+  map.push(2, 3);
+  map.pop();
+  EXPECT_EQ(0, map.size());
+}
+
+TEST_F(QueueMapTest, Size_AfterPushingOnePoppingOnePerKey) {
+  map.push(2, 3);
+  map.pop(2);
+  EXPECT_EQ(0, map.size());
+}
+
+TEST_F(QueueMapTest, Size_AfterPushingOnePoppingOnePushingOne) {
+  map.push(2, 3);
+  map.pop();
+  map.push(3, 4);
+  EXPECT_EQ(1, map.size());
+}
+
+TEST_F(QueueMapTest, Size_AfterPushingOnePoppingOnePerKeyPushingOne) {
+  map.push(2, 3);
+  map.pop(2);
+  map.push(3, 4);
+  EXPECT_EQ(1, map.size());
+}
+
+TEST_F(QueueMapTest, Size_AfterPushingOnePoppingOnePushingSame) {
+  map.push(2, 3);
+  map.pop();
+  map.push(2, 3);
+  EXPECT_EQ(1, map.size());
+}
+
+TEST_F(QueueMapTest, Size_AfterPushingOnePoppingOnePerKeyPushingSame) {
+  map.push(2, 3);
+  map.pop(2);
+  map.push(2, 3);
+  EXPECT_EQ(1, map.size());
+}
+
+//TODO Pushing the same key twice
+//TODO Popping from empty
+//TODO Popping invalid key
+//TODO Test that in all cases, destructors of Value are called correctly in QueueMap when [a] pop() [b] pop(key) [c] ~QueueMap()