From 42b960417c15ac41bab4b13b5b84b644620d3f44 Mon Sep 17 00:00:00 2001 From: Ivan Savenko Date: Wed, 18 Dec 2024 14:01:31 +0000 Subject: [PATCH] Reworked bonus caching locking scheme: - use concurrent map from tbb for faster access to already cached values - use std::shared_mutex instead of boost::mutex to access bonuses - accessing to values existing in cache is now done without locking main mutex --- lib/bonuses/CBonusSystemNode.cpp | 90 +++++++++++++++++--------------- lib/bonuses/CBonusSystemNode.h | 20 ++++++- mapeditor/StdInc.h | 1 + 3 files changed, 67 insertions(+), 44 deletions(-) diff --git a/lib/bonuses/CBonusSystemNode.cpp b/lib/bonuses/CBonusSystemNode.cpp index b5a2d582d..d155edcb8 100644 --- a/lib/bonuses/CBonusSystemNode.cpp +++ b/lib/bonuses/CBonusSystemNode.cpp @@ -63,22 +63,10 @@ void CBonusSystemNode::getAllParents(TCNodes & out) const //retrieves list of pa void CBonusSystemNode::getAllBonusesRec(BonusList &out, const CSelector & selector) const { - //out has been reserved sufficient capacity at getAllBonuses() call - BonusList beforeUpdate; TCNodes lparents; getAllParents(lparents); - if(!lparents.empty()) - { - //estimate on how many bonuses are missing yet - must be positive - beforeUpdate.reserve(std::max(out.capacity() - out.size(), bonuses.size())); - } - else - { - beforeUpdate.reserve(bonuses.size()); //at most all local bonuses - } - for(const auto * parent : lparents) { parent->getAllBonusesRec(beforeUpdate, selector); @@ -111,46 +99,64 @@ TConstBonusListPtr CBonusSystemNode::getAllBonuses(const CSelector &selector, co { if (CBonusSystemNode::cachingEnabled) { - // Exclusive access for one thread - boost::lock_guard lock(sync); - - // If the bonus system tree changes(state of a single node or the relations to each other) then - // cache all bonus objects. Selector objects doesn't matter. - if (cachedLast != treeChanged) - { - BonusList allBonuses; - allBonuses.reserve(cachedBonuses.capacity()); //we assume we'll get about the same number of bonuses - - cachedBonuses.clear(); - cachedRequests.clear(); - - getAllBonusesRec(allBonuses, Selector::all); - limitBonuses(allBonuses, cachedBonuses); - cachedBonuses.stackBonuses(); - - cachedLast = treeChanged; - } - // If a bonus system request comes with a caching string then look up in the map if there are any // pre-calculated bonus results. Limiters can't be cached so they have to be calculated. - if(!cachingStr.empty()) + if (cachedLast == treeChanged && !cachingStr.empty()) { - auto it = cachedRequests.find(cachingStr); - if(it != cachedRequests.end()) - { - //Cached list contains bonuses for our query with applied limiters - return it->second; - } + RequestsMap::const_accessor accessor; + + //Cached list contains bonuses for our query with applied limiters + if (cachedRequests.find(accessor, cachingStr) && accessor->second.first == cachedLast) + return accessor->second.second; } //We still don't have the bonuses (didn't returned them from cache) //Perform bonus selection auto ret = std::make_shared(); - cachedBonuses.getBonuses(*ret, selector, limit); + + if (cachedLast == treeChanged) + { + // Cached bonuses are up-to-date - use shared/read access and compute results + std::shared_lock lock(sync); + cachedBonuses.getBonuses(*ret, selector, limit); + } + else + { + // If the bonus system tree changes(state of a single node or the relations to each other) then + // cache all bonus objects. Selector objects doesn't matter. + std::lock_guard lock(sync); + if (cachedLast == treeChanged) + { + // While our thread was waiting, another one have updated bonus tree. Use cached bonuses. + cachedBonuses.getBonuses(*ret, selector, limit); + } + else + { + // Cached bonuses may be outdated - regenerate them + BonusList allBonuses; + + cachedBonuses.clear(); + + getAllBonusesRec(allBonuses, Selector::all); + limitBonuses(allBonuses, cachedBonuses); + cachedBonuses.stackBonuses(); + cachedLast = treeChanged; + cachedBonuses.getBonuses(*ret, selector, limit); + } + } // Save the results in the cache - if(!cachingStr.empty()) - cachedRequests[cachingStr] = ret; + if (!cachingStr.empty()) + { + RequestsMap::accessor accessor; + if (cachedRequests.find(accessor, cachingStr)) + { + accessor->second.second = ret; + accessor->second.first = cachedLast; + } + else + cachedRequests.emplace(cachingStr, std::pair{ cachedLast, ret }); + } return ret; } diff --git a/lib/bonuses/CBonusSystemNode.h b/lib/bonuses/CBonusSystemNode.h index 460957f8b..5baad0e96 100644 --- a/lib/bonuses/CBonusSystemNode.h +++ b/lib/bonuses/CBonusSystemNode.h @@ -14,6 +14,8 @@ #include "../serializer/Serializeable.h" +#include + VCMI_LIB_NAMESPACE_BEGIN using TNodes = std::set; @@ -30,6 +32,19 @@ public: UNKNOWN, STACK_INSTANCE, STACK_BATTLE, SPECIALTY, ARTIFACT, CREATURE, ARTIFACT_INSTANCE, HERO, PLAYER, TEAM, TOWN_AND_VISITOR, BATTLE, COMMANDER, GLOBAL_EFFECTS, ALL_CREATURES, TOWN }; + + struct HashStringCompare { + static size_t hash(const std::string& data) + { + std::hash hasher; + return hasher(data); + } + static bool equal(const std::string& x, const std::string& y) + { + return x == y; + } + }; + private: BonusList bonuses; //wielded bonuses (local or up-propagated here) BonusList exportedBonuses; //bonuses coming from this node (wielded or propagated away) @@ -49,8 +64,9 @@ private: // Setting a value to cachingStr before getting any bonuses caches the result for later requests. // This string needs to be unique, that's why it has to be set in the following manner: // [property key]_[value] => only for selector - mutable std::map cachedRequests; - mutable boost::mutex sync; + using RequestsMap = tbb::concurrent_hash_map, HashStringCompare>; + mutable RequestsMap cachedRequests; + mutable std::shared_mutex sync; void getAllBonusesRec(BonusList &out, const CSelector & selector) const; TConstBonusListPtr getAllBonusesWithoutCaching(const CSelector &selector, const CSelector &limit) const; diff --git a/mapeditor/StdInc.h b/mapeditor/StdInc.h index 4ff74c6b0..dfe16b541 100644 --- a/mapeditor/StdInc.h +++ b/mapeditor/StdInc.h @@ -10,6 +10,7 @@ #pragma once #include "../Global.h" +#include // Workaround for Qt / tbb name clash #define VCMI_EDITOR_NAME "VCMI Map Editor"