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

@@ -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<boost::mutex> 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)
+		// 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 (cachedLast == treeChanged && !cachingStr.empty())
 		{
-			BonusList allBonuses;
-			allBonuses.reserve(cachedBonuses.capacity()); //we assume we'll get about the same number of bonuses
+			RequestsMap::const_accessor accessor;
 
-			cachedBonuses.clear();
-			cachedRequests.clear();
+			//Cached list contains bonuses for our query with applied limiters
+			if (cachedRequests.find(accessor, cachingStr) && accessor->second.first == cachedLast)
+				return accessor->second.second;
+		}
 
-			getAllBonusesRec(allBonuses, Selector::all);
-			limitBonuses(allBonuses, cachedBonuses);
-			cachedBonuses.stackBonuses();
+		//We still don't have the bonuses (didn't returned them from cache)
+		//Perform bonus selection
+		auto ret = std::make_shared<BonusList>();
 
-			cachedLast = treeChanged;
+		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);
 		}
-
-		// 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())
+		else
 		{
-			auto it = cachedRequests.find(cachingStr);
-			if(it != cachedRequests.end())
+			// 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)
 			{
-				//Cached list contains bonuses for our query with applied limiters
-				return it->second;
+				// 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;
 
-		//We still don't have the bonuses (didn't returned them from cache)
-		//Perform bonus selection
-		auto ret = std::make_shared<BonusList>();
-		cachedBonuses.getBonuses(*ret, selector, limit);
+				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<int64_t, TBonusListPtr>{ cachedLast, ret });
+		}
 
 		return ret;
 	}

lib/bonuses/CBonusSystemNode.h

@@ -14,6 +14,8 @@
 
 #include "../serializer/Serializeable.h"
 
+#include <tbb/concurrent_hash_map.h>
+
 VCMI_LIB_NAMESPACE_BEGIN
 
 using TNodes = std::set<CBonusSystemNode *>;
@@ -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<std::string> 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<std::string, TBonusListPtr > cachedRequests;
-	mutable boost::mutex sync;
+	using RequestsMap = tbb::concurrent_hash_map<std::string, std::pair<int64_t, TBonusListPtr>, 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;

mapeditor/StdInc.h

@@ -10,6 +10,7 @@
 #pragma once
 
 #include "../Global.h"
+#include <tbb/concurrent_hash_map.h> // Workaround for Qt / tbb name clash
 
 #define VCMI_EDITOR_NAME "VCMI Map Editor"