Fix of #173 makes Lua unit (proxy variable) invalid after update_stats()

[edwardspec]

Deleting [unstore_unit] in https://github.com/Dugy/Legend_of_the_Invincibles/commit/40cad2b2eb9df889ef676d5bec777005b6dc379c (fix for #173) could have broken something. Here is a testcase:

u = wesnoth.get_units()[1]
wesnoth.update_stats(u.__cfg)
wesnoth.log("error", "u.valid = " .. tostring(u.valid))

Before this change, u.valid was "map", after, u.valid is nil.

[edwardspec]

194 has a workaround for this.

(I didn't investigate why exactly is unstore_unit needed there, but it fixes the problem)

[Dugy]

It is caused by the removal of the [unstore_unit] block. I don't know why and it should better be done in the lua code of wesnoth.update_stats() (as I just learned you did).

However, why does it matter that the unit was invalidated? It's better to work with the WML table representation of the unit, otherwise you are just repeatedly unstoring it and storing it again.

[edwardspec]

Because it's really bad for code readability to have to return "new unit" from methods like loti.item.on_unit.remove(), and then replace all unit variables that could have referred to this unit in the code that called it.

[Dugy]

I am not sure if I understand what you mean by that. You cannot keep any unit variables if you are working with the unit type rather than its WML table representation.

[edwardspec]

Unit variable is a "proxy object". It remains valid as long as there is a unit with the same underlying_id.

[edwardspec]

with the same underlying_id.

(or rather a very outdated wiki page says so. Maybe it's just "id" now, need to test to be sure)

[Dugy]

Aha, that is what you mean. Why do you need the unit variable? Almost any work with it requires changing it into a WML table.

My intended approach is that a WML table is obtained out of it when it's called from WML, then all the stuff is done with it as with a WML table and then it's unstored. Using the unit variable requires copious use of unnecessary storing.

[edwardspec]

This is a premature optimization.

Why do you need the unit variable?

Because it has some nice APIs, which result in shorter, more readable and less error-prone code than manipulating the internal arrays?

Almost any work with it requires changing it into a WML table.

First, there are many things that can be done via the unit APIs. Adding/removing items, moving the unit, listing attacks, etc. Such code is often cleaner than code that works with WML.

The only place that absolutely needs the WML table is update_stats() itself.

Second, if you are worried about performance, unit.something() is faster (because it is directly mapped to the C++ function), and the C++ code that serializes unit into unit.__cfg doesn't look like a performance bottleneck.

Using the unit variable requires copious use of unnecessary storing.

I can write a benchmark if you want (how many store/unstore operations can be done in 1 second), but I'm sure that we are talking about like 2ms. of time per update_stats(). This is not the kind of optimization that justifies longer code.

[Dugy]

Execution speed was not my only concern. If some functions use WML arrays and others use proxy variables, it's easy to accidentally use the wrong one (errors of this kind manifest fast enough). If most of the more complex modifications need to convert the proxy array into a WML array, using proxy arrays results in more code. It works differently on units on the recall list and again differently on units stored by WML code. And as you have seen, unit invalidation that breaks all proxy variables can happen quite unpredictably (I have found no rationale why should omitting that [unstore_unit] invalidate it).

There are not many API functions that could be beneficial here. When I look at the documentation, only wesnoth.add_modification() and wesnoth.remove_modifications() are needed here. Others are useful elsewhere, for storing/unstoring units, AI or some ability-related code.

Now, both wesnoth.add_modification() and wesnoth.remove_modifications(), while executed inherently faster by virtue of being written in C++, also do a lot of unnecessary stuff. While wesnoth.add_modification() is useful for adding a tag to the unit's [modifications], it also applies the effect to the unit, which is a much more time-consuming operation which may include calling a lua-based function (because of the possibility to add functions to wesnoth.effects, which I plan to use later) - the same code will be executed in the following update_stats() call. The other function, wesnoth.remove_modifications(), is even heavier, because it removes one modification and applies all the remaining ones - which is quite pointless to be done in a loop after removing each item (which you do in loti.item.util.undress_unit()). Everything in modifications is stored as a WML table in C++, so it cannot be done too fast anyway. So there isn't much of a performance gain, if there even is any, at using the C++ bound functions for this. It might be better here to reimplement both these functions as loti.on_unit.add_modification() and loti.on_unit.remove_modification(), which would not these unnecessary operations and might work with WML tables instead of proxy units.

I am also considering a change in the location where items and advancements are stored, because the modifications table is read a bit too often since it may contain also stuff that changes the visuals (like image_mod or new_animation), which would make these two function unusable. However, I would need to experiment first to see if this has any benefit, as wesnoth takes 100% of all of my CPUs even if I don't play LotI (which I learned very recently).

While premature optimization is a common issue, this isn't a case. Caring about performance isn't necessarily premature optimization. When considering if something is premature optimization, you also have to consider that there is another anti-pattern caused premature pessimization, when one is choosing from two approaches with similar readability, design quality and difficulty and chooses the approach that is less efficient just because it requires less work with what is already written, is slightly shorter or some other reasons.

[edwardspec]

it's easy to accidentally use the wrong one (errors of this kind manifest fast enough)

They immediately manifest when testing. It's not like we commit any code that we never tried to run. Also parameters of functions can be described in a comment (as you've seen me doing) - avoids a lot of headache, with Lua's weak typing and all.

I am also considering a change in the location where items and advancements are stored, because the modifications table is read a bit too often

Now, that may actually be a serious optimization.

We can store them in a Lua array (with indexes by whatever criteria we use when searching for them, which would be super-fast), and then use wesnoth.persistent_tags to ensure that this array is serialized on Save and restored on Load.

wesnoth takes 100% of all of my CPUs even if I don't play LotI

... which is a problem with Wesnoth. (we can even investigate the cause. Maybe most of CPU usage is just drawing animations, or maybe it's a bug in Wesnoth that we can find/fix, or maybe it's just swapping)

[edwardspec]

Regarding the performance, I will run some benchmarks on things like update_stats(), unit.__cfg, unit.remove_modification(), etc. to know exactly how much they contribute to "overall slowness".

[Dugy]

We can store them in a Lua array (with indexes by whatever criteria we use when searching for them, which would be super-fast), and then use wesnoth.persistent_tags to ensure that this array is serialized on Save and restored on Load.

I didn't think of doing it this way. This would allow storing only the numbers and possibly crafted sorts for items and ids and counts for advancements and grab all the rest from the unit types and prevent them from being duplicated at the start of every save file.

It should greatly help tackle the size of save files. They would end up bloated only by history and by the repeated weapon specials (which possibly can't be dealt with without some really bad magic).

Therefore, I recommend creating a set of functions to access unit inventories that would be eventually used exclusively to access them. At that point, we might start changing the way items are stored.

May I close this issue and continue discussion here? I have accepted your pull request fixing this problem anyway.

[edwardspec]

Sure. Can also open a new issue (since it's a new feature idea) for this persistent_tags optimization.

[edwardspec]

Here are the promised benchmarks.

Conditions: CPU: Pentium 3805U (1.90GHz) OS: Fedora Linux 28 with xfce4 desktop

Tested Wesnoth screen resolution: 960x600. Wesnoth preferences: "Limit FPS" enabled, all animations enabled. Wesnoth is running with SCHED_RR realtime scheduler (chrt -r 1). Swap partitions are disabled to be sure that no slowdown is caused by swapping. Wesnoth runs in Debug mode.

Tested Wesnoth versions: 1) Wesnoth 1.14.5, as pre-packaged in Fedora Linux. 2) Wesnoth (master branch, v1.15.0-dev), compiled by latest GCC with maximum optimizations, including LTO.

Wesnoth cache was populated (same save file was loaded into Wesnoth, then Wesnoth was closed, then Wesnoth was opened to run the benchmark) and not outdated.

Several unit-specific functions were tested. The following units were used: 1) Ancient Lich (no items) selected in Gladiators scenario (1 human player). 2) Efraim from attached save file. BENCHMARK.gz 3) Lethalia from attached save file. 4) Elvish Nightprowler from attached save file. 5) Dwarvish Battlerager from attached save file.

