Skip to content

Commit

Permalink
(1.18 backport)Replace the recursion decoptge method with the less CP…
Browse files Browse the repository at this point in the history 
…U-intensive config pointer comparison method

When I use an ability id=A and include [filter][filter_adjacent]ability_id_active=A, the more units with the adjacent ability I add to it, the slower the game becomes, and at the third unit the game freezes, whereas with the direct comparison of the configs the game only slows down significantly after the 5th unit added.
  • Loading branch information
@newfrenchy83
newfrenchy83 committed Oct 16, 2024
1 parent 245576c commit 469c03b
Show file tree
Hide file tree
Showing 4 changed files with 142 additions and 167 deletions.
168 changes: 93 additions & 75 deletions src/units/abilities.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,8 +182,7 @@ bool affects_side(const config& cfg, std::size_t side, std::size_t other_side)
bool unit::get_ability_bool(const std::string& tag_name, const map_location& loc) const
{
for (const config &i : this->abilities_.child_range(tag_name)) {
if (ability_active(tag_name, i, loc) &&
ability_affects_self(tag_name, i, loc))
if (get_self_ability_bool(i, tag_name, loc))
{
return true;
}
Expand All @@ -204,9 +203,7 @@ bool unit::get_ability_bool(const std::string& tag_name, const map_location& loc
if ( &*it == this )
continue;
for (const config &j : it->abilities_.child_range(tag_name)) {
if (affects_side(j, side(), it->side()) &&
it->ability_active(tag_name, j, adjacent[i]) &&
ability_affects_adjacent(tag_name, j, i, loc, *it))
if (get_adj_ability_bool(j, tag_name, i, loc,*it))
{
return true;
}
Expand All @@ -222,8 +219,7 @@ unit_ability_list unit::get_abilities(const std::string& tag_name, const map_loc
unit_ability_list res(loc_);

for(const config& i : this->abilities_.child_range(tag_name)) {
if(ability_active(tag_name, i, loc)
&& ability_affects_self(tag_name, i, loc))
if (get_self_ability_bool(i, tag_name, loc))
{
res.emplace_back(&i, loc, loc);
}
Expand All @@ -244,9 +240,7 @@ unit_ability_list unit::get_abilities(const std::string& tag_name, const map_loc
if ( &*it == this )
continue;
for(const config& j : it->abilities_.child_range(tag_name)) {
if(affects_side(j, side(), it->side())
&& it->ability_active(tag_name, j, adjacent[i])
&& ability_affects_adjacent(tag_name, j, i, loc, *it))
if(get_adj_ability_bool(j, tag_name, i, loc,*it))
{
res.emplace_back(&j, loc, adjacent[i]);
}
Expand Down Expand Up @@ -351,18 +345,6 @@ std::vector<std::tuple<std::string, t_string, t_string, t_string>> unit::ability
return res;
}

namespace {
/**
* Value of unit::num_recursion_ at which allocations of further recursion_guards fail. This
* value is used per unit.
*
*
* With the limit set to 2, all tests pass, but as the limit only affects cases that would otherwise
* lead to a crash, it seems reasonable to leave a little headroom for more complex logic.
*/
constexpr unsigned int UNIT_RECURSION_LIMIT = 3;
};

namespace {
/**
* Print "Recursion limit reached" log messages, including deduplication if the same problem has
Expand Down Expand Up @@ -395,20 +377,20 @@ namespace {
}
}//anonymous namespace

unit::recursion_guard unit::update_variables_recursion() const
unit::recursion_guard unit::update_variables_recursion(const config& ability) const
{
if(num_recursion_ < UNIT_RECURSION_LIMIT) {
return recursion_guard(*this);
if(utils::contains(open_queries_, &ability)) {
return recursion_guard();
}
return recursion_guard();
return recursion_guard(*this, ability);
}

unit::recursion_guard::recursion_guard() = default;

unit::recursion_guard::recursion_guard(const unit & u)
unit::recursion_guard::recursion_guard(const unit & u, const config& ability)
: parent(u.shared_from_this())
{
u.num_recursion_++;
u.open_queries_.emplace_back(&ability);
}

unit::recursion_guard::recursion_guard(unit::recursion_guard&& other)
Expand All @@ -431,19 +413,23 @@ unit::recursion_guard& unit::recursion_guard::operator=(unit::recursion_guard&&
unit::recursion_guard::~recursion_guard()
{
if(parent) {
assert(parent->num_recursion_ > 0);
parent->num_recursion_--;
assert(!parent->open_queries_.empty());
parent->open_queries_.pop_back();
}
}

bool unit::ability_active(const std::string& ability,const config& cfg,const map_location& loc) const
{
unit::recursion_guard filter_lock;
filter_lock = update_variables_recursion();
auto filter_lock = update_variables_recursion(cfg);
if(!filter_lock) {
show_recursion_warning(*this, cfg);
return false;
}
return ability_active_impl(ability, cfg, loc);
}

bool unit::ability_active_impl(const std::string& ability,const config& cfg,const map_location& loc) const
{
bool illuminates = ability == "illuminates";

if (auto afilter = cfg.optional_child("filter"))
Expand Down Expand Up @@ -516,14 +502,6 @@ bool unit::ability_active(const std::string& ability,const config& cfg,const map

bool unit::ability_affects_adjacent(const std::string& ability, const config& cfg,int dir,const map_location& loc,const unit& from) const
{
unit::recursion_guard adj_lock;
if(cfg.has_child("affect_adjacent")){
adj_lock = update_variables_recursion();
if(!adj_lock) {
show_recursion_warning(*this, cfg);
return false;
}
}
bool illuminates = ability == "illuminates";

assert(dir >=0 && dir <= 5);
Expand Down Expand Up @@ -552,14 +530,6 @@ bool unit::ability_affects_adjacent(const std::string& ability, const config& cf
bool unit::ability_affects_self(const std::string& ability,const config& cfg,const map_location& loc) const
{
auto filter = cfg.optional_child("filter_self");
unit::recursion_guard self_lock;
if(filter){
self_lock = update_variables_recursion();
if(!self_lock) {
show_recursion_warning(*this, cfg);
return false;
}
}
bool affect_self = cfg["affect_self"].to_bool(true);
if (!filter || !affect_self) return affect_self;
return unit_filter(vconfig(*filter)).set_use_flat_tod(ability == "illuminates").matches(*this, loc);
Expand Down Expand Up @@ -1383,6 +1353,36 @@ namespace { // Helpers for attack_type::special_active()
return false;
}

/**
* Print "Recursion limit reached" log messages, including deduplication if the same problem has
* already been logged.
*/
void show_recursion_warning(const_attack_ptr& attack, const config& filter) {
// This function is only called when a special is checked for the second time
// filter has already been parsed multiple times, so I'm not trying to optimize the performance
// of this; it's merely to prevent the logs getting spammed. For example, each of
// four_cycle_recursion_branching and event_test_filter_attack_student_weapon_condition only log
// 3 unique messages, but without deduplication they'd log 1280 and 392 respectively.
static std::vector<std::tuple<std::string, std::string>> already_shown;

auto identifier = std::tuple<std::string, std::string>{attack->id(), filter.debug()};
if(utils::contains(already_shown, identifier)) {
return;
}

std::string_view filter_text_view = std::get<1>(identifier);
utils::trim(filter_text_view);
ERR_NG << "Looped recursion error for weapon '" << attack->id()
<< "' while checking weapon special '" << filter_text_view << "'";

// Arbitrary limit, just ensuring that having a huge number of specials causing recursion
// warnings can't lead to unbounded memory consumption here.
if(already_shown.size() > 100) {
already_shown.clear();
}
already_shown.push_back(std::move(identifier));
}

/**
* Determines if a unit/weapon combination matches the specified child
* (normally a [filter_*] child) of the provided filter.
Expand All @@ -1398,7 +1398,7 @@ namespace { // Helpers for attack_type::special_active()
static bool special_unit_matches(unit_const_ptr & u,
unit_const_ptr & u2,
const map_location & loc,
const_attack_ptr weapon,
const_attack_ptr& weapon,
const config & filter,
const bool for_listing,
const std::string & child_tag, const std::string& check_if_recursion)
Expand All @@ -1411,7 +1411,25 @@ namespace { // Helpers for attack_type::special_active()
// need to select an appropriate opponent.)
return true;

auto filter_child = filter.optional_child(child_tag);
//Add wml filter if "backstab" attribute used.
if (!filter["backstab"].blank() && child_tag == "filter_opponent") {
deprecated_message("backstab= in weapon specials", DEP_LEVEL::INDEFINITE, "", "Use [filter_opponent] with a formula instead; the code can be found in data/core/macros/ in the WEAPON_SPECIAL_BACKSTAB macro.");
}
config cfg = filter;
if(filter["backstab"].to_bool() && child_tag == "filter_opponent"){
const std::string& backstab_formula = "enemy_of(self, flanker) and not flanker.petrified where flanker = unit_at(direction_from(loc, other.facing))";
config& filter_child = cfg.child_or_add("filter_opponent");
if(!filter.has_child("filter_opponent")){
filter_child["formula"] = backstab_formula;
} else {
config filter_opponent;
filter_opponent["formula"] = backstab_formula;
filter_child.add_child("and", filter_opponent);
}
}
const config& filter_backstab = filter["backstab"].to_bool() ? cfg : filter;

auto filter_child = filter_backstab.optional_child(child_tag);
if ( !filter_child )
// The special does not filter on this unit, so we pass.
return true;
Expand All @@ -1426,6 +1444,14 @@ namespace { // Helpers for attack_type::special_active()
// If the other unit doesn't exist, try matching without it


attack_type::recursion_guard filter_lock;
if (weapon && (filter_child->optional_child("has_attack") || filter_child->optional_child("filter_weapon"))) {
filter_lock = weapon->update_variables_recursion(filter);
if(!filter_lock) {
show_recursion_warning(weapon, filter);
return false;
}
}
// Check for a weapon match.
if (auto filter_weapon = filter_child->optional_child("filter_weapon") ) {
if ( !weapon || !weapon->matches_filter(*filter_weapon, check_if_recursion) )
Expand Down Expand Up @@ -1610,15 +1636,25 @@ static void get_ability_children(std::vector<special_match>& tag_result,
}
}

bool unit::get_self_ability_bool(const config& special, const std::string& tag_name, const map_location& loc) const
bool unit::get_self_ability_bool(const config& cfg, const std::string& ability, const map_location& loc) const
{
return (ability_active(tag_name, special, loc) && ability_affects_self(tag_name, special, loc));
auto filter_lock = update_variables_recursion(cfg);
if(!filter_lock) {
show_recursion_warning(*this, cfg);
return false;
}
return (ability_active_impl(ability, cfg, loc) && ability_affects_self(ability, cfg, loc));
}

bool unit::get_adj_ability_bool(const config& special, const std::string& tag_name, int dir, const map_location& loc, const unit& from) const
bool unit::get_adj_ability_bool(const config& cfg, const std::string& ability, int dir, const map_location& loc, const unit& from) const
{
auto filter_lock = from.update_variables_recursion(cfg);
if(!filter_lock) {
show_recursion_warning(from, cfg);
return false;
}
const auto adjacent = get_adjacent_tiles(loc);
return (affects_side(special, side(), from.side()) && from.ability_active(tag_name, special, adjacent[dir]) && ability_affects_adjacent(tag_name, special, dir, loc, from));
return (affects_side(cfg, side(), from.side()) && from.ability_active_impl(ability, cfg, adjacent[dir]) && ability_affects_adjacent(ability, cfg, dir, loc, from));
}

bool unit::get_self_ability_bool_weapon(const config& special, const std::string& tag_name, const map_location& loc, const_attack_ptr weapon, const_attack_ptr opp_weapon) const
Expand Down Expand Up @@ -1896,8 +1932,8 @@ bool attack_type::special_active_impl(
unit_const_ptr & def = is_attacker ? other : self;
const map_location & att_loc = is_attacker ? self_loc : other_loc;
const map_location & def_loc = is_attacker ? other_loc : self_loc;
const_attack_ptr att_weapon = is_attacker ? self_attack : other_attack;
const_attack_ptr def_weapon = is_attacker ? other_attack : self_attack;
const_attack_ptr& att_weapon = is_attacker ? self_attack : other_attack;
const_attack_ptr& def_weapon = is_attacker ? other_attack : self_attack;

// Filter firststrike here, if both units have first strike then the effects cancel out. Only check
// the opponent if "whom" is the defender, otherwise this leads to infinite recursion.
Expand All @@ -1907,24 +1943,6 @@ bool attack_type::special_active_impl(
return false;
}

//Add wml filter if "backstab" attribute used.
if (!special["backstab"].blank()) {
deprecated_message("backstab= in weapon specials", DEP_LEVEL::INDEFINITE, "", "Use [filter_opponent] with a formula instead; the code can be found in data/core/macros/ in the WEAPON_SPECIAL_BACKSTAB macro.");
}
config cfg = special;
if(special["backstab"].to_bool()){
const std::string& backstab_formula = "enemy_of(self, flanker) and not flanker.petrified where flanker = unit_at(direction_from(loc, other.facing))";
config& filter_child = cfg.child_or_add("filter_opponent");
if(!special.has_child("filter_opponent")){
filter_child["formula"] = backstab_formula;
} else {
config filter;
filter["formula"] = backstab_formula;
filter_child.add_child("and", filter);
}
}
const config& special_backstab = special["backstab"].to_bool() ? cfg : special;

// Filter the units involved.
//If filter concerns the unit on which special is applied,
//then the type of special must be entered to avoid calling
Expand All @@ -1935,7 +1953,7 @@ bool attack_type::special_active_impl(
if (!special_unit_matches(self, other, self_loc, self_attack, special, is_for_listing, filter_self, self_check_if_recursion))
return false;
std::string opp_check_if_recursion = (applied_both || !whom_is_self) ? tag_name : "";
if (!special_unit_matches(other, self, other_loc, other_attack, special_backstab, is_for_listing, "filter_opponent", opp_check_if_recursion))
if (!special_unit_matches(other, self, other_loc, other_attack, special, is_for_listing, "filter_opponent", opp_check_if_recursion))
return false;
//in case of apply_to=attacker|defender, if both [filter_attacker] and [filter_defender] are used,
//check what is_attacker is true(or false for (filter_defender]) in affect self case only is necessary for what unit affected by special has a tag_name check.
Expand Down
Loading

0 comments on commit 469c03b

Please sign in to comment.