From d0f857c3c490c8bc273386006ce962e1d5602aac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Zieli=C5=84ski?= Date: Wed, 14 Sep 2022 11:00:40 +0200 Subject: [PATCH 1/2] - Generate caching string with sprintf - Reserve BonusList space to avoid costly vector reallocation - Tweaks in int3 and UNDEAD bonus --- lib/HeroBonus.cpp | 49 +++++++++++++++++++++++-------- lib/HeroBonus.h | 2 ++ lib/int3.h | 10 +++---- lib/mapObjects/CArmedInstance.cpp | 6 +++- 4 files changed, 49 insertions(+), 18 deletions(-) diff --git a/lib/HeroBonus.cpp b/lib/HeroBonus.cpp index 7218e23ae..1b24804ed 100644 --- a/lib/HeroBonus.cpp +++ b/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 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] = {}; + 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] = {}; + 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(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); } diff --git a/lib/HeroBonus.h b/lib/HeroBonus.h index 28f00d05f..61941c27a 100644 --- a/lib/HeroBonus.h +++ b/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 c = nullptr ); + void reserve(TInternalContainer::size_type sz); + TInternalContainer::size_type capacity() const { return bonuses.capacity(); } STRONG_INLINE std::shared_ptr &operator[] (TInternalContainer::size_type n) { return bonuses[n]; } STRONG_INLINE const std::shared_ptr &operator[] (TInternalContainer::size_type n) const { return bonuses[n]; } std::shared_ptr &back() { return bonuses.back(); } diff --git a/lib/int3.h b/lib/int3.h index 3c066e056..0894892fe 100644 --- a/lib/int3.h +++ b/lib/int3.h @@ -158,11 +158,11 @@ public: //returns "(x y z)" string std::string toString() const { - std::string result("("); - result += boost::lexical_cast(x); result += ' '; - result += boost::lexical_cast(y); result += ' '; - result += boost::lexical_cast(z); result += ')'; - return result; + //Performance is important here + char str[16] = {}; + sprintf(str, "(%d %d %d)", x, y, z); + + return std::string(str); } bool valid() const //Should be named "isValid"? diff --git a/lib/mapObjects/CArmedInstance.cpp b/lib/mapObjects/CArmedInstance.cpp index 9c84ba0f2..5d23e8ef4 100644 --- a/lib/mapObjects/CArmedInstance.cpp +++ b/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 From 55b142a8c90a98cb9e18186bb3261a41737648d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Zieli=C5=84ski?= Date: Wed, 14 Sep 2022 11:37:12 +0200 Subject: [PATCH 2/2] Suggested tweaks --- lib/HeroBonus.cpp | 6 +++--- lib/int3.h | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/HeroBonus.cpp b/lib/HeroBonus.cpp index 1b24804ed..081d874f7 100644 --- a/lib/HeroBonus.cpp +++ b/lib/HeroBonus.cpp @@ -663,7 +663,7 @@ int IBonusBearer::valOfBonuses(Bonus::BonusType type, int subtype) const //This part is performance-critical char cachingStr[20] = {}; - sprintf(cachingStr, "type_%ds_%d", (int)type, subtype); + std::sprintf(cachingStr, "type_%ds_%d", (int)type, subtype); CSelector s = Selector::type()(type); if(subtype != -1) @@ -693,7 +693,7 @@ bool IBonusBearer::hasBonusOfType(Bonus::BonusType type, int subtype) const { //This part is performance-ciritcal char cachingStr[20] = {}; - sprintf(cachingStr, "type_%ds_%d", (int)type, subtype); + std::sprintf(cachingStr, "type_%ds_%d", (int)type, subtype); CSelector s = Selector::type()(type); if(subtype != -1) @@ -975,7 +975,7 @@ void CBonusSystemNode::getAllBonusesRec(BonusList &out) const } bonuses.getAllBonuses(beforeUpdate); - for(auto & b : beforeUpdate) + for(const auto & b : beforeUpdate) { auto updated = b->updater ? getUpdatedBonus(b, b->updater) diff --git a/lib/int3.h b/lib/int3.h index 0894892fe..d8861e314 100644 --- a/lib/int3.h +++ b/lib/int3.h @@ -160,7 +160,7 @@ public: { //Performance is important here char str[16] = {}; - sprintf(str, "(%d %d %d)", x, y, z); + std::sprintf(str, "(%d %d %d)", x, y, z); return std::string(str); }