0000764: Random crashes thanks to bonus system

Notes
(0001840) beegee (developer) 2011-07-05 18:31	Thanks much that you informed me about problems concerning caching of the bonus system. Sorry about that it drives you crazy, I've implemented it carefully but as you know the bonus system isn't trivial and the interactions with them too. So I'll kill them all those evil bugs in the following weekends. =) I've noticed these random crashes only when I've played against AI, in hotseat no problem. In the newest revision, I've disabled bonus caching.

(0001842) beegee (developer) 2011-07-05 19:14	OK, I've detected where the bug comes from. Some simple re-arrangement of the code is necessary. Just want to know, do you noticed any speed gains through the bonus caching? Without I get 25 FPS in a non-siege battle, with caching I get 46-48 fps.

(0001843) Warmonger (administrator) 2011-07-05 19:20	Yes, it helps a lot. Simply no more waiting for every stack to move.

(0001844) Tow dragon (viewer) 2011-07-07 10:17	I think we just shouldn't cache bonuses, it causes too much trouble. And there are better ways of gaining speedup like turning off STL debugging or implementing "subset bonus retrieval". Bonus system-side caching is much more troublesome than it's worth. And you can always cache your queries in place where you need the knowledge about bonuses (e.g. make flag curStakckFlies in battle interface) --- it's much simpler and more transparent, because the battle interface is what cares about validity of cache (bonus system just can't do that without heavy assumptions).

(0001846) beegee (developer) 2011-07-13 18:54	Sorry but I had to do it, the bonus system caching system is now much more robust(>99%) and more important thread-safe. It should work without any problems. Warmonger could you test it if the bugs you recognized are fixed? I've implemented thread safety really simple, perhaps it could be done better, but I haven't much experience about it(i'll look further into it some day=)). So it's a little bit slower than the original implemantion, but anyway it's much more faster than without bonus caching. @Tow dragon: Turning off STL debugging particurly _HAS_ITERATOR_DEBUGGING is not possible due to many complication. There are reported bugs for Visual Studio concerning that point. I don't have the link ATM, but it's true. Also every binary have to be build with that macro, also every boost lib. So changing BOOST_FOREACH to simple for loops and list STL containers to vector STL containers just for performance-critical functions should do the trick. What do you meant with subset bonus retrieval?

(0001848) Warmonger (administrator) 2011-07-13 19:14	I think it's working. However, thread-unsafe problems rarely got properly solved ;) so I leave this bug for now.

(0001849) Tow (developer) 2011-07-14 17:21 edited on: 2011-07-14 17:22	Bonus caching concept has two elemental flaws. Firstly, the underlying issue (performance in debug) has different cause — STL implementation in MSVC. This in conjunction with flaws in GUI (costly checks whether stack is a catapult at every cursor movement, etc) was indeed causing annoying slowdowns, however it was not a problem for release builds. Introducing so much additional logic just for sake of debug builds performance looks like an overkill, especially when much simpler solutions should also do the trick. (The few actual chokepoints can be buffered on GUI/engine; replacing STL list with another list implementation should also help). Secondly, bonus caching system is based on assumption that is not entirely valid and we rather don't want to make it valid. The assumption is that result of a bonus query is dependant only on that query and bonus nodes „tree” state. However we have limiters that can rely on other elements of gamestate (eg. creature type tier, native terrain -> that's data outside bonus system) and we generally intended on introducing more such type limiters in future. That means that result of bonus query can change because of change in other parts of game state (which changes are not „visible” for bonus system). However other parts of engine can often assume that gamestate (including bonus „tree” state) won't change for some time (GUI obtains mutex on GS when processing input). So if any caching is to work, it should be turned on/off each time by asker (GUI tells bonus system, that between two calls to given funcs, gamestate won't change -> then no checking on the bonus system is needed at all.) @Thread-safety That won't work, you have a global state that is left unprotected. Consider following scenario with two threads A and B trying to parallely access cache. A: setCachingStr("a"); B: setCachingStr("b"); A: enters getAllBonuses, obtains a mutex, returns results meant for thread B (invalid!), clears cachingStr B: enters getAllBonuses, makes full look-up and returns valid results >>What do you meant with subset bonus retrieval? You shouldn't have asked :P

(0001850) Warmonger (administrator) 2011-07-16 14:32	It doesn't make much sense to cache bonuses without limiters, as currently major part of bonuses (coming from stack experience) is handled by them. On the other hand, stack experience certainly won't change during battle.

(0001851) beegee (developer) 2011-07-16 14:49	Sure, there are simpler solutions to archieve a faster bonus system performance but I think it's better in the long-term to create a global cache than to store often used bonus request results several times here and there. Second point is that you have to manually validate those stored information when you know that the bonus system has been changed. With the revision 2252 the bonus caching system doesn't cache results of limiter objects. Normally everything which gets changed in the bonus system and which is visible for the caching system get's now cached. Thread-safety was already correctly implemented in my previous commit. I completely forgot about that point in the first version of the bonus caching system. Thread-safety is programmed in the following way: It gets exclusive access in the getAllBonuses method where all cached data are read or written. So this should be correct, but I don't know if it could be done better because acquiring a exclusive lock every frame could be a bit slow on network/internet games with several players. It's runs fast on a single player game or a hotseat game. With Threading Building Blocks(TBB) or with C++0x in some years it could be done a bit faster due to atomic data types. The latest commit to the bonus caching system increased performance when moving the cursor in battle. Have a nice day =)

(0002240) Warmonger (administrator) 2012-02-27 12:26	Seems already fixed. Please reopen if you discover random crashes at the beginning of stacks' turn.

Issue History
Date Modified	Username	Field	Change
2011-07-05 06:12	Warmonger	New Issue
2011-07-05 06:12	Warmonger	Status	new => assigned
2011-07-05 06:12	Warmonger	Assigned To	=> beegee
2011-07-05 06:12	Warmonger	File Added: Testy4 Power Rating.h3m
2011-07-05 18:31	beegee	Note Added: 0001840
2011-07-05 19:14	beegee	Note Added: 0001842
2011-07-05 19:20	Warmonger	Note Added: 0001843
2011-07-07 10:17	Tow dragon	Note Added: 0001844
2011-07-13 18:54	beegee	Note Added: 0001846
2011-07-13 19:14	Warmonger	Note Added: 0001848
2011-07-14 17:21	Tow	Note Added: 0001849
2011-07-14 17:22	Tow	Note Edited: 0001849	View Revisions
2011-07-16 14:32	Warmonger	Note Added: 0001850
2011-07-16 14:49	beegee	Note Added: 0001851
2012-02-27 12:26	Warmonger	Note Added: 0002240
2012-02-27 12:26	Warmonger	Status	assigned => resolved
2012-02-27 12:26	Warmonger	Fixed in Version	=> 0.87
2012-02-27 12:26	Warmonger	Resolution	open => fixed
2012-02-27 12:51	Warmonger	Status	resolved => closed
2012-03-01 23:31	Tow	Relationship added	related to 0000823