From d713875a1748f20db89ed97a83b1d447a45afb30 Mon Sep 17 00:00:00 2001 From: Wouter van Kleunen Date: Sat, 28 Nov 2020 10:29:04 +0100 Subject: [PATCH 1/2] Fix and cleanup retained map test --- test/retained_topic_map.cpp | 55 +++++++++++++++++++++++-------------- test/retained_topic_map.hpp | 31 +++++++++++++++------ test/subscription_map.hpp | 8 +++++- 3 files changed, 64 insertions(+), 30 deletions(-) diff --git a/test/retained_topic_map.cpp b/test/retained_topic_map.cpp index d005fb3d0..00ba239cb 100644 --- a/test/retained_topic_map.cpp +++ b/test/retained_topic_map.cpp @@ -3,6 +3,7 @@ // Distributed under the Boost Software License, Version 1.0. // (See accompanying file LICENSE_1_0.txt or copy at // http://www.boost.org/LICENSE_1_0.txt) + #include "test_main.hpp" #include "combi_test.hpp" #include "checker.hpp" @@ -11,6 +12,8 @@ #include "retained_topic_map.hpp" #include +#include +#include BOOST_AUTO_TEST_SUITE(test_retained_map) @@ -206,39 +209,49 @@ BOOST_AUTO_TEST_CASE(erase_upper_first) { } } -#if 0 - BOOST_AUTO_TEST_CASE(large_number_of_topics) { - retained_topic_map map; + retained_topic_map< std::pair > map; - constexpr std::size_t num_topics = 10000; + std::vector< std::pair > > created_topics; + + constexpr std::size_t num_topics = 25; for (std::size_t i = 0; i < num_topics; ++i) { - map.insert_or_assign((boost::format("topic/%d") % i).str(), i); - } + for (std::size_t j = 0; j < num_topics; ++j) { + std::string topic = (boost::format("topic/first_level_%d/second_level_%d") % i % j).str(); + auto value = std::make_pair(i, j); - BOOST_TEST(map.size() == num_topics); - try { - for (std::size_t i = 0; i < num_topics; ++i) { - map.find( - (boost::format("topic/%d") % i).str(), - [&](std::size_t value) { - if (value != i) throw false; - } - ); + map.insert_or_assign(topic, value); + + created_topics.push_back(std::make_pair(topic, value)); } } - catch (bool) { - BOOST_TEST(false); + + BOOST_TEST(map.size() == num_topics * num_topics); + + std::vector< std::pair > received_values; + std::vector< std::pair > searched_values; + + std::shuffle(created_topics.begin(), created_topics.end(), std::default_random_engine(0x12345)); + + for (auto const& i : created_topics) { + map.find( + i.first, + [&](std::pair const& value) { + received_values.push_back(value); + } + ); + + searched_values.push_back(i.second); } - for (std::size_t i = 0; i < num_topics; ++i) { - map.erase((boost::format("topic/%d") % i).str()); + BOOST_TEST(searched_values == received_values); + + for (auto const& i : created_topics) { + map.erase(i.first); } BOOST_TEST(map.size() == 0); BOOST_TEST(map.internal_size() == 1); } -#endif - BOOST_AUTO_TEST_SUITE_END() diff --git a/test/retained_topic_map.hpp b/test/retained_topic_map.hpp index a7d2fd834..cfca2407d 100644 --- a/test/retained_topic_map.hpp +++ b/test/retained_topic_map.hpp @@ -23,6 +23,11 @@ namespace mi = boost::multi_index; template class retained_topic_map { + + // Exceptions used + static void throw_max_stored_topics() { throw std::overflow_error("Retained map maximum number of topics reached"); } + static void throw_no_wildcards_allowed() { throw std::runtime_error("Retained map no wildcards allowed in retained topic name"); } + using node_id_t = std::size_t; static constexpr node_id_t root_parent_id = 0; @@ -37,9 +42,23 @@ class retained_topic_map { node_id_t id; std::size_t count = 1; - static constexpr std::size_t max_count = std::numeric_limits::max(); + // Increase the count for this node + void increase_count() { + if(count == max_count) { + throw_max_stored_topics(); + } + + ++count; + } + + // Decrease the count for this node + void decrease_count() { + BOOST_ASSERT(count >= count); + --count; + } + MQTT_NS::optional value; path_entry(node_id_t parent_id, MQTT_NS::string_view name, node_id_t id) @@ -98,7 +117,7 @@ class retained_topic_map { } } else { - direct_index.modify(entry, [](path_entry &entry){ ++entry.count; }); + direct_index.modify(entry, [](path_entry &entry){ entry.increase_count(); }); } parent = entry; @@ -230,7 +249,7 @@ class retained_topic_map { // Do iterators stay valid when erasing ? I think they do ? for (auto entry : path) { - direct_index.modify(entry, [](path_entry &entry){ --entry.count; }); + direct_index.modify(entry, [](path_entry &entry){ entry.decrease_count(); }); if (entry->count == 0) { map.erase(entry); @@ -248,14 +267,10 @@ class retained_topic_map { auto& direct_index = map.template get(); for(auto& i : path) { - direct_index.modify(i, [](path_entry &entry){ ++entry.count; }); + direct_index.modify(i, [](path_entry &entry){ entry.increase_count(); }); } } - // Exceptions used - static void throw_max_stored_topics() { throw std::overflow_error("Retained map maximum number of topics reached"); } - static void throw_no_wildcards_allowed() { throw std::runtime_error("Retained map no wildcards allowed in retained topic name"); } - // Increase the map size (total number of topics stored) void increase_map_size() { if(map_size == std::numeric_limits::max()) { diff --git a/test/subscription_map.hpp b/test/subscription_map.hpp index 3d2e90a3d..db4f2e0c4 100644 --- a/test/subscription_map.hpp +++ b/test/subscription_map.hpp @@ -202,6 +202,12 @@ class subscription_map_base { count.increment_value(); } + // Decrease the subscription count for a specific node + static void decrease_count_storage(count_storage_t &count) { + BOOST_ASSERT(count.value() > 0); + count.decrement_value(); + } + using this_type = subscription_map_base; // Use boost hash to hash pair in path_entry_key @@ -311,7 +317,7 @@ class subscription_map_base { remove_hash_child_flag = false; } - entry->second.count.decrement_value(); + decrease_count_storage(entry->second.count); if (entry->second.count.value() == 0) { remove_plus_child_flag = (entry->first.second == "+"); remove_hash_child_flag = (entry->first.second == "#"); From 420714feeff194d4c77f7d1946589dcc3210f337 Mon Sep 17 00:00:00 2001 From: Wouter van Kleunen <230050+kleunen@users.noreply.github.com> Date: Sun, 29 Nov 2020 10:37:34 +0100 Subject: [PATCH 2/2] Apply suggestions from code review Co-authored-by: Takatoshi Kondo --- test/retained_topic_map.cpp | 2 +- test/retained_topic_map.hpp | 8 ++++---- test/subscription_map.hpp | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/test/retained_topic_map.cpp b/test/retained_topic_map.cpp index 00ba239cb..3a85ab121 100644 --- a/test/retained_topic_map.cpp +++ b/test/retained_topic_map.cpp @@ -210,7 +210,7 @@ BOOST_AUTO_TEST_CASE(erase_upper_first) { } BOOST_AUTO_TEST_CASE(large_number_of_topics) { - retained_topic_map< std::pair > map; + retained_topic_map> map; std::vector< std::pair > > created_topics; diff --git a/test/retained_topic_map.hpp b/test/retained_topic_map.hpp index cfca2407d..7de2edf0c 100644 --- a/test/retained_topic_map.hpp +++ b/test/retained_topic_map.hpp @@ -46,7 +46,7 @@ class retained_topic_map { // Increase the count for this node void increase_count() { - if(count == max_count) { + if (count == max_count) { throw_max_stored_topics(); } @@ -117,7 +117,7 @@ class retained_topic_map { } } else { - direct_index.modify(entry, [](path_entry &entry){ entry.increase_count(); }); + direct_index.modify(entry, [](path_entry& entry){ entry.increase_count(); }); } parent = entry; @@ -249,7 +249,7 @@ class retained_topic_map { // Do iterators stay valid when erasing ? I think they do ? for (auto entry : path) { - direct_index.modify(entry, [](path_entry &entry){ entry.decrease_count(); }); + direct_index.modify(entry, [](path_entry& entry){ entry.decrease_count(); }); if (entry->count == 0) { map.erase(entry); @@ -267,7 +267,7 @@ class retained_topic_map { auto& direct_index = map.template get(); for(auto& i : path) { - direct_index.modify(i, [](path_entry &entry){ entry.increase_count(); }); + direct_index.modify(i, [](path_entry& entry){ entry.increase_count(); }); } } diff --git a/test/subscription_map.hpp b/test/subscription_map.hpp index db4f2e0c4..e120aac05 100644 --- a/test/subscription_map.hpp +++ b/test/subscription_map.hpp @@ -203,7 +203,7 @@ class subscription_map_base { } // Decrease the subscription count for a specific node - static void decrease_count_storage(count_storage_t &count) { + static void decrease_count_storage(count_storage_t& count) { BOOST_ASSERT(count.value() > 0); count.decrement_value(); }