Opened 7 years ago

Closed 6 years ago

Last modified 5 months ago

#1984 closed enhancement (fixed)

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

Reported by: Jonas Platte Owned by: JoshuaJB
Priority: Should Have Milestone: Alpha 16
Component: UI & Simulation Keywords: memory performance
Cc: Patch:

Description

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>.

Attachments (1)

gui_sprite_patch.diff (17.3 KB) - added by Jonas Platte 7 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 7 years ago by leper

Keywords: patch review added
Milestone: BacklogAlpha 15

comment:2 Changed 7 years ago by Jonas Platte

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

comment:3 Changed 7 years ago by Jonas Platte

Update: Changed it++ to ++it

comment:4 Changed 7 years ago by Jonas Platte

Update: Use NONCOPYABLE macro.

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

comment:5 Changed 7 years ago by 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).

Changed 7 years ago by Jonas Platte

Attachment: gui_sprite_patch.diff added

comment:6 Changed 7 years ago by Jonas Platte

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

comment:7 Changed 7 years ago by historic_bruno

Cc: Jorma Rebane added

comment:8 Changed 6 years ago by Jorma Rebane

Keywords: reviewed added; review removed

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.

comment:9 Changed 6 years ago by Jorma Rebane

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

comment:10 Changed 6 years ago by historic_bruno

Cc: Jorma Rebane removed
Keywords: reviewed removed

comment:11 Changed 6 years ago by historic_bruno

Keywords: review memory performance added
Milestone: Alpha 15Alpha 16

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.

comment:12 Changed 6 years ago by 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.

comment:13 Changed 6 years ago by Josh

Keywords: reviewed added; review removed

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.

comment:14 Changed 6 years ago by JoshuaJB

Owner: set to JoshuaJB
Resolution: fixed
Status: newclosed

In 14493:

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

comment:15 Changed 6 years ago by Josh

Component: Core engineUI & Simulation
Keywords: patch reviewed removed

comment:16 Changed 6 months ago by 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

comment:17 Changed 5 months ago by 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.

Note: See TracTickets for help on using tickets.