perf: EventCallback optimization (opentibiabr#3004)

This PR refactors how event callbacks are stored and retrieved. Specifically, the internal structure for managing callbacks was changed from `std::vector` to a more efficient `phmap::flat_hash_map`. This change avoids the need to recreate a vector every time event callbacks are accessed, improving performance and reducing memory overhead. Additionally, the following issues were addressed: Duplicate log messages: A duplicate log was being created in luaEventCallbackCreate. This has been removed, as the message was already sent earlier in the process. --------- Co-authored-by: Renato Machado <[email protected]>
vllworldbuilding · Oct 22, 2024 · 7684026 · 7684026
1 parent 6f9754f
commit 7684026
Show file tree

Hide file tree

Showing 5 changed files with 67 additions and 65 deletions.
diff --git a/data-otservbr-global/scripts/quests/soul_war/globalevent-ebb_and_flow_change_maps.lua b/data-otservbr-global/scripts/quests/soul_war/globalevent-ebb_and_flow_change_maps.lua
@@ -37,7 +37,7 @@ local function loadMapEmpty()
 	SoulWarQuest.ebbAndFlow.setLoadedEmptyMap(true)
 	SoulWarQuest.ebbAndFlow.setActive(false)
 
-	local updatePlayers = EventCallback("UpdatePlayersEmptyEbbFlowMap")
+	local updatePlayers = EventCallback("UpdatePlayersEmptyEbbFlowMap", true)
 	function updatePlayers.mapOnLoad(mapPath)
 		if mapPath ~= SoulWarQuest.ebbAndFlow.mapsPath.empty then
 			return
@@ -95,7 +95,7 @@ local function loadMapInundate()
 	SoulWarQuest.ebbAndFlow.setLoadedEmptyMap(false)
 	SoulWarQuest.ebbAndFlow.setActive(true)
 
-	local updatePlayers = EventCallback("UpdatePlayersInundateEbbFlowMap")
+	local updatePlayers = EventCallback("UpdatePlayersInundateEbbFlowMap", true)
 	function updatePlayers.mapOnLoad(mapPath)
 		if mapPath ~= SoulWarQuest.ebbAndFlow.mapsPath.inundate then
 			return

diff --git a/src/creatures/players/player.cpp b/src/creatures/players/player.cpp
@@ -4121,23 +4121,23 @@ std::map<uint32_t, uint32_t> &Player::getAllItemTypeCount(std::map<uint32_t, uin
 }
 
 std::map<uint16_t, uint16_t> &Player::getAllSaleItemIdAndCount(std::map<uint16_t, uint16_t> &countMap) const {
-    for (const auto &item : getAllInventoryItems(false, true)) {
-        if (item->getID() != ITEM_GOLD_POUCH) {
-            if (!item->hasMarketAttributes()) {
-                continue;
-            }
-            
-            if (const auto &container = item->getContainer()) {
-                if (!container->empty()) {
-                    continue;
-                }
-            }
-        }
-
-        countMap[item->getID()] += item->getItemCount();
-    }
-
-    return countMap;
+	for (const auto &item : getAllInventoryItems(false, true)) {
+		if (item->getID() != ITEM_GOLD_POUCH) {
+			if (!item->hasMarketAttributes()) {
+				continue;
+			}
+
+			if (const auto &container = item->getContainer()) {
+				if (!container->empty()) {
+					continue;
+				}
+			}
+		}
+
+		countMap[item->getID()] += item->getItemCount();
+	}
+
+	return countMap;
 }
 
 void Player::getAllItemTypeCountAndSubtype(std::map<uint32_t, uint32_t> &countMap) const {

diff --git a/src/lua/callbacks/events_callbacks.cpp b/src/lua/callbacks/events_callbacks.cpp
@@ -29,39 +29,35 @@ EventsCallbacks &EventsCallbacks::getInstance() {
 }
 
 bool EventsCallbacks::isCallbackRegistered(const std::shared_ptr<EventCallback> &callback) {
-	if (g_game().getGameState() == GAME_STATE_STARTUP && !callback->skipDuplicationCheck() && m_callbacks.find(callback->getName()) != m_callbacks.end()) {
-		return true;
+	auto it = m_callbacks.find(callback->getType());
+
+	if (it == m_callbacks.end()) {
+		return false;
 	}
 
-	return false;
-}
+	const auto &callbacks = it->second;
 
-void EventsCallbacks::addCallback(const std::shared_ptr<EventCallback> &callback) {
-	if (m_callbacks.find(callback->getName()) != m_callbacks.end() && !callback->skipDuplicationCheck()) {
-		g_logger().trace("Event callback already registered: {}", callback->getName());
-		return;
-	}
+	auto isSameCallbackName = [&callback](const auto &pair) {
+		return pair.name == callback->getName();
+	};
 
-	g_logger().trace("Registering event callback: {}", callback->getName());
+	auto found = std::ranges::find_if(callbacks, isSameCallbackName);
 
-	m_callbacks[callback->getName()] = callback;
+	return (found != callbacks.end() && !callback->skipDuplicationCheck());
 }
 
-std::unordered_map<std::string, std::shared_ptr<EventCallback>> EventsCallbacks::getCallbacks() const {
-	return m_callbacks;
-}
+void EventsCallbacks::addCallback(const std::shared_ptr<EventCallback> &callback) {
+	auto &callbackList = m_callbacks[callback->getType()];
 
-std::unordered_map<std::string, std::shared_ptr<EventCallback>> EventsCallbacks::getCallbacksByType(EventCallback_t type) const {
-	std::unordered_map<std::string, std::shared_ptr<EventCallback>> eventCallbacks;
-	for (auto [name, callback] : getCallbacks()) {
-		if (callback->getType() != type) {
-			continue;
+	for (const auto &entry : callbackList) {
+		if (entry.name == callback->getName() && !callback->skipDuplicationCheck()) {
+			g_logger().trace("Event callback already registered: {}", callback->getName());
+			return;
 		}
-
-		eventCallbacks[name] = callback;
 	}
 
-	return eventCallbacks;
+	g_logger().trace("Registering event callback: {}", callback->getName());
+	callbackList.emplace_back(EventCallbackEntry { callback->getName(), callback });
 }
 
 void EventsCallbacks::clear() {

diff --git a/src/lua/callbacks/events_callbacks.hpp b/src/lua/callbacks/events_callbacks.hpp
@@ -62,19 +62,6 @@ class EventsCallbacks {
 	 */
 	void addCallback(const std::shared_ptr<EventCallback> &callback);
 
-	/**
-	 * @brief Gets all registered event callbacks.
-	 * @return Vector of pointers to EventCallback objects.
-	 */
-	std::unordered_map<std::string, std::shared_ptr<EventCallback>> getCallbacks() const;
-
-	/**
-	 * @brief Gets event callbacks by their type.
-	 * @param type The type of callbacks to retrieve.
-	 * @return Vector of pointers to EventCallback objects of the specified type.
-	 */
-	std::unordered_map<std::string, std::shared_ptr<EventCallback>> getCallbacksByType(EventCallback_t type) const;
-
 	/**
 	 * @brief Clears all registered event callbacks.
 	 */
@@ -88,9 +75,14 @@ class EventsCallbacks {
 	 */
 	template <typename CallbackFunc, typename... Args>
 	void executeCallback(EventCallback_t eventType, CallbackFunc callbackFunc, Args &&... args) {
-		for (const auto &[name, callback] : getCallbacksByType(eventType)) {
-			if (callback && callback->isLoadedCallback()) {
-				std::invoke(callbackFunc, *callback, args...);
+		auto it = m_callbacks.find(eventType);
+		if (it == m_callbacks.end()) {
+			return;
+		}
+
+		for (const auto &entry : it->second) {
+			if (entry.callback && entry.callback->isLoadedCallback()) {
+				std::invoke(callbackFunc, *entry.callback, args...);
 			}
 		}
 	}
@@ -104,9 +96,14 @@ class EventsCallbacks {
 	 */
 	template <typename CallbackFunc, typename... Args>
 	ReturnValue checkCallbackWithReturnValue(EventCallback_t eventType, CallbackFunc callbackFunc, Args &&... args) {
-		for (const auto &[name, callback] : getCallbacksByType(eventType)) {
-			if (callback && callback->isLoadedCallback()) {
-				ReturnValue callbackResult = std::invoke(callbackFunc, *callback, args...);
+		auto it = m_callbacks.find(eventType);
+		if (it == m_callbacks.end()) {
+			return RETURNVALUE_NOERROR;
+		}
+
+		for (const auto &entry : it->second) {
+			if (entry.callback && entry.callback->isLoadedCallback()) {
+				ReturnValue callbackResult = std::invoke(callbackFunc, *entry.callback, args...);
 				if (callbackResult != RETURNVALUE_NOERROR) {
 					return callbackResult;
 				}
@@ -125,18 +122,28 @@ class EventsCallbacks {
 	template <typename CallbackFunc, typename... Args>
 	bool checkCallback(EventCallback_t eventType, CallbackFunc callbackFunc, Args &&... args) {
 		bool allCallbacksSucceeded = true;
-		for (const auto &[name, callback] : getCallbacksByType(eventType)) {
-			if (callback && callback->isLoadedCallback()) {
-				bool callbackResult = std::invoke(callbackFunc, *callback, args...);
+		auto it = m_callbacks.find(eventType);
+		if (it == m_callbacks.end()) {
+			return allCallbacksSucceeded;
+		}
+
+		for (const auto &entry : it->second) {
+			if (entry.callback && entry.callback->isLoadedCallback()) {
+				bool callbackResult = std::invoke(callbackFunc, *entry.callback, args...);
 				allCallbacksSucceeded &= callbackResult;
 			}
 		}
 		return allCallbacksSucceeded;
 	}
 
 private:
+	struct EventCallbackEntry {
+		std::string name;
+		std::shared_ptr<EventCallback> callback;
+	};
+
 	// Container for storing registered event callbacks.
-	std::unordered_map<std::string, std::shared_ptr<EventCallback>> m_callbacks;
+	phmap::flat_hash_map<EventCallback_t, std::vector<EventCallbackEntry>> m_callbacks;
 };
 
 constexpr auto g_callbacks = EventsCallbacks::getInstance;
diff --git a/src/lua/functions/events/event_callback_functions.cpp b/src/lua/functions/events/event_callback_functions.cpp
@@ -101,7 +101,6 @@ int EventCallbackFunctions::luaEventCallbackRegister(lua_State* luaState) {
 int EventCallbackFunctions::luaEventCallbackLoad(lua_State* luaState) {
 	auto callback = getUserdataShared<EventCallback>(luaState, 1);
 	if (!callback) {
-		reportErrorFunc("EventCallback is nil");
 		return 1;
 	}