Page MenuHomeWildfire Games

MOVABLE idiom, const CGUI databases and avoid unintentional SGUIIcon, SGUIStyle copies
ClosedPublic

Authored by elexis on Aug 9 2019, 2:09 PM.

Details

Summary

This patch introduces the MOVABLE idiom indicating a class or struct supporting move semantics, analogous to the COPYABLE idiom, and uses it for the CGUI struct maps that store immutable XML data.

The design of the GUI is to load all data from the XML file and then only refer to that later on.
However during the course of its implementation, in a number of incidences this data was copied around for no reason, most crucially every frame in Draw or GenerateText calls.

For example rP22570 made CGUISpriteInstance NONCOPYABLE and MOVABLE following rP1518 introducing linker errors to avoid unintentonal copies in one specific function to CGUISpriteInstance after rP1507 unintentionally copied the entire CGUISpriteInstance DrawCall cache.

Another example is rP14493 removing the CGUISprite and SGUIImage copies by using pointers and making them NONCOPYABLE in advance.

This patch makes SGUIIcon and SGUIStyle NONCOPYABLE and MOVABLE as well and replace their copies by const references that were performed every CGUI::GenerateText and CGUIString::GenerateTextCall.

Use in-place move construction using emplace for the temporary objects created by the CGUI Xeromyces functions for these struct maps.

Finally as the cherry on top, make the struct values of these four CGUI "database" maps constant, because
(a) it is conceptually valid, because the data is considered permanent by the CGUI design since it only load and possibly replaces these structs from XML and maybe JS and only refers to that data later on (during draw calls).
(b) hence copies are unintentional copies and thus making the compiler complain protects against unintentional copies.
(c) unintentional copies are very easy to introduce, because the topic is complex (there are many rules to copy vs. move assignment, assignment vs constructor, guaranteed copy elision, permitted but not guaranteed optimizations, how containers perform), and commit history has shown that people do copy unintentionally.

The downside to making the map values const is that one has to erase if one wants to keep supporting overwriting of XML styles.
This is a bummer, but still seems much preferable to waste some nanoseconds when loading a page rather than wasting some nanoseconds every frame.

Making these const means that we gain assurance that the future of source/gui/ rendering will use no copies and that this is enforced by the compiler, whereas keeping it mutable means that we have to worry in every commit accessing these items.
Aside, erasing and inplace move constructing is still a performance improvement to the previous copy assignment on init.

Test Plan
  1. Have seen people copy too many things to grant this to endure any longer. Convince yourself that it is better to error on the side of caution than to keep "vulnerable" to unintentional copies on possibly every frame and that there is a benefit in enforcing this as a policy until the GUI was redesigned.
  2. Become educated about the benefits and use move semantics and emplace, copy elision. Become educated about the disadvantages and pitfalls of move semantics.
  3. Do some benchmarks
  4. Test code correctness.
  5. Notice that this will be one of multiple commits, since there are a number of related shenanigans going on in source/gui/.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Aug 9 2019, 2:09 PM
Vulcan added a comment.Aug 9 2019, 2:42 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/gui/CGUI.h
|  27| #include·"GUIbase.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classGUITooltip{' is invalid C code. Use --std or --language to configure the language.

source/gui/CGUISprite.h
| 173| »   CStr·m_SpriteName;
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCClientArea{' is invalid C code. Use --std or --language to configure the language.

source/lib/code_annotation.h
| 337| template<typename·T,·size_t·n>·char·(*ArraySizeDeducer(T·(&)[n]))[n];
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'template<...' is invalid C code. Use --std or --language to configure the language.

source/gui/GUIRenderer.h
|  33| namespace·GUIRenderer
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceGUIRenderer{' is invalid C code. Use --std or --language to configure the language.

source/gui/GUIbase.h
| 173| class·CClientArea
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCClientArea{' is invalid C code. Use --std or --language to configure the language.

source/gui/IGUIScrollBar.h
| 173| ·····*·If·an·object·that·contains·a·scrollbar·has·got·messages,·send
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCClientArea{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/343/display/redirect

elexis added a comment.EditedAug 9 2019, 5:25 PM

Have seen people copy too many things to grant this to endure any longer. Convince yourself that it is better to error on the side of caution than to keep "vulnerable" to unintentional copies on possibly every frame and that there is a benefit in enforcing this as a policy until the GUI was redesigned.

NONCOPYABLE CGUISpriteInstances DrawCall Cache: rP22570, rP1518, rP1507
NONCOPYABLE Image, Sprite data rP14493
NONCOPYABLE GUI page rP13419
NONCOPYABLE GUIManager rP7259

Become educated about the benefits and use move semantics and emplace, copy elision. Become educated about the disadvantages and pitfalls of move semantics.
Do some benchmarks

Sadly I lost all my notes when my OS froze when testing on windows.
But those were some of the references I used to become sure what happens:

https://en.cppreference.com/w/cpp/utility/move
https://en.cppreference.com/w/cpp/language/move_constructor
https://en.cppreference.com/w/cpp/language/move_assignment
https://en.cppreference.com/w/cpp/language/copy_assignment
https://en.cppreference.com/w/cpp/language/copy_constructor
https://en.cppreference.com/w/cpp/language/copy_elision
https://en.cppreference.com/w/cpp/language/value_category
https://en.cppreference.com/w/cpp/container/map/emplace
https://en.cppreference.com/w/cpp/container/map/operator_at

https://stackoverflow.com/questions/26860749/efficiency-of-c11-push-back-with-stdmove-versus-emplace-back-for-already
https://stackoverflow.com/questions/9987298/move-semantics-and-primitive-types
https://developers.redhat.com/blog/2019/04/12/understanding-when-not-to-stdmove-in-c/

source/gui/CGUI.cpp
1603 ↗(On Diff #9277)

I wrote some benchmarks for this change.

Avoiding the previous copy assignment already made it like twice as fast (mostly because the std::vector can be exchanged instead of copied).

The move assignment would be something like 20% faster than erase + emplace move.

Performing a count() operation before erase() makes it like 5% faster.

However we're speaking about nanoseconds each time a GUI page is loaded, so it's not so relevant.

The code however is added because the advantage of having things const that are immutable is that the compiler enforces this policy.

So even if the erase call is making this a bit slower than a move assignment, it's (A) still faster than it was before and (B) we ensured at the ground level that the data becomes immutable, and thus don't have to rely on const ref Getters anymore and privacy shields.

elexis added a comment.Aug 9 2019, 7:26 PM

The patch is a furtherance of the NONCOPYABLE MOVABLE use of rP22570 which is a progression of rP1518.

On communication:

The MOVABLE hunk was shown to Itms briefly on http://irclogs.wildfiregames.com/2019-08/2019-08-05-QuakeNet-%230ad-dev.log
CGUI const map was discussed with wraitii on http://irclogs.wildfiregames.com/2019-08/2019-08-09-QuakeNet-%230ad-dev.log
(Random IRC monologs don't count)

source/lib/code_annotation.h
219 ↗(On Diff #9277)

This was a class in rP5679. macro in rP6536, class removed in rP7498.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 9 2019, 7:26 PM
This revision was automatically updated to reflect the committed changes.