Each test measured how many times can the tested function be completed in 30 seconds. The following Lua functions were benchmarked:

1) Serialization of unit into WML table: unit.cfg.moves = 2 - unit.cfg.moves (at the beginning, __cfg.moves are 1)

2) wesnoth.update_stats(unit_wml) 3) wesnoth.update_stats(unit.__cfg)

4) wesnoth.add_modification(unit, "object", loti.item.type[100]) ... and wesnoth.remove_modifications(unit, { number = 100 })

These functions have to be tested together, because adding many thousands of modifications to the same unit will exhaust available memory, and creating a temporary copy of unit before each add_modification() would impact the results.

---

Results:

RUN #1: Wesnoth (master) + non-geared unit in Gladiators

$ loti.run_benchmarks(wesnoth.get_unit("hero1"))
Testing unit Anonymous player 1 (09 Ancient Lich)...
Benchmark #1: switch between 1 and 2 moves via unit.__cfg:
Benchmark: did 39533 in 30 second(s).
Benchmark #2: running update_stats() on unit WML table:
Benchmark: did 5486 in 30 second(s).
Benchmark #3: running update_stats() on unit.__cfg:
Benchmark: did 5098 in 30 second(s).
Benchmark #4: add and then delete an item via add_modification()/remove_modifications():
Benchmark: did 162235 in 30 second(s).

