[PATCH] GUI code improvement

[kromxp]

--The problem--

The gui code (source/gui) uses many variables linked with js and the xml reader (I suppose), these variables are added using AddSetting(type, stringy_name). Later many of this variables are getted/setted using GUI<type>::SetSetting and GUI<type>::GetSetting

These getters and setters was searching on a std::map each time. Using a profiler (gprof) on my computer, these appears on the top, taking > 7% of the running time. Running the game for around 1 minute would generate ~20 millions calls to these functions, this is because many of this calls was done each loop when redrawing the screen.

--Suggested solution--

When creating the new types, the old code using a dynamically allocated variable. With the new changes it can use member variables of the calling class.

Then, because the values now are stored in member variables, almost all the parts that was using the old code to find the values now just get the value of the member variable which is an order of magnitude faster than searching a map.

In case the variables are "Setted", care has been taken to generate the set events so other parts of the code might receive it, just like it was done before.

After applying the changes, the numbers of calls to std::map::find reduced drastically (some thousands after couple of minutes), and those are mostly coming from js code (which should be the correct way in this case). Now takes less than 0.1% of the running time.

[kromxp]

gui patch

[Philip]

Thanks - I think this is a valuable problem to fix. I'm not yet convinced this is the best solution, though. It looks like it should work but my concern is that it seems to be breaking the current architectural abstractions for settings, which gives it the potential to be more error-prone and cause more pain in the future. (I don't think the GUI has a particularly great or clean architecture, so I'm not trying to defend the current design - I just want to make sure any major changes are improvements (or at least not making it any worse).)

In particular, it breaks the currently-almost-reliable guarantee that GUIM_SETTINGS_UPDATED will be sent when a setting changes, since the code can now update its m_... fields directly instead of through SetSetting. It's unlikely the code will update them directly, but it's possible, and it seems a potential source of bugs - I think it's better if these things are enforced by the design rather than just by convention and documentation.

It also provides two different codepaths to update the same setting (e.g. SetHidden vs SetSetting), which is at least confusing (you have to work out which to call when writing new code) and might get out of sync (if someone updates one path and not the other, e.g. changing how it triggers QueryResetting). Similarly there's three codepaths for reading settings (GetHidden etc, GetSetting, and m_... for ones in the current class) which is adding complexity and inconsistency to what should be a straightforward task of reading a value.

(Please let me know if I'm mistaken in my understanding of any of this :-) )

As a potential alternate solution that should avoid these problems and should hopefully give most of the performance benefit, I think it may be possible to simply replace the setting name strings in C++ with an enum integer, and replace the std::map<CStr, SGUISetting> with a std::map<int, SGUISetting>. Accessing a property via C++ (with GetSetting(this, GUIS_hidden, hidden) etc) would still involve a map lookup, but it wouldn't involve string comparisons or string memory allocations so it should be extremely cheap. When JS reads/writes a setting, the script interface can do a lookup to get the integer ID, or if the name has never been seen before then allocate a new integer, and then carry on as before. I think this should work with exactly the same AddSetting/GetSetting/SetSetting API as now (except with strings replaced by ints) so it wouldn't be adding any complexity. Maybe I'm missing some problems with this, though?

[historic_bruno]

The above points need to be addressed and this can be considered reviewed for now :)