From e177c6f45c3a880e7c7c729c836068c5c50d2e78 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Mon, 27 Apr 2015 01:22:39 +0200 Subject: [PATCH] Switch to a QueueMap implementation with less indirections (directly store elements instead of pointers) --- implementations/caching/Cache.cpp | 4 +- implementations/caching/Cache.h | 1 - implementations/caching/QueueMap.h | 79 ++++++---- test/implementations/caching/QueueMapTest.cpp | 137 ++++++++++++++++++ 4 files changed, 190 insertions(+), 31 deletions(-) create mode 100644 test/implementations/caching/QueueMapTest.cpp diff --git a/implementations/caching/Cache.cpp b/implementations/caching/Cache.cpp index d3a9dba1..050dbc40 100644 --- a/implementations/caching/Cache.cpp +++ b/implementations/caching/Cache.cpp @@ -27,7 +27,7 @@ Cache::~Cache() { unique_ptr Cache::pop(const Key &key) { lock_guard 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) { assert(_cachedBlocks.size() == MAX_ENTRIES-1); } Key key = block->key(); - _cachedBlocks.push(key, make_unique(std::move(block))); + _cachedBlocks.push(key, std::move(block)); } void Cache::_popOldEntries() { diff --git a/implementations/caching/Cache.h b/implementations/caching/Cache.h index 740e9003..8ff93ae6 100644 --- a/implementations/caching/Cache.h +++ b/implementations/caching/Cache.h @@ -31,7 +31,6 @@ public: private: void _popOldEntries(); - mutable std::mutex _mutex; QueueMap _cachedBlocks; std::unique_ptr _timeoutFlusher; diff --git a/implementations/caching/QueueMap.h b/implementations/caching/QueueMap.h index 758b2bd4..d21b31cf 100644 --- a/implementations/caching/QueueMap.h +++ b/implementations/caching/QueueMap.h @@ -5,6 +5,8 @@ #include #include #include +#include +#include namespace blockstore { namespace caching { @@ -14,35 +16,43 @@ namespace caching { template class QueueMap { public: - QueueMap(): _entries(), _sentinel(nullptr, nullptr, &_sentinel, &_sentinel) { + QueueMap(): _entries(), _sentinel(&_sentinel, &_sentinel) { } - virtual ~QueueMap() {} - - void push(const Key &key, std::unique_ptr value) { - auto newEntry = std::make_unique(&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); + virtual ~QueueMap() { + for (auto &entry : _entries) { + entry.second.release(); + } } - std::unique_ptr 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 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 pop() { + boost::optional 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_, Entry *prev_, Entry *next_): key(nullptr), value(std::move(value_)), prev(prev_), next(next_) { - if (key_ != nullptr) { - key = std::make_unique(*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)); + reinterpret_cast(_value)->~Value(); + return value; + } + const Value &value() { + return *reinterpret_cast(_value); } - std::unique_ptr key; - std::unique_ptr 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 and Entry has unique_ptr. Necessary? - std::unordered_map> _entries; + std::unordered_map _entries; Entry _sentinel; + + DISALLOW_COPY_AND_ASSIGN(QueueMap); }; } diff --git a/test/implementations/caching/QueueMapTest.cpp b/test/implementations/caching/QueueMapTest.cpp new file mode 100644 index 00000000..90447d0a --- /dev/null +++ b/test/implementations/caching/QueueMapTest.cpp @@ -0,0 +1,137 @@ +#include +#include +#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 { + 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 map; +}; + +TEST_F(QueueMapTest, TypeConstraints) { + QueueMap 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()