RUN #2: Wesnoth (1.14.5) + non-geared unit in Gladiators

$ loti.run_benchmarks(wesnoth.get_unit("hero1"))
Testing unit Anonymous player 1 (09 Ancient Lich)...
Benchmark #1: switch between 1 and 2 moves via unit.__cfg:
Benchmark: did 39260 in 30 second(s).
Benchmark #2: running update_stats() on unit WML table:
Benchmark: did 5204 in 30 second(s).
Benchmark #3: running update_stats() on unit.__cfg:
Benchmark: did 4844 in 30 second(s).
Benchmark #4: add and then delete an item via add_modification()/remove_modifications():
Benchmark: did 155130 in 30 second(s).

RUN #3: Wesnoth (1.14.5) + real scenario with heavily geared units

$ loti.run_benchmarks(wesnoth.get_unit("Efraim")); loti.run_benchmarks(wesnoth.get_unit("Lethalia")); loti.run_benchmarks(wesnoth.get_unit("Elvish Avenger-2365")); loti.run_benchmarks(wesnoth.get_unit("gladiator3"))
Testing unit Efraim (Efraim_god)...
Benchmark #1: switch between 1 and 2 moves via unit.__cfg:
Benchmark: did 4786 in 30 second(s).
Benchmark #2: running update_stats() on unit WML table:
Benchmark: did 604 in 30 second(s).
Benchmark #3: running update_stats() on unit.__cfg:
Benchmark: did 563 in 30 second(s).
Benchmark #4: add and then delete an item via add_modification()/remove_modifications():
Benchmark: did 3232 in 30 second(s).
Testing unit Lethalia (Lethalia_god)...
Benchmark #1: switch between 1 and 2 moves via unit.__cfg:
Benchmark: did 5692 in 30 second(s).
Benchmark #2: running update_stats() on unit WML table:
Benchmark: did 777 in 30 second(s).
Benchmark #3: running update_stats() on unit.__cfg:
Benchmark: did 732 in 30 second(s).
Benchmark #4: add and then delete an item via add_modification()/remove_modifications():
Benchmark: did 3820 in 30 second(s).
Testing unit 1 Nirér from Hidden Hideout (Elvish Nightprowler)...
Benchmark #1: switch between 1 and 2 moves via unit.__cfg:
Benchmark: did 10875 in 30 second(s).
Benchmark #2: running update_stats() on unit WML table:
Benchmark: did 818 in 30 second(s).
Benchmark #3: running update_stats() on unit.__cfg:
Benchmark: did 780 in 30 second(s).
Benchmark #4: add and then delete an item via add_modification()/remove_modifications():
Benchmark: did 10056 in 30 second(s).
Testing unit 1 Алдрол the Slayer of the Puny (Dwarvish Battlerager)...
Benchmark #1: switch between 1 and 2 moves via unit.__cfg:
Benchmark: did 13795 in 30 second(s).
Benchmark #2: running update_stats() on unit WML table:
Benchmark: did 821 in 30 second(s).
Benchmark #3: running update_stats() on unit.__cfg:
Benchmark: did 801 in 30 second(s).
Benchmark #4: add and then delete an item via add_modification()/remove_modifications():
Benchmark: did 11954 in 30 second(s).

