Merge pull request #894 from vcmi/optimization_rebase

Bonus and string optimization

lib/HeroBonus.cpp

@@ -555,6 +555,7 @@ void BonusList::getBonuses(BonusList & out, const CSelector &selector) const
 
 void BonusList::getBonuses(BonusList & out, const CSelector &selector, const CSelector &limit) const
 {
+	out.reserve(bonuses.size());
 	for (auto & b : bonuses)
 	{
 		//add matching bonuses that matches limit predicate or have NO_LIMIT if no given predicate
@@ -620,6 +621,11 @@ void BonusList::resize(BonusList::TInternalContainer::size_type sz, std::shared_
 	changed();
 }
 
+void BonusList::reserve(TInternalContainer::size_type sz)
+{
+	bonuses.reserve(sz);
+}
+
 void BonusList::insert(BonusList::TInternalContainer::iterator position, BonusList::TInternalContainer::size_type n, std::shared_ptr<Bonus> const &x)
 {
 	bonuses.insert(position, n, x);
@@ -654,14 +660,16 @@ int IBonusBearer::valOfBonuses(Bonus::BonusType type, const CSelector &selector)
 
 int IBonusBearer::valOfBonuses(Bonus::BonusType type, int subtype) const
 {
-	boost::format fmt("type_%ds_%d");
-	fmt % (int)type % subtype;
+	//This part is performance-critical
+
+	char cachingStr[20] = {};
+	std::sprintf(cachingStr, "type_%ds_%d", (int)type, subtype);
 
 	CSelector s = Selector::type()(type);
 	if(subtype != -1)
 		s = s.And(Selector::subtype()(subtype));
 
-	return valOfBonuses(s, fmt.str());
+	return valOfBonuses(s, cachingStr);
 }
 
 int IBonusBearer::valOfBonuses(const CSelector &selector, const std::string &cachingStr) const
@@ -672,6 +680,7 @@ int IBonusBearer::valOfBonuses(const CSelector &selector, const std::string &cac
 }
 bool IBonusBearer::hasBonus(const CSelector &selector, const std::string &cachingStr) const
 {
+	//TODO: We don't need to count all bonuses and could break on first matching
 	return getBonuses(selector, cachingStr)->size() > 0;
 }
 
@@ -682,14 +691,15 @@ bool IBonusBearer::hasBonus(const CSelector &selector, const CSelector &limit, c
 
 bool IBonusBearer::hasBonusOfType(Bonus::BonusType type, int subtype) const
 {
-	boost::format fmt("type_%ds_%d");
-	fmt % (int)type % subtype;
+	//This part is performance-ciritcal
+	char cachingStr[20] = {};
+	std::sprintf(cachingStr, "type_%ds_%d", (int)type, subtype);
 
 	CSelector s = Selector::type()(type);
 	if(subtype != -1)
 		s = s.And(Selector::subtype()(subtype));
 
-	return hasBonus(s, fmt.str());
+	return hasBonus(s, cachingStr);
 }
 
 TConstBonusListPtr IBonusBearer::getBonuses(const CSelector &selector, const std::string &cachingStr) const
@@ -943,21 +953,34 @@ void CBonusSystemNode::getAllParents(TCNodes & out) const //retrieves list of pa
 
 void CBonusSystemNode::getAllBonusesRec(BonusList &out) const
 {
+	//out has been reserved sufficient capacity at getAllBonuses() call
+
 	BonusList beforeUpdate;
 	TCNodes lparents;
 	getAllParents(lparents);
 
-	for(auto parent : lparents)
-		parent->getAllBonusesRec(beforeUpdate);
+	if (lparents.size())
+	{
+		//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 (auto parent : lparents)
+	{
+		parent->getAllBonusesRec(beforeUpdate);
+	}
 	bonuses.getAllBonuses(beforeUpdate);
 
-	for(auto b : beforeUpdate)
+	for(const auto & b : beforeUpdate)
 	{
 		auto updated = b->updater 
 			? getUpdatedBonus(b, b->updater) 
 			: b;
-		
+
 		//do not add bonus with same pointer
 		if(!vstd::contains(out, updated))
 			out.push_back(updated);
@@ -976,10 +999,12 @@ TConstBonusListPtr CBonusSystemNode::getAllBonuses(const CSelector &selector, co
 		// 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();
 
-			BonusList allBonuses;
 			getAllBonusesRec(allBonuses);
 			limitBonuses(allBonuses, cachedBonuses);
 			cachedBonuses.stackBonuses();
@@ -1409,7 +1434,7 @@ void CBonusSystemNode::getRedAncestors(TNodes &out)
 
 	TNodes redParents; 
 	getRedParents(redParents);
-	
+
 	for(CBonusSystemNode * parent : redParents)
 		parent->getRedAncestors(out);
 }

lib/HeroBonus.h

@@ -549,6 +549,8 @@ public:
 	void clear();
 	bool empty() const { return bonuses.empty(); }
 	void resize(TInternalContainer::size_type sz, std::shared_ptr<Bonus> c = nullptr );
+	void reserve(TInternalContainer::size_type sz);
+	TInternalContainer::size_type capacity() const { return bonuses.capacity(); }
 	STRONG_INLINE std::shared_ptr<Bonus> &operator[] (TInternalContainer::size_type n) { return bonuses[n]; }
 	STRONG_INLINE const std::shared_ptr<Bonus> &operator[] (TInternalContainer::size_type n) const { return bonuses[n]; }
 	std::shared_ptr<Bonus> &back() { return bonuses.back(); }

lib/int3.h

@@ -158,11 +158,11 @@ public:
 	//returns "(x y z)" string
 	std::string toString() const
 	{
-		std::string result("(");
-		result += boost::lexical_cast<std::string>(x); result += ' ';
-		result += boost::lexical_cast<std::string>(y); result += ' ';
-		result += boost::lexical_cast<std::string>(z); result += ')';
-		return result;
+		//Performance is important here
+		char str[16] = {};
+		std::sprintf(str, "(%d %d %d)", x, y, z);
+
+		return std::string(str);
 	}
 
 	bool valid() const //Should be named "isValid"?

lib/mapObjects/CArmedInstance.cpp

@@ -76,7 +76,11 @@ void CArmedInstance::updateMoraleBonusFromArmy()
 
 		factions.insert(creature->faction);
 		// Check for undead flag instead of faction (undead mummies are neutral)
-		hasUndead |= inst->hasBonus(undeadSelector, undeadCacheKey);
+		if (!hasUndead)
+		{
+			//this is costly check, let's skip it at first undead
+			hasUndead |= inst->hasBonus(undeadSelector, undeadCacheKey);
+		}
 	}
 
 	size_t factionsInArmy = factions.size(); //town garrison seems to take both sets into account