diff --git a/src/activeobjectmgr.h b/src/activeobjectmgr.h index 711c9243f..0205aee85 100644 --- a/src/activeobjectmgr.h +++ b/src/activeobjectmgr.h @@ -19,10 +19,9 @@ with this program; if not, write to the Free Software Foundation, Inc., #pragma once -#include #include -#include #include "debug.h" +#include "util/container.h" #include "irrlichttypes.h" #include "util/basic_macros.h" @@ -52,24 +51,19 @@ public: void clear() { - while (!m_active_objects.empty()) - removeObject(m_active_objects.begin()->first); + // on_destruct could add new objects so this has to be a loop + do { + for (auto &it : m_active_objects.iter()) { + if (!it.second) + continue; + m_active_objects.remove(it.first); + } + } while (!m_active_objects.empty()); } T *getActiveObject(u16 id) { - auto it = m_active_objects.find(id); - return it != m_active_objects.end() ? it->second.get() : nullptr; - } - - std::vector getAllIds() const - { - std::vector ids; - ids.reserve(m_active_objects.size()); - for (auto &it : m_active_objects) { - ids.push_back(it.first); - } - return ids; + return m_active_objects.get(id).get(); } protected: @@ -88,11 +82,9 @@ protected: bool isFreeId(u16 id) const { - return id != 0 && m_active_objects.find(id) == m_active_objects.end(); + return id != 0 && !m_active_objects.get(id); } - // ordered to fix #10985 - // Note: ActiveObjects can access the ActiveObjectMgr. Only erase objects using - // removeObject()! - std::map> m_active_objects; + // Note that this is ordered to fix #10985 + ModifySafeMap> m_active_objects; }; diff --git a/src/benchmark/CMakeLists.txt b/src/benchmark/CMakeLists.txt index 402c81cbf..a94f55da5 100644 --- a/src/benchmark/CMakeLists.txt +++ b/src/benchmark/CMakeLists.txt @@ -3,6 +3,7 @@ set (BENCHMARK_SRCS ${CMAKE_CURRENT_SOURCE_DIR}/benchmark_lighting.cpp ${CMAKE_CURRENT_SOURCE_DIR}/benchmark_serialize.cpp ${CMAKE_CURRENT_SOURCE_DIR}/benchmark_mapblock.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/benchmark_mapmodify.cpp PARENT_SCOPE) set (BENCHMARK_CLIENT_SRCS diff --git a/src/benchmark/benchmark_mapmodify.cpp b/src/benchmark/benchmark_mapmodify.cpp new file mode 100644 index 000000000..9643c0eb9 --- /dev/null +++ b/src/benchmark/benchmark_mapmodify.cpp @@ -0,0 +1,152 @@ +/* +Minetest +Copyright (C) 2023 sfan5 + +This program is free software; you can redistribute it and/or modify +it under the terms of the GNU Lesser General Public License as published by +the Free Software Foundation; either version 2.1 of the License, or +(at your option) any later version. + +This program is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +GNU Lesser General Public License for more details. + +You should have received a copy of the GNU Lesser General Public License along +with this program; if not, write to the Free Software Foundation, Inc., +51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. +*/ + +#include "benchmark_setup.h" +#include "util/container.h" + +// Testing the standard library is not useful except to compare +//#define TEST_STDLIB + +using TestMap = ModifySafeMap; + +static inline void fill(TestMap &map, size_t n) +{ + map.clear(); + for (size_t i = 0; i < n; i++) + map.put(i, reinterpret_cast(0x40000U + i)); +} + +static inline void pollute(TestMap &map) +{ + auto dummy = reinterpret_cast(123); + // produce some garbage to avoid best case behaviour + map.put(0xffff, dummy); + for (auto it : map.iter()) { + (void)it; + map.remove(0xffff); + break; + } +} + +static inline void remove(TestMap &map, size_t offset, size_t count) +{ + for (size_t i = 0; i < count; i++) + map.remove(static_cast(i + offset)); +} + +#define BENCH_ITERATE_(_label, _count, _best) \ + BENCHMARK_ADVANCED(_label)(Catch::Benchmark::Chronometer meter) { \ + TestMap map; \ + fill(map, _count); \ + if (!_best) pollute(map); \ + meter.measure([&] { \ + size_t x = map.size(); \ + for (auto &it : map.iter()) { \ + if (!it.second) \ + continue; \ + x ^= reinterpret_cast(it.second); \ + } \ + return x; \ + }); \ + }; + +#define BENCH_ITERATE(_count) \ + BENCH_ITERATE_("iterate_" #_count, _count, 0) \ + BENCH_ITERATE_("iterate_bestcase_" #_count, _count, 1) + +#define BENCH_REMOVE(_count) \ + BENCHMARK_ADVANCED("remove_" #_count)(Catch::Benchmark::Chronometer meter) { \ + TestMap map; \ + fill(map, _count); \ + meter.measure([&] { \ + for (auto it : map.iter()) { \ + (void)it; \ + remove(map, (_count) / 7, (_count) / 2); /* delete half */ \ + break; \ + } \ + }); \ + }; + +TEST_CASE("ModifySafeMap") { + BENCH_ITERATE(50) + BENCH_ITERATE(400) + BENCH_ITERATE(1000) + + BENCH_REMOVE(50) + BENCH_REMOVE(400) + BENCH_REMOVE(1000) +} + +using TestMap2 = std::map; + +static inline void fill2(TestMap2 &map, size_t n) +{ + map.clear(); + for (size_t i = 0; i < n; i++) + map.emplace(i, reinterpret_cast(0x40000U + i)); +} + +static inline void remove2(TestMap2 &map, size_t offset, size_t count) +{ + for (size_t i = 0; i < count; i++) + map.erase(static_cast(i + offset)); +} + +#define BENCH2_ITERATE(_count) \ + BENCHMARK_ADVANCED("iterate_" #_count)(Catch::Benchmark::Chronometer meter) { \ + TestMap2 map; \ + fill2(map, _count); \ + meter.measure([&] { \ + size_t x = map.size(); \ + /* mirrors what ActiveObjectMgr::step used to do */ \ + std::vector keys; \ + keys.reserve(x); \ + for (auto &it : map) \ + keys.push_back(it.first); \ + for (auto key : keys) { \ + auto it = map.find(key); \ + if (it == map.end()) \ + continue; \ + x ^= reinterpret_cast(it->second); \ + } \ + return x; \ + }); \ + }; + +#define BENCH2_REMOVE(_count) \ + BENCHMARK_ADVANCED("remove_" #_count)(Catch::Benchmark::Chronometer meter) { \ + TestMap2 map; \ + fill2(map, _count); \ + meter.measure([&] { \ + /* no overhead so no fake iteration */ \ + remove2(map, (_count) / 7, (_count) / 2); \ + }); \ + }; + +#ifdef TEST_STDLIB +TEST_CASE("std::map") { + BENCH2_ITERATE(50) + BENCH2_ITERATE(400) + BENCH2_ITERATE(1000) + + BENCH2_REMOVE(50) + BENCH2_REMOVE(400) + BENCH2_REMOVE(1000) +} +#endif diff --git a/src/client/activeobjectmgr.cpp b/src/client/activeobjectmgr.cpp index cafd22f20..96c8fd9bb 100644 --- a/src/client/activeobjectmgr.cpp +++ b/src/client/activeobjectmgr.cpp @@ -37,17 +37,14 @@ ActiveObjectMgr::~ActiveObjectMgr() void ActiveObjectMgr::step( float dtime, const std::function &f) { - g_profiler->avg("ActiveObjectMgr: CAO count [#]", m_active_objects.size()); - - // Same as in server activeobjectmgr. - std::vector ids = getAllIds(); - - for (u16 id : ids) { - auto it = m_active_objects.find(id); - if (it == m_active_objects.end()) - continue; // obj was removed - f(it->second.get()); + size_t count = 0; + for (auto &ao_it : m_active_objects.iter()) { + if (!ao_it.second) + continue; + count++; + f(ao_it.second.get()); } + g_profiler->avg("ActiveObjectMgr: CAO count [#]", count); } bool ActiveObjectMgr::registerObject(std::unique_ptr obj) @@ -71,7 +68,7 @@ bool ActiveObjectMgr::registerObject(std::unique_ptr obj) } infostream << "Client::ActiveObjectMgr::registerObject(): " << "added (id=" << obj->getId() << ")" << std::endl; - m_active_objects[obj->getId()] = std::move(obj); + m_active_objects.put(obj->getId(), std::move(obj)); return true; } @@ -79,16 +76,14 @@ void ActiveObjectMgr::removeObject(u16 id) { verbosestream << "Client::ActiveObjectMgr::removeObject(): " << "id=" << id << std::endl; - auto it = m_active_objects.find(id); - if (it == m_active_objects.end()) { + + std::unique_ptr obj = m_active_objects.take(id); + if (!obj) { infostream << "Client::ActiveObjectMgr::removeObject(): " << "id=" << id << " not found" << std::endl; return; } - std::unique_ptr obj = std::move(it->second); - m_active_objects.erase(it); - obj->removeFromScene(true); } @@ -96,8 +91,10 @@ void ActiveObjectMgr::getActiveObjects(const v3f &origin, f32 max_d, std::vector &dest) { f32 max_d2 = max_d * max_d; - for (auto &ao_it : m_active_objects) { + for (auto &ao_it : m_active_objects.iter()) { ClientActiveObject *obj = ao_it.second.get(); + if (!obj) + continue; f32 d2 = (obj->getPosition() - origin).getLengthSQ(); @@ -114,8 +111,10 @@ std::vector ActiveObjectMgr::getActiveSelectableObje f32 max_d = shootline.getLength(); v3f dir = shootline.getVector().normalize(); - for (auto &ao_it : m_active_objects) { + for (auto &ao_it : m_active_objects.iter()) { ClientActiveObject *obj = ao_it.second.get(); + if (!obj) + continue; aabb3f selection_box; if (!obj->getSelectionBox(&selection_box)) diff --git a/src/server/activeobjectmgr.cpp b/src/server/activeobjectmgr.cpp index 1b3376ae6..b4185d240 100644 --- a/src/server/activeobjectmgr.cpp +++ b/src/server/activeobjectmgr.cpp @@ -36,19 +36,12 @@ ActiveObjectMgr::~ActiveObjectMgr() void ActiveObjectMgr::clearIf(const std::function &cb) { - // Make a defensive copy of the ids in case the passed callback changes the - // set of active objects. - // The callback is called for newly added objects iff they happen to reuse - // an old id. - std::vector ids = getAllIds(); - - for (u16 id : ids) { - auto it = m_active_objects.find(id); - if (it == m_active_objects.end()) - continue; // obj was already removed - if (cb(it->second.get(), id)) { - // erase by id, `it` can be invalid now - removeObject(id); + for (auto &it : m_active_objects.iter()) { + if (!it.second) + continue; + if (cb(it.second.get(), it.first)) { + // Remove reference from m_active_objects + m_active_objects.remove(it.first); } } } @@ -56,17 +49,16 @@ void ActiveObjectMgr::clearIf(const std::function &f) { - g_profiler->avg("ActiveObjectMgr: SAO count [#]", m_active_objects.size()); + size_t count = 0; - // See above. - std::vector ids = getAllIds(); - - for (u16 id : ids) { - auto it = m_active_objects.find(id); - if (it == m_active_objects.end()) - continue; // obj was removed - f(it->second.get()); + for (auto &ao_it : m_active_objects.iter()) { + if (!ao_it.second) + continue; + count++; + f(ao_it.second.get()); } + + g_profiler->avg("ActiveObjectMgr: SAO count [#]", count); } bool ActiveObjectMgr::registerObject(std::unique_ptr obj) @@ -99,12 +91,17 @@ bool ActiveObjectMgr::registerObject(std::unique_ptr obj) return false; } - auto obj_p = obj.get(); - m_active_objects[obj->getId()] = std::move(obj); + auto obj_id = obj->getId(); + m_active_objects.put(obj_id, std::move(obj)); + auto new_size = m_active_objects.size(); verbosestream << "Server::ActiveObjectMgr::addActiveObjectRaw(): " - << "Added id=" << obj_p->getId() << "; there are now " - << m_active_objects.size() << " active objects." << std::endl; + << "Added id=" << obj_id << "; there are now "; + if (new_size == decltype(m_active_objects)::unknown) + verbosestream << "???"; + else + verbosestream << new_size; + verbosestream << " active objects." << std::endl; return true; } @@ -112,17 +109,13 @@ void ActiveObjectMgr::removeObject(u16 id) { verbosestream << "Server::ActiveObjectMgr::removeObject(): " << "id=" << id << std::endl; - auto it = m_active_objects.find(id); - if (it == m_active_objects.end()) { + + // this will take the object out of the map and then destruct it + bool ok = m_active_objects.remove(id); + if (!ok) { infostream << "Server::ActiveObjectMgr::removeObject(): " << "id=" << id << " not found" << std::endl; - return; } - - // Delete the obj before erasing, as the destructor may indirectly access - // m_active_objects. - it->second.reset(); - m_active_objects.erase(id); // `it` can be invalid now } void ActiveObjectMgr::getObjectsInsideRadius(const v3f &pos, float radius, @@ -130,8 +123,10 @@ void ActiveObjectMgr::getObjectsInsideRadius(const v3f &pos, float radius, std::function include_obj_cb) { float r2 = radius * radius; - for (auto &activeObject : m_active_objects) { + for (auto &activeObject : m_active_objects.iter()) { ServerActiveObject *obj = activeObject.second.get(); + if (!obj) + continue; const v3f &objectpos = obj->getBasePosition(); if (objectpos.getDistanceFromSQ(pos) > r2) continue; @@ -145,8 +140,10 @@ void ActiveObjectMgr::getObjectsInArea(const aabb3f &box, std::vector &result, std::function include_obj_cb) { - for (auto &activeObject : m_active_objects) { + for (auto &activeObject : m_active_objects.iter()) { ServerActiveObject *obj = activeObject.second.get(); + if (!obj) + continue; const v3f &objectpos = obj->getBasePosition(); if (!box.isPointInside(objectpos)) continue; @@ -167,7 +164,7 @@ void ActiveObjectMgr::getAddedActiveObjectsAroundPos(const v3f &player_pos, f32 - discard objects that are found in current_objects. - add remaining objects to added_objects */ - for (auto &ao_it : m_active_objects) { + for (auto &ao_it : m_active_objects.iter()) { u16 id = ao_it.first; // Get object diff --git a/src/util/container.h b/src/util/container.h index b55777ff7..985a9a447 100644 --- a/src/util/container.h +++ b/src/util/container.h @@ -28,9 +28,11 @@ with this program; if not, write to the Free Software Foundation, Inc., #include #include #include +#include +#include /* -Queue with unique values with fast checking of value existence + Queue with unique values with fast checking of value existence */ template @@ -75,6 +77,10 @@ private: std::queue m_queue; }; +/* + Thread-safe map +*/ + template class MutexedMap { @@ -116,7 +122,9 @@ private: }; -// Thread-safe Double-ended queue +/* + Thread-safe double-ended queue +*/ template class MutexedQueue @@ -237,6 +245,10 @@ protected: Semaphore m_signal; }; +/* + LRU cache +*/ + template class LRUCache { @@ -305,3 +317,206 @@ private: // we can't use std::deque here, because its iterators get invalidated std::list m_queue; }; + +/* + Map that can be safely modified (insertion, deletion) during iteration + Caveats: + - you cannot insert null elements + - you have to check for null elements during iteration, those are ones already deleted + - size() and empty() don't work during iteration + - not thread-safe in any way + + How this is implemented: + - there are two maps: new and real + - if inserting duration iteration, the value is inserted into the "new" map + - if deleting during iteration, the value is set to null (to be GC'd later) + - when iteration finishes the "new" map is merged into the "real" map +*/ + +template +class ModifySafeMap +{ +public: + // this allows bare pointers but also e.g. std::unique_ptr + static_assert(std::is_default_constructible::value, + "Value type must be default constructible"); + static_assert(std::is_constructible::value, + "Value type must be convertible to bool"); + static_assert(std::is_move_assignable::value, + "Value type must be move-assignable"); + + typedef K key_type; + typedef V mapped_type; + + ModifySafeMap() { + // the null value must convert to false and all others to true, but + // we can't statically check the latter. + sanity_check(!null_value); + } + ~ModifySafeMap() { + assert(!m_iterating); + } + + // possible to implement but we don't need it + DISABLE_CLASS_COPY(ModifySafeMap) + ALLOW_CLASS_MOVE(ModifySafeMap) + + const V &get(const K &key) const { + if (m_iterating) { + auto it = m_new.find(key); + if (it != m_new.end()) + return it->second; + } + auto it = m_values.find(key); + return it == m_values.end() ? null_value : it->second; + } + + void put(const K &key, const V &value) { + if (!value) { + assert(false); + return; + } + if (m_iterating) + m_new.emplace(key, value); + else + m_values.emplace(key, value); + } + + void put(const K &key, V &&value) { + if (!value) { + assert(false); + return; + } + if (m_iterating) + m_new.emplace(key, std::move(value)); + else + m_values.emplace(key, std::move(value)); + } + + V take(const K &key) { + V ret = V(); + if (m_iterating) { + auto it = m_new.find(key); + if (it != m_new.end()) { + ret = std::move(it->second); + m_new.erase(it); + } + } + auto it = m_values.find(key); + if (it == m_values.end()) + return ret; + ret = std::move(it->second); + if (m_iterating) { + it->second = V(); + m_garbage++; + } else { + m_values.erase(it); + } + return ret; + } + + bool remove(const K &key) { + return !!take(key); + } + + // Warning: not constant-time! + size_t size() const { + if (m_iterating) { + // This is by no means impossible to determine, it's just annoying + // to code and we happen to not need this. + return unknown; + } + assert(m_new.empty()); + if (m_garbage == 0) + return m_values.size(); + size_t n = 0; + for (auto &it : m_values) + n += !it.second ? 0 : 1; + return n; + } + + // Warning: not constant-time! + bool empty() const { + if (m_iterating) + return false; // maybe + if (m_garbage == 0) + return m_values.empty(); + for (auto &it : m_values) { + if (!!it.second) + return false; + } + return true; + } + + auto iter() { return IterationHelper(this); } + + void clear() { + if (m_iterating) { + for (auto &it : m_values) + it.second = V(); + m_garbage = m_values.size(); + } else { + m_values.clear(); + m_garbage = 0; + } + } + + static inline const V null_value = V(); + + // returned by size() if called during iteration + static constexpr size_t unknown = static_cast(-1); + +protected: + void merge_new() { + assert(!m_iterating); + if (!m_new.empty()) { + m_new.merge(m_values); // entries in m_new take precedence + m_values.clear(); + std::swap(m_values, m_new); + } + } + + void collect_garbage() { + assert(!m_iterating); + if (m_values.size() < GC_MIN_SIZE || m_garbage < m_values.size() / 2) + return; + for (auto it = m_values.begin(); it != m_values.end(); ) { + if (!it->second) + it = m_values.erase(it); + else + ++it; + } + m_garbage = 0; + } + + struct IterationHelper { + friend class ModifySafeMap; + ~IterationHelper() { + assert(m->m_iterating); + m->m_iterating--; + if (!m->m_iterating) { + m->merge_new(); + m->collect_garbage(); + } + } + + auto begin() { return m->m_values.cbegin(); } + auto end() { return m->m_values.cend(); } + + private: + IterationHelper(ModifySafeMap *parent) : m(parent) { + assert(m->m_iterating < std::numeric_limits::max()); + m->m_iterating++; + } + + ModifySafeMap *m; + }; + +private: + std::map m_values; + std::map m_new; + unsigned int m_iterating = 0; + size_t m_garbage = 0; // upper bound for null-placeholders in m_values + + static constexpr size_t GC_MIN_SIZE = 30; +};