RUN #4: Wesnoth (master) + real scenario (warning: save from 1.14.5) with heavily geared units

$ loti.run_benchmarks(wesnoth.get_unit("Efraim")); loti.run_benchmarks(wesnoth.get_unit("Lethalia")); loti.run_benchmarks(wesnoth.get_unit("Elvish Avenger-2365")); loti.run_benchmarks(wesnoth.get_unit("gladiator3"))
Testing unit Efraim (Efraim_god)...
Benchmark #1: switch between 1 and 2 moves via unit.__cfg:
Benchmark: did 5039 in 30 second(s).
Benchmark #2: running update_stats() on unit WML table:
Benchmark: did 595 in 30 second(s).
Benchmark #3: running update_stats() on unit.__cfg:
Benchmark: did 571 in 30 second(s).
Benchmark #4: add and then delete an item via add_modification()/remove_modifications():
Benchmark: did 3167 in 30 second(s).
Testing unit Lethalia (Lethalia_god)...
Benchmark #1: switch between 1 and 2 moves via unit.__cfg:
Benchmark: did 6110 in 30 second(s).
Benchmark #2: running update_stats() on unit WML table:
Benchmark: did 619 in 30 second(s).
Benchmark #3: running update_stats() on unit.__cfg:
Benchmark: did 587 in 30 second(s).
Benchmark #4: add and then delete an item via add_modification()/remove_modifications():
Benchmark: did 3533 in 30 second(s).
Testing unit 1 Nirér from Hidden Hideout (Elvish Nightprowler)...
Benchmark #1: switch between 1 and 2 moves via unit.__cfg:
Benchmark: did 11336 in 30 second(s).
Benchmark #2: running update_stats() on unit WML table:
Benchmark: did 632 in 30 second(s).
Benchmark #3: running update_stats() on unit.__cfg:
Benchmark: did 608 in 30 second(s).
Benchmark #4: add and then delete an item via add_modification()/remove_modifications():
Benchmark: did 10460 in 30 second(s).
Testing unit 1 Алдрол the Slayer of the Puny (Dwarvish Battlerager)...
Benchmark #1: switch between 1 and 2 moves via unit.__cfg:
Benchmark: did 14084 in 30 second(s).
Benchmark #2: running update_stats() on unit WML table:
Benchmark: did 638 in 30 second(s).
Benchmark #3: running update_stats() on unit.__cfg:
Benchmark: did 619 in 30 second(s).
Benchmark #4: add and then delete an item via add_modification()/remove_modifications():
Benchmark: did 12338 in 30 second(s).

[edwardspec]

For the most heavily geared unit (Efraim) in Wesnoth 1.14.5, update_stats(wml) took 0.04967 seconds and update_stats(unit.__cfg) took 0.05329 seconds. Adding and then removing a modification via add_modification()/remove_modifications() took 0.00947 seconds.

[Dugy]

The add_modification()/remove_modifications() functions will be significantly slower when I use wesnoth.effects to add new effects instead of the current method. But the benchmarks clearly show that the .__cfg conversion far faster than most lua functions. The time of remove_modifications() clearly depends on the number of items on the unit and can be longer than the .__cfg conversion if there are enough items.

What you didn't benchmark and was possibly the main point of the argument: the time to add or remove a modification from the WML table compared to the time of a add_modification()/remove_modifications() call. However, the argument became pointless once you got the idea with wesnoth.persistence_tags.

EDIT: What I didn't realise at the start is that it shows how much time is spent working with the units while they have so many modifications.