0ct0pu5/wesnoth
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Replace the recursion computation method with the less CPU-intensive config comparison method

Browse source 
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.
This commit is contained in:
newfrenchy83 2024-10-15 20:13:01 +02:00 • committed by GitHub
parent 63d6fbf89d
commit 1c4d2f449f
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 142 additions and 167 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

168
src/units/abilities.cpp
View file

@ -181,8 +181,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;
}
@ -203,9 +202,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;
}
@ -221,8 +218,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);
}
@ -243,9 +239,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]);
}
@ -350,18 +344,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
@ -394,20 +376,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)
@ -430,19 +412,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"))
@ -515,14 +501,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);
@ -551,14 +529,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);
@ -1449,6 +1419,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.
@ -1464,7 +1464,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)
@ -1477,7 +1477,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;
@ -1492,6 +1510,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) )
@ -1680,15 +1706,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
@ -2166,8 +2202,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.
@ -2177,24 +2213,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
@ -2205,7 +2223,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.

80
src/units/attack_type.cpp
View file

@ -47,27 +47,6 @@ static lg::log_domain log_unit("unit");
static lg::log_domain log_wml("wml");
#define ERR_WML LOG_STREAM(err, log_wml)
namespace {
/**
* Value of attack_type::num_recursion_ at which allocations of further recursion_guards fail. This
* value is used per weapon, so if two weapon specials are depending on each other being active then
* with ATTACK_RECURSION_LIMIT = 3 the recursion could go 6 levels deep (and then return false on
* the 7th call to matches_simple_filter).
*
* The counter is checked at the start of matches_simple_filter, and even the first level needs an
* allocation; setting the limit to zero would make matches_simple_filter always return false.
*
* With the recursion limit set to 1, the following tests fail; they just need a reasonable depth.
* event_test_filter_attack_specials
* event_test_filter_attack_opponent_weapon_condition
* event_test_filter_attack_student_weapon_condition
*
* 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 ATTACK_RECURSION_LIMIT = 4;
};
attack_type::attack_type(const config& cfg) :
self_loc_(),
other_loc_(),
@ -130,50 +109,12 @@ std::string attack_type::accuracy_parry_description() const
return s.str();
}
namespace {
/**
* Print "Recursion limit reached" log messages, including deduplication if the same problem has
* already been logged.
*/
void show_recursion_warning(const attack_type& attack, const config& filter) {
// This function is only called when the recursion limit has already been reached, meaning the
// 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_UT << "Recursion limit reached for weapon '" << attack.id()
<< "' while checking filter '" << 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));
}
/**
* Returns whether or not *this matches the given @a filter, ignoring the
* complexities introduced by [and], [or], and [not].
*/
bool matches_simple_filter(const attack_type& attack, const config& filter, const std::string& check_if_recursion)
static bool matches_simple_filter(const attack_type & attack, const config & filter, const std::string& check_if_recursion)
{
//update and check variable_recursion for prevent check special_id/type_active in case of infinite recursion.
attack_type::recursion_guard filter_lock= attack.update_variables_recursion();
if(!filter_lock) {
show_recursion_warning(attack, filter);
return false;
}
const std::set<std::string> filter_range = utils::split_set(filter["range"].str());
const std::string& filter_min_range = filter["min_range"];
const std::string& filter_max_range = filter["max_range"];
@ -339,7 +280,6 @@ bool matches_simple_filter(const attack_type& attack, const config& filter, cons
// Passed all tests.
return true;
}
} // anonymous namespace
/**
* Returns whether or not *this matches the given @a filter.
@ -717,20 +657,20 @@ bool attack_type::describe_modification(const config& cfg,std::string* descripti
return true;
}
attack_type::recursion_guard attack_type::update_variables_recursion() const
attack_type::recursion_guard attack_type::update_variables_recursion(const config& special) const
{
if(num_recursion_ < ATTACK_RECURSION_LIMIT) {
return recursion_guard(*this);
if(utils::contains(open_queries_, &special)) {
return recursion_guard();
}
return recursion_guard();
return recursion_guard(*this, special);
}
attack_type::recursion_guard::recursion_guard() = default;
attack_type::recursion_guard::recursion_guard(const attack_type& weapon)
attack_type::recursion_guard::recursion_guard(const attack_type& weapon, const config& special)
: parent(weapon.shared_from_this())
{
weapon.num_recursion_++;
parent->open_queries_.emplace_back(&special);
}
attack_type::recursion_guard::recursion_guard(attack_type::recursion_guard&& other)
@ -756,8 +696,10 @@ attack_type::recursion_guard& attack_type::recursion_guard::operator=(attack_typ
attack_type::recursion_guard::~recursion_guard()
{
if(parent) {
assert(parent->num_recursion_ > 0);
parent->num_recursion_--;
// As this only expects nested recursion, simply pop the top of the open_queries_ stack
// without checking that the top of the stack matches the filter passed to the constructor.
assert(!parent->open_queries_.empty());
parent->open_queries_.pop_back();
}
}

17
src/units/attack_type.hpp
View file

@ -174,7 +174,7 @@ public:
/**
* Only expected to be called in update_variables_recursion(), which handles some of the checks.
*/
explicit recursion_guard(const attack_type& weapon);
explicit recursion_guard(const attack_type& weapon, const config& special);
public:
/**
* Construct an empty instance, only useful for extending the lifetime of a
@ -204,12 +204,11 @@ public:
* recursion might occur, similar to a reentrant mutex that's limited to a small number of
* reentrances.
*
* This is a cheap function, so no reason to optimise by doing some filters before calling it.
* However, it only expects to be called in a single thread, but the whole of attack_type makes
* that assumption, for example its mutable members are assumed to be set up by the current
* This only expects to be called in a single thread, but the whole of attack_type makes
* that assumption, for example its' mutable members are assumed to be set up by the current
* caller (or caller's caller, probably several layers up).
*/
recursion_guard update_variables_recursion() const;
recursion_guard update_variables_recursion(const config& special) const;
private:
// In unit_abilities.cpp:
@ -427,8 +426,12 @@ private:
int parry_;
config specials_;
bool changed_;
/** Number of instances of recursion_guard that are currently allocated permission to recurse */
mutable unsigned int num_recursion_ = 0;
/**
* While processing a recursive match, all the filters that are currently being checked, oldest first.
* Each will have an instance of recursion_guard that is currently allocated permission to recurse, and
* which will pop the config off this stack when the recursion_guard is finalized.
*/
mutable std::vector<const config*> open_queries_;
};
using attack_list = std::vector<attack_ptr>;

