Custom data structure for active objects to get performance *and* safety (#13880)

src/activeobjectmgr.h

@@ -19,10 +19,9 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 
 #pragma once
 
-#include <map>
 #include <memory>
-#include <vector>
 #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())
+		// on_destruct could add new objects so this has to be a loop
-			removeObject(m_active_objects.begin()->first);
+		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 m_active_objects.get(id).get();
-		return it != m_active_objects.end() ? it->second.get() : nullptr;
-	}
-
-	std::vector<u16> getAllIds() const
-	{
-		std::vector<u16> ids;
-		ids.reserve(m_active_objects.size());
-		for (auto &it : m_active_objects) {
-			ids.push_back(it.first);
-		}
-		return ids;
 	}
 
 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 that this is ordered to fix #10985
-	// Note: ActiveObjects can access the ActiveObjectMgr. Only erase objects using
+	ModifySafeMap<u16, std::unique_ptr<T>> m_active_objects;
-	// removeObject()!
-	std::map<u16, std::unique_ptr<T>> m_active_objects;
 };

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

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<u16, void*>;
+
+static inline void fill(TestMap &map, size_t n)
+{
+	map.clear();
+	for (size_t i = 0; i < n; i++)
+		map.put(i, reinterpret_cast<TestMap::mapped_type>(0x40000U + i));
+}
+
+static inline void pollute(TestMap &map)
+{
+	auto dummy = reinterpret_cast<TestMap::mapped_type>(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<TestMap::key_type>(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<intptr_t>(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<u16, void*>;
+
+static inline void fill2(TestMap2 &map, size_t n)
+{
+	map.clear();
+	for (size_t i = 0; i < n; i++)
+		map.emplace(i, reinterpret_cast<TestMap2::mapped_type>(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<TestMap2::key_type>(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<TestMap2::key_type> 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<intptr_t>(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

src/client/activeobjectmgr.cpp

@@ -37,17 +37,14 @@ ActiveObjectMgr::~ActiveObjectMgr()
 void ActiveObjectMgr::step(
 		float dtime, const std::function<void(ClientActiveObject *)> &f)
 {
-	g_profiler->avg("ActiveObjectMgr: CAO count [#]", m_active_objects.size());
+	size_t count = 0;
-
+	for (auto &ao_it : m_active_objects.iter()) {
-	// Same as in server activeobjectmgr.
+		if (!ao_it.second)
-	std::vector<u16> ids = getAllIds();
+			continue;
-
+		count++;
-	for (u16 id : ids) {
+		f(ao_it.second.get());
-		auto it = m_active_objects.find(id);
-		if (it == m_active_objects.end())
-			continue; // obj was removed
-		f(it->second.get());
 	}
+	g_profiler->avg("ActiveObjectMgr: CAO count [#]", count);
 }
 
 bool ActiveObjectMgr::registerObject(std::unique_ptr<ClientActiveObject> obj)
@@ -71,7 +68,7 @@ bool ActiveObjectMgr::registerObject(std::unique_ptr<ClientActiveObject> 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<ClientActiveObject> obj = m_active_objects.take(id);
+	if (!obj) {
 		infostream << "Client::ActiveObjectMgr::removeObject(): "
 				<< "id=" << id << " not found" << std::endl;
 		return;
 	}
 
-	std::unique_ptr<ClientActiveObject> 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<DistanceSortedActiveObject> &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<DistanceSortedActiveObject> 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))

src/server/activeobjectmgr.cpp

@@ -36,19 +36,12 @@ ActiveObjectMgr::~ActiveObjectMgr()
 
 void ActiveObjectMgr::clearIf(const std::function<bool(ServerActiveObject *, u16)> &cb)
 {
-	// Make a defensive copy of the ids in case the passed callback changes the
+	for (auto &it : m_active_objects.iter()) {
-	// set of active objects.
+		if (!it.second)
-	// The callback is called for newly added objects iff they happen to reuse
+			continue;
-	// an old id.
+		if (cb(it.second.get(), it.first)) {
-	std::vector<u16> ids = getAllIds();
+			// Remove reference from m_active_objects
-
+			m_active_objects.remove(it.first);
-	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);
 		}
 	}
 }
@@ -56,17 +49,16 @@ void ActiveObjectMgr::clearIf(const std::function<bool(ServerActiveObject *, u16
 void ActiveObjectMgr::step(
 		float dtime, const std::function<void(ServerActiveObject *)> &f)
 {
-	g_profiler->avg("ActiveObjectMgr: SAO count [#]", m_active_objects.size());
+	size_t count = 0;
 
-	// See above.
+	for (auto &ao_it : m_active_objects.iter()) {
-	std::vector<u16> ids = getAllIds();
+		if (!ao_it.second)
-
+			continue;
-	for (u16 id : ids) {
+		count++;
-		auto it = m_active_objects.find(id);
+		f(ao_it.second.get());
-		if (it == m_active_objects.end())
-			continue; // obj was removed
-		f(it->second.get());
 	}
+
+	g_profiler->avg("ActiveObjectMgr: SAO count [#]", count);
 }
 
 bool ActiveObjectMgr::registerObject(std::unique_ptr<ServerActiveObject> obj)
@@ -99,12 +91,17 @@ bool ActiveObjectMgr::registerObject(std::unique_ptr<ServerActiveObject> obj)
 		return false;
 	}
 
-	auto obj_p = obj.get();
+	auto obj_id = obj->getId(); 
-	m_active_objects[obj->getId()] = std::move(obj);
+	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 "
+			<< "Added id=" << obj_id << "; there are now ";
-			<< m_active_objects.size() << " active objects." << std::endl;
+	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<bool(ServerActiveObject *obj)> 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<ServerActiveObject *> &result,
 		std::function<bool(ServerActiveObject *obj)> 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

src/util/container.h

@@ -28,6 +28,8 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 #include <map>
 #include <set>
 #include <queue>
+#include <cassert>
+#include <limits>
 
 /*
 	Queue with unique values with fast checking of value existence
@@ -75,6 +77,10 @@ private:
 	std::queue<Value> m_queue;
 };
 
+/*
+	Thread-safe map
+*/
+
 template<typename Key, typename Value>
 class MutexedMap
 {
@@ -116,7 +122,9 @@ private:
 };
 
 
-// Thread-safe Double-ended queue
+/*
+	Thread-safe double-ended queue
+*/
 
 template<typename T>
 class MutexedQueue
@@ -237,6 +245,10 @@ protected:
 	Semaphore m_signal;
 };
 
+/*
+	LRU cache
+*/
+
 template<typename K, typename V>
 class LRUCache
 {
@@ -305,3 +317,206 @@ private:
 	// we can't use std::deque here, because its iterators get invalidated
 	std::list<K> 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<typename K, typename V>
+class ModifySafeMap
+{
+public:
+	// this allows bare pointers but also e.g. std::unique_ptr
+	static_assert(std::is_default_constructible<V>::value,
+		"Value type must be default constructible");
+	static_assert(std::is_constructible<bool, V>::value,
+		"Value type must be convertible to bool");
+	static_assert(std::is_move_assignable<V>::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<size_t>(-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<K, V>;
+		~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<K, V> *parent) : m(parent) {
+			assert(m->m_iterating < std::numeric_limits<decltype(m_iterating)>::max());
+			m->m_iterating++;
+		}
+
+		ModifySafeMap<K, V> *m;
+	};
+
+private:
+	std::map<K, V> m_values;
+	std::map<K, V> 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;
+};