Hi all First, here's a quote by thonsew where he's writing some stuff about his token changes on IRC.
-------------------- 20110922 13:00:16< thonsew> Hello again. 20110922 13:00:16< thonsew> First, thank you for all of the bug reports. Keep them coming. Some of the bugs like the static de-initialization bugs (crashing on exit) I would never and will never be able to replicate on my machine. 20110922 13:00:16< thonsew> Second, I have poorly communicated my intent and progress (or lack thereof). I will try and rectify that below, in a question and answer style, focusing on the lua problems. The TA answers are more technical. 20110922 13:00:30< thonsew> Q1. What does t_token do and why is it a good thing? 20110922 13:00:30< thonsew> A1. When I profiled wesnoth I noticed that it was spending more than 25% of its time allocating, constructing and parsing the exact same strings. t_token is a way that it can allocate, parse and construct a given string once per game. When I profiled 1.9.9 versus trunk yesterday it is 23% faster running one turn of the orcs, which I think is a substantial speed improvement. 20110922 13:00:31< thonsew> TA. t_token uses an unordered_map to change copying, copy construction, equality comparison and hashing of strings and string literals of O(1) operations requiring no memory allocation. 20110922 13:00:48< thonsew> Q2. Why is there a problem with lua? 20110922 13:00:48< thonsew> A2. Lua and wesnoth don't consider t_token to be the same kind of string. Lua handles strings in exactly the same way as t_token, with a table for fast equality comparison and copying. So there are now 2 different hash tables for the same string. If lua is used primarily for user interface operations on a human time scale, then the slow down of constructing a string, passing a string and constructing a t_token/lua_string will be imper 20110922 13:00:48< thonsew> ceptible. However, if as I suspect lua code is being used to do loops at AI time scales, then that will wipeout all of the gains of t_token in lua intensive code. 20110922 13:01:16< thonsew> Q3. How are you trying to solve this problem and what is the problem with the solution? 20110922 13:01:16< thonsew> A3. If you create a third table to lookup lua_string <--> t_token then neither side of the transaction pays the price for crossing the interface. The lua bug problem right now is this fix is leaking into user based lua code. 20110922 13:01:16< thonsew> TA3. Lua indexes tables or metatables with hashed of lua_strings, userdata or lightuserdata with equal facility. So I have tried to solve the problem by created a t_token userdata proxy for t_token that lua can use as an index into its tables and also the wesnoth classes that are exposed through the lua interface. The problem with this solution is that lua does not all for the creation of a heterogeneous __eq operator in a metatable. 20110922 13:01:18< thonsew> Consequently, there is no way to create userdata that will act as a interchangeable component with string in userland lua. However, inside wesnoth lualand I am going to try to use this proxy to make the lua work correctly and quickly if profiling shows that the lua<-->wesnoth boundary is a source of slowdown 20110922 13:01:34< thonsew> Q4. What did you mean in the previous commit comment by the lua code hole? 20110922 13:01:34< thonsew> A4. The wesnoth lua interface is designed to allow for the execution of arbitrary userland lua code. I need to fix the wesnoth code so that references to the t_token object never leak into userland and so that all strings from userland are properly converted to t_tokens. 20110922 13:01:52< thonsew> Q5. Why is it taking so long? 20110922 13:01:52< thonsew> A5. There is no lua automated set of test to test the entire lua interface, so I need to run specific scenarios to uncover problems and then fix them. That is why it is good to keep submitting bugs in mainline campaigns until I've found and fixed them all. 20110922 13:01:52< thonsew> TA5a. There are no lua/WML unit tests. I do not know enough about the expected syntactic rules to write them. Without unit tests all I can do is find/fix lua bugs by running scenarios. 20110922 13:01:52< thonsew> TA5c. There are the wesnoth.zzz functions defined in game_events, the wml_actions operations defined in lua.cpp, the userdata C++ metatable operations also described in lua.cpp and the wml_actions and helper functions defined in lua code. Finding all the places where there is a type mismatch at the interface between lua and C++ without strict typing or type inference is a time consuming process running scenarios in the debugger. 20110922 13:02:08< thonsew> Q6. What went wrong? 20110922 13:02:09< thonsew> A6. I tested with HttT Scenario 1 and NR Showdown and the unit_tests, none of which have lua that threw errors in them. 20110922 13:02:44< thonsew> That is all. I hope that clarifies what I did, why I did it, what went wrong and how I intend to fix it. Thanks. -------------------- So, here's my opinion, mainly as a member of the large group of "advanced wml or lua authors or wml pioneers with own addons". -The token changes introduced a flood of bugs in the wml and lua interface, generally preventing me from doing what I wanted to do and forcing me to nail down a bug, report it and wait for the fix. I suspect that this will last for a long time still, considering that the bugs remaining are now very hard to reproduce or fix, such as bug #18769 and bug #18666 , and that the group of testers and test content has been very small until now. -I can reproduce a noticable slowdown with the wml reading phase on windows, and only on windows, I couldn't objectively messure a difference with the same hardware on Linux/scons build system. This still needs to be reproduced, but since it likely affects the official windows releases as well I consider it inacceptable. -Due to the token changes, code in many places got much less readable. At the start of functions there's now 10 lines of generate_static-... and then same amount of "real" code. -It seems to me that tokens were unneccessarily used at many places where this actually doesn't make sense. Mainly the lua interface and game_events.cpp (lua action and wml action engine). These functions are mostly called during human turns, not during "loops at AI time scales". So unless thonsew has evidence that these changes improve speed/whatever they should be reverted. That being said, I have reservations against thonsew's general coding style. It seems to me that he often changes code without a good reason for doing so and does afterwards not test these changes. (That is, unless both is done, changing some code should not be allowed.) These random small unrelated changes packed into large revisions lead to various bugs. Some examples are in r51250 and r51240 (token support in lua code and interface, I revealed one of them in IRC), which I both reverted, given that I understood best what he did there (compared to his other revisions). IMHO it's just "exaggerated" to let UMC authors/wml authors/lua authors see anything of tokens, thonsew didn't have evidence these changes made sense (see above, making changes without a previous reason) and lua code got much less readable and more complicated. When thonsew submitted his token changes first (r51053), he called it a "work in progress". IMHO calling something a "work in progress" is sort of an excuse for not getting stuff done right in the first place and the tendency to push one's own job to other devs (if it causes bugs or other problems). Here I can cite silene again that work should be "committed once it's finished". Thonsew also seems to have a tendency to not actually fix bugs but cover them. A proof for this claim is in r51056 where he changed the behavior of the [item] tag due to apparently changed behavior of the __parsed field (due to the config/token changes, not any changes in the lua interface). He covered a bug in HttT scenario 1 by doing so, which seems to be reproducible only sometimes (me and Elvish_Hunter could reproduce it sometimes and sometimes not). The problem should of course be fixed in the area of config.cpp or variable.cpp, not in the lua code, which would imply that UMC lua code needed fixing as well. So if he changes the behavior of accessing key values in vconfig objects, he should at least give detailed description why the previous behavior can no longer be maintained and announce it to all lua authors. I admit I don't understand most of the changes thonsew did. And the problem is, I don't trust thonsew that he himself does. My vote goes for undoing the whole thing, either by branching at the latest "stable" revision before tokens (r51052) and porting revisions which should be kept or by reverting token revisions and revisions which react to them (compiler errors+warnings, build system updates (file additions), bug fixes). Not sure what would be less problematic. "Fixing all these bugs instead of reverting the stuff is probably less work" should not be an argument IMHO. Then I could say "OK, so by not communicating at all previously and then making lots of not-so-good revisions I can change the engine in the way I want"! Note that there also appeared some bugs apparently caused by thonsew's older changes (such as the unit map) which we may have a hard time to fix. Cheers, Anonymissimus _______________________________________________ Wesnoth-dev mailing list [email protected] https://mail.gna.org/listinfo/wesnoth-dev