44
src/units/unit.hpp
View file

@ -1742,13 +1742,13 @@ public:
return get_ability_bool(tag_name, loc_);
}
/** Checks whether this unit currently possesses a given ability used like weapon
/** Checks whether this unit currently possesses a given ability, and that that ability is active.
* @return True if the ability @a tag_name is active.
* @param special the const config to one of abilities @a tag_name checked.
* @param tag_name name of ability type checked.
* @param cfg the const config to one of abilities @a tag_name checked.
* @param ability name of ability type checked.
* @param loc location of the unit checked.
*/
bool get_self_ability_bool(const config& special, const std::string& tag_name, const map_location& loc) const;
bool get_self_ability_bool(const config& cfg, const std::string& ability, const map_location& loc) const;
/** Checks whether this unit currently possesses a given ability of leadership type
* @return True if the ability @a tag_name is active.
* @param special the const config to one of abilities @a tag_name checked.
@ -1758,15 +1758,15 @@ public:
* @param opp_weapon the attack used by opponent to unit checked.
*/
bool get_self_ability_bool_weapon(const config& special, const std::string& tag_name, const map_location& loc, const_attack_ptr weapon = nullptr, const_attack_ptr opp_weapon = nullptr) const;
/** Checks whether this unit is affected by a given ability used like weapon
/** Checks whether this unit is affected by a given ability, and that that ability is active.
* @return True if the ability @a tag_name is active.
* @param special the const config to one of abilities @a tag_name checked.
* @param tag_name name of ability type checked.
* @param cfg the const config to one of abilities @a ability checked.
* @param ability name of ability type checked.
* @param loc location of the unit checked.
* @param from unit adjacent to @a this is checked in case of [affect_adjacent] abilities.
* @param dir direction to research a unit adjacent to @a this.
*/
bool get_adj_ability_bool(const config& special, const std::string& tag_name, int dir, const map_location& loc, const unit& from) const;
bool get_adj_ability_bool(const config& cfg, const std::string& ability, int dir, const map_location& loc, const unit& from) const;
/** Checks whether this unit is affected by a given ability of leadership type
* @return True if the ability @a tag_name is active.
* @param special the const config to one of abilities @a tag_name checked.
@ -1869,6 +1869,9 @@ public:
*/
bool ability_matches_filter(const config & cfg, const std::string& tag_name, const config & filter) const;
private:
/**
* Helper similar to std::unique_lock for detecting when calculations such as abilities
* have entered infinite recursion.
@ -1881,7 +1884,7 @@ public:
/**
* Only expected to be called in update_variables_recursion(), which handles some of the checks.
*/
explicit recursion_guard(const unit& u);
explicit recursion_guard(const unit& u, const config& ability);
public:
/**
* Construct an empty instance, only useful for extending the lifetime of a
@ -1905,20 +1908,25 @@ public:
std::shared_ptr<const unit> parent;
};
recursion_guard update_variables_recursion() const;
private:
recursion_guard update_variables_recursion(const config& ability) const;
const std::set<std::string> checking_tags_{"disable", "attacks", "damage", "chance_to_hit", "berserk", "swarm", "drains", "heal_on_hit", "plague", "slow", "petrifies", "firststrike", "poison", "damage_type"};
/**
* Check if an ability is active.
* Check if an ability is active. Includes checks to prevent excessive recursion.
* @param ability The type (tag name) of the ability
* @param cfg an ability WML structure
* @param loc The location on which to resolve the ability
* @returns true if it is active
*/
bool ability_active(const std::string& ability, const config& cfg, const map_location& loc) const;
/**
* Check if an ability is active. The caller is responsible for preventing excessive recursion, so must hold a recursion_guard.
* @param ability The type (tag name) of the ability
* @param cfg an ability WML structure
* @param loc The location on which to resolve the ability
* @returns true if it is active
*/
bool ability_active_impl(const std::string& ability, const config& cfg, const map_location& loc) const;
/**
* Check if an ability affects adjacent units.
@ -2057,8 +2065,12 @@ private:
std::string role_;
attack_list attacks_;
/** Number of instances of recursion_guard that are currently allocated permission to recurse */
mutable unsigned int num_recursion_ = 0;
/**
* While processing a recursive match, all the filters that are currently being checked, oldest first.
* Each will have an instance of recursion_guard that is currently allocated permission to recurse, and
* which will pop the config off this stack when the recursion_guard is finalized.
*/
mutable std::vector<const config*> open_queries_;
protected:
// TODO: I think we actually consider this to be part of the gamestate, so it might be better if it's not mutable,