From 048e0a33eebc0e1ebd843bbc0fbbec4f178ece19 Mon Sep 17 00:00:00 2001 From: Pavel Kirienko Date: Fri, 15 May 2015 21:32:08 +0300 Subject: [PATCH] Non-moving multiset, tests are failing in C++03 mode --- libuavcan/include/uavcan/util/map.hpp | 1 + libuavcan/include/uavcan/util/multiset.hpp | 440 +++++++++------------ libuavcan/test/util/multiset.cpp | 82 ++-- 3 files changed, 238 insertions(+), 285 deletions(-) diff --git a/libuavcan/include/uavcan/util/map.hpp b/libuavcan/include/uavcan/util/map.hpp index dcbdf638a0..ed5535266d 100644 --- a/libuavcan/include/uavcan/util/map.hpp +++ b/libuavcan/include/uavcan/util/map.hpp @@ -11,6 +11,7 @@ #include #include #include +#include namespace uavcan { diff --git a/libuavcan/include/uavcan/util/multiset.hpp b/libuavcan/include/uavcan/util/multiset.hpp index 7cce1e2a17..1c62dd3fdf 100644 --- a/libuavcan/include/uavcan/util/multiset.hpp +++ b/libuavcan/include/uavcan/util/multiset.hpp @@ -11,45 +11,62 @@ #include #include #include +#include + +#if !defined(UAVCAN_CPP_VERSION) || !defined(UAVCAN_CPP11) +# error UAVCAN_CPP_VERSION +#endif namespace uavcan { /** - * Slow but memory efficient unordered set. + * Slow but memory efficient unordered multiset. Unlike Map<>, this container does not move objects, so + * they don't have to be copyable. * * Items can be allocated in a static buffer or in the node's memory pool if the static buffer is exhausted. - * When an item is deleted from the static buffer, one pair from the memory pool will be moved in the free - * slot of the static buffer, so the use of the memory pool is minimized. - * - * Please be aware that this container does not perform any speed optimizations to minimize memory footprint, - * so the complexity of most operations is O(N). - * - * Type requirements: - * T must be copyable, assignable and default constructible. - * T must implement a comparison operator. - * T's default constructor must initialize the object into invalid state. - * Size of T must not exceed MemPoolBlockSize. */ template class UAVCAN_EXPORT MultisetBase : Noncopyable { - template friend class Multiset; - protected: - /* - * Purpose of this type is to enforce default initialization of T - */ - struct Item + struct Item : ::uavcan::Noncopyable { - T value; - Item() : value() { } - Item(const T& v) : value(v) { } - bool operator==(const Item& rhs) const { return rhs.value == value; } - bool operator!=(const Item& rhs) const { return !operator==(rhs); } - operator T() const { return value; } + T* ptr; + +#if UAVCAN_CPP_VERSION >= UAVCAN_CPP11 + alignas(T) unsigned char pool[sizeof(T)]; ///< Memory efficient version +#else + union + { + unsigned char pool[sizeof(T)]; + long double _aligner1_; + long long _aligner2_; + }; +#endif + + Item() + : ptr(NULL) + { + fill_n(pool, sizeof(pool), static_cast(0)); + } + + ~Item() { destroy(); } + + bool isConstructed() const { return ptr != NULL; } + + void destroy() + { + if (ptr != NULL) + { + ptr->~T(); + ptr = NULL; + fill_n(pool, sizeof(pool), static_cast(0)); + } + } }; - struct Chunk : LinkedListNode +private: + struct Chunk : LinkedListNode, ::uavcan::Noncopyable { enum { NumItems = (MemPoolBlockSize - sizeof(LinkedListNode)) / sizeof(Item) }; Item items[NumItems]; @@ -58,7 +75,7 @@ protected: { StaticAssert<(static_cast(NumItems) > 0)>::check(); IsDynamicallyAllocatable::check(); - UAVCAN_ASSERT(items[0].value == T()); + UAVCAN_ASSERT(!items[0].isConstructed()); } static Chunk* instantiate(IPoolAllocator& allocator) @@ -81,11 +98,11 @@ protected: } } - Item* find(const Item& item) + Item* findFreeSlot() { for (unsigned i = 0; i < static_cast(NumItems); i++) { - if (items[i] == item) + if (!items[i].isConstructed()) { return items + i; } @@ -94,7 +111,6 @@ protected: } }; -private: LinkedListRoot list_; IPoolAllocator& allocator_; #if !UAVCAN_TINY @@ -102,11 +118,8 @@ private: const unsigned num_static_entries_; #endif - Item* find(const Item& item); + Item* findOrCreateFreeSlot(); -#if !UAVCAN_TINY - void optimizeStorage(); -#endif void compact(); struct YesPredicate @@ -114,6 +127,20 @@ private: bool operator()(const T&) const { return true; } }; + struct IndexPredicate : ::uavcan::Noncopyable + { + unsigned index; + IndexPredicate(unsigned target_index) : index(target_index) { } + bool operator()(const T&) { return index--==0; } + }; + + struct ComparingPredicate + { + const T& reference; + ComparingPredicate(const T& ref) : reference(ref) { } + bool operator()(const T& sample) { return reference == sample; } + }; + protected: #if UAVCAN_TINY MultisetBase(IPoolAllocator& allocator) @@ -126,9 +153,7 @@ protected: : allocator_(allocator) , static_(static_buf) , num_static_entries_(num_static_entries) - { - UAVCAN_ASSERT(Item() == Item()); - } + { } #endif /// Derived class destructor must call removeAll(); @@ -138,17 +163,70 @@ protected: } public: + /** + * This is needed for testing. + */ + enum { NumItemsPerDynamicChunk = Chunk::NumItems }; + /** * Adds one item and returns a pointer to it. * If add fails due to lack of memory, NULL will be returned. */ - T* add(const T& item); + T* add() + { + Item* const item = findOrCreateFreeSlot(); + if (item == NULL) + { + return NULL; + } + UAVCAN_ASSERT(item->ptr == NULL); + item->ptr = new (item->pool) T(); + return item->ptr; + } + + template + T* add(P1 p1) + { + Item* const item = findOrCreateFreeSlot(); + if (item == NULL) + { + return NULL; + } + UAVCAN_ASSERT(item->ptr == NULL); + item->ptr = new (item->pool) T(p1); + return item->ptr; + } + + template + T* add(P1 p1, P2 p2) + { + Item* const item = findOrCreateFreeSlot(); + if (item == NULL) + { + return NULL; + } + UAVCAN_ASSERT(item->ptr == NULL); + item->ptr = new (item->pool) T(p1, p2); + return item->ptr; + } + + template + T* add(P1 p1, P2 p2, P3 p3) + { + Item* const item = findOrCreateFreeSlot(); + if (item == NULL) + { + return NULL; + } + UAVCAN_ASSERT(item->ptr == NULL); + item->ptr = new (item->pool) T(p1, p2, p3); + return item->ptr; + } /** - * Does nothing if there's no such item. - * Only the first matching item will be removed. + * @ref removeMatching() */ - void remove(const T& item); + enum RemoveStrategy { RemoveOne, RemoveAll }; /** * Removes entries where the predicate returns true. @@ -156,7 +234,15 @@ public: * bool (T& item) */ template - void removeWhere(Predicate predicate); + void removeMatching(Predicate predicate, RemoveStrategy strategy); + + template + void removeAllMatching(Predicate predicate) { removeMatching(predicate, RemoveAll); } + + template + void removeFirstMatching(Predicate predicate) { removeMatching(predicate, RemoveOne); } + + void removeFirst(const T& ref) { removeFirstMatching(ComparingPredicate(ref)); } /** * Returns first entry where the predicate returns true. @@ -164,28 +250,45 @@ public: * bool (const T& item) */ template - const T* findFirst(Predicate predicate) const; + T* find(Predicate predicate); + + template + const T* find(Predicate predicate) const + { + return const_cast*>(this)->find(predicate); + } /** * Removes all items; all pool memory will be released. */ - void removeAll(); + void removeAll() { removeAllMatching(YesPredicate()); } /** * Returns an item located at the specified position from the beginning. * Note that any insertion or deletion may greatly disturb internal ordering, so use with care. * If index is greater than or equal the number of items, null pointer will be returned. */ - T* getByIndex(unsigned index); - const T* getByIndex(unsigned index) const; + T* getByIndex(unsigned index) + { + IndexPredicate predicate(index); + return find(predicate); + } - bool isEmpty() const; + const T* getByIndex(unsigned index) const + { + return const_cast*>(this)->getByIndex(index); + } + + /** + * This is O(1) + */ + bool isEmpty() const { return find(YesPredicate()) == NULL; } /** * Counts number of items stored. * Best case complexity is O(N). */ - unsigned getSize() const; + unsigned getSize() const { return getNumStaticItems() + getNumDynamicItems(); } /** * For testing, do not use directly. @@ -207,9 +310,7 @@ public: // This instantiation will not be valid in UAVCAN_TINY mode explicit Multiset(IPoolAllocator& allocator) : MultisetBase(static_, NumStaticEntries, allocator) - { - UAVCAN_ASSERT(static_[0].value == T()); - } + { } ~Multiset() { this->removeAll(); } @@ -238,87 +339,42 @@ public: * MultisetBase<> */ template -typename MultisetBase::Item* MultisetBase::find(const Item& item) +typename MultisetBase::Item* MultisetBase::findOrCreateFreeSlot() { #if !UAVCAN_TINY + // Search in static pool for (unsigned i = 0; i < num_static_entries_; i++) { - if (static_[i] == item) + if (!static_[i].isConstructed()) { - return static_ + i; + return &static_[i]; } } #endif - Chunk* p = list_.get(); - while (p) + // Search in dynamic pool { - Item* const dyn = p->find(item); - if (dyn != NULL) - { - return dyn; - } - p = p->getNextListNode(); - } - return NULL; -} - -#if !UAVCAN_TINY - -template -void MultisetBase::optimizeStorage() -{ - while (true) - { - // Looking for first EMPTY static entry - Item* stat = NULL; - for (unsigned i = 0; i < num_static_entries_; i++) - { - if (static_[i] == Item()) - { - stat = static_ + i; - break; - } - } - if (stat == NULL) - { - break; - } - - // Looking for the first NON-EMPTY dynamic entry, erasing immediately Chunk* p = list_.get(); - Item dyn; - UAVCAN_ASSERT(dyn == Item()); while (p) { - bool stop = false; - for (int i = 0; i < Chunk::NumItems; i++) + Item* const dyn = p->findFreeSlot(); + if (dyn != NULL) { - if (p->items[i] != Item()) // Non empty - { - dyn = p->items[i]; // Copy by value - p->items[i] = Item(); // Erase immediately - stop = true; - break; - } - } - if (stop) - { - break; + return dyn; } p = p->getNextListNode(); } - if (dyn == Item()) - { - break; - } - - // Migrating - *stat = dyn; } -} -#endif // !UAVCAN_TINY + // Create new dynamic chunk + Chunk* const chunk = Chunk::instantiate(allocator_); + if (chunk == NULL) + { + return NULL; + } + list_.insert(chunk); + return &chunk->items[0]; +} template void MultisetBase::compact() @@ -330,7 +386,7 @@ void MultisetBase::compact() bool remove_this = true; for (int i = 0; i < Chunk::NumItems; i++) { - if (p->items[i] != Item()) + if (p->items[i].isConstructed()) { remove_this = false; break; @@ -345,103 +401,73 @@ void MultisetBase::compact() } } -template -T* MultisetBase::add(const T& value) -{ - UAVCAN_ASSERT(!(value == T())); - remove(value); - - Item* const item = find(Item()); - if (item) - { - *item = Item(value); - return &item->value; - } - - Chunk* const itemg = Chunk::instantiate(allocator_); - if (itemg == NULL) - { - return NULL; - } - list_.insert(itemg); - itemg->items[0] = Item(value); - return &itemg->items[0].value; -} - -template -void MultisetBase::remove(const T& value) -{ - UAVCAN_ASSERT(!(value == T())); - Item* const item = find(Item(value)); - if (item != NULL) - { - *item = Item(); -#if !UAVCAN_TINY - optimizeStorage(); -#endif - compact(); - } -} - template template -void MultisetBase::removeWhere(Predicate predicate) +void MultisetBase::removeMatching(Predicate predicate, const RemoveStrategy strategy) { unsigned num_removed = 0; #if !UAVCAN_TINY for (unsigned i = 0; i < num_static_entries_; i++) { - if (static_[i] != Item()) + if (static_[i].isConstructed()) { - if (predicate(static_[i].value)) + if (predicate(*static_[i].ptr)) { num_removed++; - static_[i] = Item(); + static_[i].destroy(); } } + + if ((num_removed > 0) && (strategy == RemoveOne)) + { + break; + } } #endif Chunk* p = list_.get(); while (p) { + if ((num_removed > 0) && (strategy == RemoveOne)) + { + break; + } + for (int i = 0; i < Chunk::NumItems; i++) { - const Item* const item = p->items + i; - if ((*item) != Item()) + Item& item = p->items[i]; + if (item.isConstructed()) { - if (predicate(item->value)) + if (predicate(*item.ptr)) { num_removed++; - p->items[i] = Item(); + item.destroy(); } } } + p = p->getNextListNode(); } if (num_removed > 0) { -#if !UAVCAN_TINY - optimizeStorage(); -#endif compact(); } } template template -const T* MultisetBase::findFirst(Predicate predicate) const +T* MultisetBase::find(Predicate predicate) { #if !UAVCAN_TINY for (unsigned i = 0; i < num_static_entries_; i++) { - if (static_[i] != Item()) + if (static_[i].isConstructed()) { - if (predicate(static_[i].value)) + if (predicate(*static_[i].ptr)) { - return &static_[i].value; + return static_[i].ptr; } } } @@ -452,12 +478,11 @@ const T* MultisetBase::findFirst(Predicate predicate) const { for (int i = 0; i < Chunk::NumItems; i++) { - const Item* const item = p->items + i; - if ((*item) != Item()) + if (p->items[i].isConstructed()) { - if (predicate(item->value)) + if (predicate(*p->items[i].ptr)) { - return &p->items[i].value; + return p->items[i].ptr; } } } @@ -466,70 +491,6 @@ const T* MultisetBase::findFirst(Predicate predicate) const return NULL; } -template -void MultisetBase::removeAll() -{ - removeWhere(YesPredicate()); -} - -template -T* MultisetBase::getByIndex(unsigned index) -{ -#if !UAVCAN_TINY - // Checking the static storage - for (unsigned i = 0; i < num_static_entries_; i++) - { - if (static_[i] != Item()) - { - if (index == 0) - { - return &static_[i].value; - } - index--; - } - } -#endif - - // Slowly crawling through the dynamic storage - Chunk* p = list_.get(); - while (p) - { - for (int i = 0; i < Chunk::NumItems; i++) - { - Item* const item = p->items + i; - if ((*item) != Item()) - { - if (index == 0) - { - return &item->value; - } - index--; - } - } - p = p->getNextListNode(); - } - - return NULL; -} - -template -const T* MultisetBase::getByIndex(unsigned index) const -{ - return const_cast*>(this)->getByIndex(index); -} - -template -bool MultisetBase::isEmpty() const -{ - return getSize() == 0; -} - -template -unsigned MultisetBase::getSize() const -{ - return getNumStaticItems() + getNumDynamicItems(); -} - template unsigned MultisetBase::getNumStaticItems() const { @@ -537,10 +498,7 @@ unsigned MultisetBase::getNumStaticItems() const #if !UAVCAN_TINY for (unsigned i = 0; i < num_static_entries_; i++) { - if (static_[i] != Item()) - { - num++; - } + num += static_[i].isConstructed() ? 1U : 0U; } #endif return num; @@ -555,11 +513,7 @@ unsigned MultisetBase::getNumDynamicItems() const { for (int i = 0; i < Chunk::NumItems; i++) { - const Item* const item = p->items + i; - if ((*item) != Item()) - { - num++; - } + num += p->items[i].isConstructed() ? 1U : 0U; } p = p->getNextListNode(); } diff --git a/libuavcan/test/util/multiset.cpp b/libuavcan/test/util/multiset.cpp index a6cb3c71ea..21f74500cd 100644 --- a/libuavcan/test/util/multiset.cpp +++ b/libuavcan/test/util/multiset.cpp @@ -44,7 +44,7 @@ TEST(Multiset, Basic) std::auto_ptr mset(new MultisetType(poolmgr)); // Empty - mset->remove("foo"); + mset->removeFirst("foo"); ASSERT_EQ(0, pool.getNumUsedBlocks()); ASSERT_FALSE(mset->getByIndex(0)); ASSERT_FALSE(mset->getByIndex(1)); @@ -61,54 +61,57 @@ TEST(Multiset, Basic) ASSERT_TRUE(*mset->getByIndex(0) == "1"); ASSERT_TRUE(*mset->getByIndex(1) == "2"); - // Dynamic addion + // Dynamic addition ASSERT_EQ("3", *mset->add("3")); + ASSERT_EQ("3", *mset->getByIndex(2)); ASSERT_EQ(1, pool.getNumUsedBlocks()); ASSERT_EQ("4", *mset->add("4")); - ASSERT_EQ(1, pool.getNumUsedBlocks()); // Assuming that at least 2 items fit one block + ASSERT_LE(1, pool.getNumUsedBlocks()); // One or more ASSERT_EQ(2, mset->getNumStaticItems()); ASSERT_EQ(2, mset->getNumDynamicItems()); // Making sure everything is here ASSERT_EQ("1", *mset->getByIndex(0)); ASSERT_EQ("2", *mset->getByIndex(1)); - ASSERT_EQ("3", *mset->getByIndex(2)); - ASSERT_EQ("4", *mset->getByIndex(3)); + // 2 and 3 are not tested because their placement depends on number of items per dynamic block ASSERT_FALSE(mset->getByIndex(100)); ASSERT_FALSE(mset->getByIndex(4)); + const std::string data_at_pos2 = *mset->getByIndex(2); + const std::string data_at_pos3 = *mset->getByIndex(3); + // Finding some items - ASSERT_EQ("1", *mset->findFirst(FindPredicate("1"))); - ASSERT_EQ("2", *mset->findFirst(FindPredicate("2"))); - ASSERT_EQ("3", *mset->findFirst(FindPredicate("3"))); - ASSERT_EQ("4", *mset->findFirst(FindPredicate("4"))); - ASSERT_FALSE(mset->findFirst(FindPredicate("nonexistent"))); + ASSERT_EQ("1", *mset->find(FindPredicate("1"))); + ASSERT_EQ("2", *mset->find(FindPredicate("2"))); + ASSERT_EQ("3", *mset->find(FindPredicate("3"))); + ASSERT_EQ("4", *mset->find(FindPredicate("4"))); + ASSERT_FALSE(mset->find(FindPredicate("nonexistent"))); - // Removing one static - mset->remove("1"); // One of dynamics now migrates to the static storage - mset->remove("foo"); // There's no such thing anyway - ASSERT_EQ(1, pool.getNumUsedBlocks()); - ASSERT_EQ(2, mset->getNumStaticItems()); - ASSERT_EQ(1, mset->getNumDynamicItems()); + // Removing one static; ordering will be preserved + mset->removeFirst("1"); + mset->removeFirst("foo"); // There's no such thing anyway + ASSERT_LE(1, pool.getNumUsedBlocks()); + ASSERT_EQ(1, mset->getNumStaticItems()); + ASSERT_EQ(2, mset->getNumDynamicItems()); // This container does not move items - // Ordering has not changed - first dynamic entry has moved to the first static slot - ASSERT_EQ("3", *mset->getByIndex(0)); - ASSERT_EQ("2", *mset->getByIndex(1)); - ASSERT_EQ("4", *mset->getByIndex(2)); + // Ordering has not changed + ASSERT_EQ("2", *mset->getByIndex(0)); // Entry "1" was here + ASSERT_EQ(data_at_pos2, *mset->getByIndex(1)); + ASSERT_EQ(data_at_pos3, *mset->getByIndex(2)); // Removing another static - mset->remove("2"); - ASSERT_EQ(2, mset->getNumStaticItems()); - ASSERT_EQ(0, mset->getNumDynamicItems()); - ASSERT_EQ(0, pool.getNumUsedBlocks()); // No dynamic entries left + mset->removeFirst("2"); + ASSERT_EQ(0, mset->getNumStaticItems()); + ASSERT_EQ(2, mset->getNumDynamicItems()); + ASSERT_LE(1, pool.getNumUsedBlocks()); - // Adding some new dynamics + // Adding some new items unsigned max_value_integer = 0; for (int i = 0; i < 100; i++) { const std::string value = toString(i); - std::string* res = mset->add(value); // Will override some from the above + std::string* res = mset->add(value); // Will NOT override above if (res == NULL) { ASSERT_LT(2, i); @@ -121,25 +124,18 @@ TEST(Multiset, Basic) max_value_integer = unsigned(i); } std::cout << "Max value: " << max_value_integer << std::endl; - ASSERT_LT(4, max_value_integer); // Making sure there is true OOM ASSERT_EQ(0, pool.getNumFreeBlocks()); ASSERT_FALSE(mset->add("nonexistent")); // Removing odd values - nearly half of them - ASSERT_EQ(2, mset->getNumStaticItems()); - const unsigned num_dynamics_old = mset->getNumDynamicItems(); - mset->removeWhere(oddValuePredicate); - ASSERT_EQ(2, mset->getNumStaticItems()); - const unsigned num_dynamics_new = mset->getNumDynamicItems(); - std::cout << "Num of dynamic pairs reduced from " << num_dynamics_old << " to " << num_dynamics_new << std::endl; - ASSERT_LT(num_dynamics_new, num_dynamics_old); + mset->removeAllMatching(oddValuePredicate); // Making sure there's no odd values left for (unsigned kv_int = 0; kv_int <= max_value_integer; kv_int++) { - const std::string* val = mset->findFirst(FindPredicate(toString(kv_int))); + const std::string* val = mset->find(FindPredicate(toString(kv_int))); if (val) { ASSERT_FALSE(kv_int & 1); @@ -160,16 +156,17 @@ TEST(Multiset, NoStatic) { using uavcan::Multiset; - static const int POOL_BLOCKS = 3; - uavcan::PoolAllocator pool; + uavcan::PoolAllocator<1024, 128> pool; // Large enough to keep everything uavcan::PoolManager<2> poolmgr; poolmgr.addPool(&pool); typedef Multiset MultisetType; std::auto_ptr mset(new MultisetType(poolmgr)); + ASSERT_LE(2, MultisetType::NumItemsPerDynamicChunk); + // Empty - mset->remove("foo"); + mset->removeFirst("foo"); ASSERT_EQ(0, pool.getNumUsedBlocks()); ASSERT_FALSE(mset->getByIndex(0)); @@ -192,16 +189,17 @@ TEST(Multiset, PrimitiveKey) { using uavcan::Multiset; - static const int POOL_BLOCKS = 3; - uavcan::PoolAllocator pool; + uavcan::PoolAllocator<1024, 128> pool; // Large enough to keep everything uavcan::PoolManager<2> poolmgr; poolmgr.addPool(&pool); - typedef Multiset MultisetType; + typedef Multiset MultisetType; std::auto_ptr mset(new MultisetType(poolmgr)); + ASSERT_LE(2, MultisetType::NumItemsPerDynamicChunk); + // Empty - mset->remove(8); + mset->removeFirst(8); ASSERT_EQ(0, pool.getNumUsedBlocks()); ASSERT_EQ(0, mset->getSize()); ASSERT_FALSE(mset->getByIndex(0));