Filter refactor

[gfgtdf]

This refactors the unit filter so that it now parses as much as possible when the filter is created to speed up the filter::matches function. This also make the provious optimisation of havign a 'nil' filter unnecessary.

Also i did comepletey reworite these files so looking at the diff probably won't be handy.

[gfgtdf]

Ok Now that i was able to configure my fork to run appvveyor builds, i was able to benhmark this and it does really increase performance, testing code:

local mainline_units = {"Blood Bat", "Dread Bat", "Vampire Bat", "Boat", "Galleon", "Pirate Galleon", "Transport Galleon", "Drake Arbiter", "Armageddon Drake", "Drake Blademaster", "Drake Burner", "Drake Clasher", "Drake Enforcer", "Drake Fighter", "Fire Drake", "Drake Flameheart", "Drake Flare", "Drake Glider", "Hurricane Drake", "Inferno Drake", "Sky Drake", "Drake Thrasher", "Drake Warden", "Drake Warrior", "Dwarvish Arcanister", "Dwarvish Berserker", "Dwarvish Dragonguard", "Dwarvish Explorer", "Dwarvish Fighter", "Dwarvish Guardsman", "Dwarvish Lord", "Dwarvish Pathfinder", "Dwarvish Runemaster", "Dwarvish Runesmith", "Dwarvish Scout", "Dwarvish Sentinel", "Dwarvish Stalwart", "Dwarvish Steelclad", "Dwarvish Thunderer", "Dwarvish Thunderguard", "Dwarvish Ulfserker", "Elvish Archer", "Elvish Avenger", "Elvish Captain", "Elvish Champion", "Elvish Druid", "Elvish Enchantress", "Elvish Fighter", "Elvish Hero", "Elvish High Lord", "Elvish Lady", "Elvish Lord", "Elvish Marksman", "Elvish Marshal", "Elvish Outrider", "Elvish Ranger", "Elvish Rider", "Elvish Scout", "Elvish Shaman", "Elvish Sharpshooter", "Elvish Shyde", "Elvish Sorceress", "Elvish Sylph", "Direwolf Rider", "Goblin Impaler", "Goblin Knight", "Goblin Pillager", "Goblin Rouser", "Goblin Spearman", "Wolf Rider", "Gryphon", "Gryphon Master", "Gryphon Rider", "Horseman", "Grand Knight", "Knight", "Lancer", "Paladin", "Bowman", "Cavalier", "Cavalryman", "Dragoon", "Duelist", "Fencer", "General", "Grand Marshal", "Halberdier", "Heavy Infantryman", "Iron Mauler", "Javelineer", "Lieutenant", "Longbowman", "Master at Arms", "Master Bowman", "Pikeman", "Royal Guard", "Sergeant", "Shock Trooper", "Spearman", "Swordsman", "Mage", "Arch Mage", "Elder Mage", "Great Mage", "Mage of Light", "Red Mage", "Silver Mage", "White Mage", "Outlaw", "Assassin", "Bandit", "Footpad", "Fugitive", "Highwayman", "Rogue", "Ruffian", "Thief", "Thug", "Peasant", "Royal Warrior", "Woodsman", "Huntsman", "Poacher", "Ranger", "Trapper", "Arif", "Batal", "Elder Falcon", "Falcon", "Faris", "Ghazi", "Hadaf", "Hakim", "Jawal", "Jundi", "Khaiyal", "Khalid", "Mighwar", "Monawish", "Mudafi", "Mufariq", "Muharib", "Naffat", "Qanas", "Qatif_al_nar", "Rami", "Rasikh", "Saree", "Shuja", "Tabib", "Tineen", "Mermaid Diviner", "Mermaid Enchantress", "Merman Entangler", "Merman Fighter", "Merman Hoplite", "Merman Hunter", "Mermaid Initiate", "Merman Javelineer", "Merman Netcaster", "Mermaid Priestess", "Mermaid Siren", "Merman Spearman", "Merman Triton", "Merman Warrior", "Cuttle Fish", "Fire Dragon", "Fire Guardian", "Giant Mudcrawler", "Giant Rat", "Giant Scorpion", "Giant Spider", "Mudcrawler", "Sea Serpent", "Skeletal Dragon", "Tentacle of the Deep", "Water Serpent", "Wolf", "Direwolf", "Great Wolf", "Yeti", "Naga Fighter", "Naga Myrmidon", "Naga Warrior", "Ogre", "Young Ogre", "Orcish Archer", "Orcish Assassin", "Orcish Crossbowman", "Orcish Grunt", "Orcish Leader", "Orcish Ruler", "Orcish Slayer", "Orcish Slurbow", "Orcish Sovereign", "Orcish Warlord", "Orcish Warrior", "Saurian Ambusher", "Saurian Augur", "Saurian Flanker", "Saurian Oracle", "Saurian Skirmisher", "Saurian Soothsayer", "Great Troll", "Troll Hero", "Troll Rocklobber", "Troll", "Troll Shaman", "Troll Warrior", "Troll Whelp", "Ghast", "Ghoul", "Necrophage", "Soulless", "Walking Corpse", "Necromancer", "Ancient Lich", "Dark Adept", "Dark Sorcerer", "Lich", "Skeleton", "Skeleton Archer", "Banebow", "Bone Shooter", "Chocobone", "Deathblade", "Death Knight", "Draug", "Revenant", "Ghost", "Nightgaunt", "Shadow", "Spectre", "Wraith", "Ancient Wose", "Elder Wose", "Wose"}

