[PATCH READY] Save GUI Sprites and Images in vectors / maps as pointers instead of objects

[Jonas Platte]

When inserting into lists or vectors, the data is often reallocated and moved. The m_Sprites map in CGUI and the m_Images vector in CGUISprite save objects instead of pointers so there are many unnecessary construction and destruction operations, specially during startup.

This patch makes CGUI use std::map<CStr, CGUISprite*> instead of std::map<CStr, CGUISprite> and CGUISprite use std::vector<SGUIImage*> instead of std::vector<SGUIImage>.

[Jonas Platte]

Update: Made SGUIImage and CGUISprite non-copyable because they now own the data their members point to (delete it in the constructor)

[Jonas Platte]

Update: Changed it++ to ++it

[Jonas Platte]

Update: Use NONCOPYABLE macro.

I hope this is the last Update for today. Sorry for all the mail updates I caused.

[sanderd17]

You do want to clean that diff before you send it here. The config part and the lua script part shouldn't be in it. (it can be manually deleted from the diff text before uploading).

[Jonas Platte]

Sorry I forgot to only make a diff of source that time. And is there a way to delete that double attachment?

[Jorma Rebane]

Hey, finally found some time to review your patch.

At first glance, there doesn't appear to be anything wrong with the patch and in my opinion it's the right way to go.

I'll mark your patch as "reviewed" and it will be committed as soon as A14 has been released and the feature lock is lifted.

[historic_bruno]

Besides checking for correctness/conventions, this patch will need to be profiled before and after applying to confirm a measurable difference in performance and/or allocations.

[Jonas Platte]

I already benchmarked this, though I did not document it. This is what I can recall:

On startup, there were about 650 objects which were created through passing other objects by value before the patch. With the patch, there is no more copying.

Although that sounded much to me at first, it seems those objects hold not much data (some of it was pointer data, so not everything was copied) and the startup timer that is shown on the command line by default indicated a very low to no impact on the startup time.

After startup, there are much less Sprites and Images created, so the patch will probably not make the game any faster – at least that was the case for the environment I tested it in, I do not know what has changed since then.

Overall this will definitely not slow down the game and it is just better style, but do not expect it to get the game starting or running noticeably faster.

[Josh]

I reviewed the patch and I really don't see a problem, it's better style and is the direction the UI code should be moving towards. I'll commit this when I have some time if there are no objections.

[JoshuaJB]

In 14493:

Use pointers instead of copying sprites in the GUI. Fixes #1984. Patch by jP_wanN.

[elexis]

In 22637:

MOVABLE idiom, const CGUI struct maps, in place move construction instead of copying temporaries during CGUI XML loading and GenerateText.

Introduce MOVABLE idiom indicating that a class can use move semantics.
Make values of CGUI struct maps holding XML data const to ensure at the root that the data is not modified.
Use NONCOPYABLE and MOVABLE for SGUIIcon and SGUIStyle to enforce the non-copy policy on the compiler level (until someone changes the GUI design to make modifications needed).

As indicated by that:

Replace copy operations by in place move operations for these CGUI struct maps in the CGUI Xeromyces XML loading functions.
Replace copy operations by const references for CSize and SGUIIcon in CGUIString::GenerateTextCall and CGUI::GenerateText.
This avoids copying of non primitive members, such as the strings and containers of strings.

Further related cleanup to be kept separated for auditability.

Differential Revision: ​https://code.wildfiregames.com/D2163
Few comments on IRC by: wraitii, Itms
Tested on: gcc 9, Jenkins, partially VS2015

Refs #1984,

NONCOPYABLE CGUISpriteInstances: rP22570, rP1518, rP1507
NONCOPYABLE Image, Sprite: rP14493
NONCOPYABLE GUI page: rP13419
NONCOPYABLE GUIManager: rP7259
NONCOPYABLE macro: rP6536

[elexis]

In 22694:

Stop copying color every draw call for every GUI object using colors.

Avoid color copies for rendering Draw calls in GUIRenderer::UpdateDrawCallCache, CButton::Draw, CChart::DrawAxes, CDropDown::Draw, CList::DrawList, COList::DrawList, refs #1984, rP1518, rP22637, rP22638.
Avoid color copies during XML loading in CGUI::Xeromyces_ReadImage, CGUI::Xeromyces_ReadEffects, COList::HandleAdditionalChildren.
Add CGUI::HasPreDefinedColor and mark m_PreDefinedColors, CGUI::GetPreDefinedColor, IGUIButtonBehavior::ChooseColor() as const for consistency with the other "databases", refs rP22637.
Mark CGUIColor as NONCOPYABLE to add compiler errors if there is an unexplicit copy, refs rP22637.
Explicit ugly copy in CGUI::Xeromyces_ReadColor and CGUIColor::ParseString.
Deregister copying <CGUIColor>GetSetting functions, refs rP1518.
Uses the const ref GetSetting from rP22693.