From c9d9782c46529844b940a1f45e93a2546f7169a3 Mon Sep 17 00:00:00 2001 From: Eduardo Dantas Date: Tue, 22 Oct 2024 10:36:42 -0300 Subject: [PATCH] perf: EventCallback optimization (#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 --- src/creatures/players/player.cpp | 34 ++++++------ src/lua/callbacks/events_callbacks.cpp | 40 +++++++------- src/lua/callbacks/events_callbacks.hpp | 53 +++++++++++-------- .../events/event_callback_functions.cpp | 1 - 4 files changed, 65 insertions(+), 63 deletions(-) diff --git a/src/creatures/players/player.cpp b/src/creatures/players/player.cpp index 30c797643cd..41e70296d1a 100644 --- a/src/creatures/players/player.cpp +++ b/src/creatures/players/player.cpp @@ -4121,23 +4121,23 @@ std::map &Player::getAllItemTypeCount(std::map &Player::getAllSaleItemIdAndCount(std::map &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 &countMap) const { diff --git a/src/lua/callbacks/events_callbacks.cpp b/src/lua/callbacks/events_callbacks.cpp index ac14fd17c5a..0cd223fbc91 100644 --- 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 &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 &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> EventsCallbacks::getCallbacks() const { - return m_callbacks; -} +void EventsCallbacks::addCallback(const std::shared_ptr &callback) { + auto &callbackList = m_callbacks[callback->getType()]; -std::unordered_map> EventsCallbacks::getCallbacksByType(EventCallback_t type) const { - std::unordered_map> 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 index 032b6bef28f..8913ee1dc4b 100644 --- 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 &callback); - /** - * @brief Gets all registered event callbacks. - * @return Vector of pointers to EventCallback objects. - */ - std::unordered_map> 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> getCallbacksByType(EventCallback_t type) const; - /** * @brief Clears all registered event callbacks. */ @@ -88,9 +75,14 @@ class EventsCallbacks { */ template 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 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,9 +122,14 @@ class EventsCallbacks { template 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; } } @@ -135,8 +137,13 @@ class EventsCallbacks { } private: + struct EventCallbackEntry { + std::string name; + std::shared_ptr callback; + }; + // Container for storing registered event callbacks. - std::unordered_map> m_callbacks; + phmap::flat_hash_map> 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 index 0968df435ba..c00300b5f47 100644 --- 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(luaState, 1); if (!callback) { - reportErrorFunc("EventCallback is nil"); return 1; }