for x = 10, 30 do
	for y = 10, 30 do
		local unit_type = mainline_units[wesnoth.random(#mainline_units)]
		local unit_side = wesnoth.random(1,2)
		wesnoth.put_unit { x= x, y=y, type = unit_type, side = unit_side }
	end
end
-- old: 
-- { level = 1 }: 16
-- { formula = "level = 1" }: 24

-- new: 
-- { level = 1 }: 0-1
-- { formula = "level = 1" }: 1-2
for i = 1, 20 do
	local stamp1 = wesnoth.get_time_stamp()
	local n_units = #wesnoth.get_units { formula = "level = 1" }
	local stamp2 = wesnoth.get_time_stamp()
	print("n_units = " .. n_units .. " time = " .. (stamp2 - stamp1))
end

running for example a simple formula = "level = 1" filter on 400 units took bwteen 24 and 25 time units, while with the new method it only takes betwen 1 and 2 time units (time unit = whatever get_time_stamp uses).

Of course this code only really increases performance if the filter is creates once and then run many times, so in order to make this trul useful we should:

1. add a lua function to create a 'compiled' filter
2. make wml event hander cache it's event filters.

[Vultraz]

Let's get this in for 1.13.9.

[gfgtdf]

I also wanted to do the same for location filter, but that is far more diffucult (due to many other mor or less useful optimisations in that file), my new plan on how to handle terrain filter is to make the engine less guessing on what would be more efficient, and instead add a new terrain filter tag [where] that behaves like where in formula or other functional languages, an example:
the filter

[store_locations]
  variable="var"
  [filter_adjacent_loction]
     terrain="Ww"
  [/filter_adjacent_loction]
[/store_locations]

would then just check for every loction for every adjacent loction whether it matches terrain="Ww" while the filter

[store_locations]
  variable="var"
  [filter_adjacent_loction]
     find_in="water"
  [/filter_adjacent_loction]
  [where]
     id="water"
     terrain="Ww"
  [/where]
[/store_locations]

would first cache all water tiles and then chesh for each tile wheterh it is ajacent to one of those just cached.

[CelticMinstrel]

Okay, so from my initial read-through, I have three comments:

• I don't like the u.u.xxx stuff. Basically I'd prefer if the unit_filter_args argument was named something other than u.
• In some cases, several attributes are handled in nearly-identical ways. Might it not be better to refactor those out to functors or template functions, rather than inline lambdas?
• I'm not entirely convinced you're implementing find_in correctly. The presence of a find_in key basically means the filter is matched only against the units in the list, so it strikes me as a little strange that it's handled exactly the same as for all the attributes that test something about the unit. I haven't looked really closely, though, so maybe it's fine.

I feel like there was another thing, but if so, I forgot it.

Also, I'd like it if you can give an overview of how the new system works.

EDIT: Also, I'm not sure I like the [where] proposal.

[gfgtdf]

Also, I'd like it if you can give an overview of how the new system works.

Basically filter objects have a vector<unique_ptr<abstract_filter>> and for each attribute in the filter a correspsiong abstract_filter-subclass instance is added to that vector,

I'm not entirely convinced you're implementing find_in correctly

I think i did it exactly the same way as it was implemented before,

[CelticMinstrel]

I mean, maybe find_in should be a separate class (like the unit_filter_xy class or whatever) rather than handled as a filter attribute.

[gfgtdf]

Note that there currently is no more efficient way to get a unit by id (which is what the variabel contains), even wensoth.get_unit(id) does simply iterate though all units and checks whether it matches the given id, so i don't think your suggestion would increase performance.

Anyways i'm still more wrried abotu the terrain filter now, not sure how to handle it.

[gfgtdf]

Another interesting thing i found out is that using variables inside filters does have a noticable impact on performance, here the results (a variable a=1 was set before):

-- old: 
-- { level = 1 }: 16
-- { level = "$a" }: 31
-- new: 
-- { level = 1 }: 0-1
-- { level = "$a" }: 8

Note how even with the old method the filter with the variables takes nearly twice as long asthe one without the variable (16 ticks longer), and even with the new method it takes 7 ticks longer than without.

[Vultraz]

Could we add a more efficient way to get a unit by id?

[gfgtdf]

we could add a map unit id -> underlying id in the unit map structure, but it'd also need to be updated which woudl make the unit_map code more cmplicated and also require some cputime to update it, not sure whether that'd be a noticable optimisation in total, so for now i probalby won't do it.

[CelticMinstrel]

Fine, but could you at least rename all the unit_filter_args u to something else? Maybe unit_filter_args args would be good.

Note that there currently is no more efficient way to get a unit by id (which is what the variabel contains), even wensoth.get_unit(id) does simply iterate though all units and checks whether it matches the given id, so i don't think your suggestion would increase performance.

Okay sure, but what does that have to do with find_in? Using find_in means you're matching a unit from a list. There's no need to iterate over the map at all.

Another interesting thing i found out is that using variables inside filters does have a noticable impact on performance,

Umm... you have a strange definition of "noticeable". I highly doubt the user would notice 16 ticks, let alone 7 ticks. Sure, if there are a lot of variables and the delays build up, it could become noticeable, but...

[gfgtdf]

Fine, but could you at least rename all the unit_filter_args u to something else?

I could but it's currently not my priority.

Okay sure, but what does that have to do with find_in? Using find_in means you're matching a unit from a list. There's no need to iterate over the map at all.

The list is a list of unit ids, in order ot get the units objects form those ids you need to do what wesnto,get_unit does.

[CelticMinstrel]

Pretty sure the list is typically an array of stored units, not just their IDs. (Admittedly, there may be cases where people took advantage of the fact that the game only tested the ID.)

[gfgtdf]

Pretty sure the list is typically an array of stored units, not just their IDs. (Admittedly, there may be cases where people took advantage of the fact that the game only tested the ID.)

Creating units form the variable woudl sureley be much slower than looing it up by the id, so i don't see how that'd matter here,

[CelticMinstrel]

Technically you wouldn't have to create actual units from the config in order to determine if it matches the filter... but whatever.

[gfgtdf]

ok, i just tested again, and was suprised that this even (although marginally, from 0.532 to 0.433 ticks ) speedsup filter for filters that are just run once. (tested with a simple { level = 1} filter). Since it does noticeabley speedup filters that are ran multiple times and does not slowdown filters that are just chekd once, i'll probably merge this before i get to do the stuff i mentioned above.

[CelticMinstrel]

Why is this PR all messed up suddenly? Did you merge master in rather than rebasing?

[gfgtdf]

i did a git pull -r wesnoth master iirc that's what i also used before

[gfgtdf]

hmm now i did a git pull -r wesnoth master again and the mess vanished, no idea why that happend

[CelticMinstrel]

Oh right, could you please rename all the const unit_filter_args& u? I think a better name would be "args" or, if you insist on a single-letter name, "f".