Opened 11 years ago
Closed 11 years ago
#2007 closed enhancement (wontfix)
[PATCH] GuiManager TickObjects performance fix
Reported by: | Jorma Rebane | Owned by: | Jorma Rebane |
---|---|---|---|
Priority: | Must Have | Milestone: | |
Component: | Core engine | Keywords: | patch performance memory |
Cc: | Patch: |
Description
As reported in the Performance ticket: #1995
GuiManager::TickObjects made a full copy of PageStackType m_PageStack in every tick:
PageStackType pageStack = m_PageStack;
PageStackType itself is an std::vector<SGUIPage> (yes, a by-value vector):
typedef std::vector<SGUIPage> PageStackType;
SGUIPage is defined as:
struct SGUIPage { CStrW name; boost::unordered_set<VfsPath> inputs; JSContext* cx; CScriptValRooted initData; shared_ptr<CGUI> gui; };
Due to these non-trivial types like unordered_set and CStrW, making a full copy of the pagestack vector resulted in some pretty serious memory allocation overhead. The data itself is in the #1995 performance ticket.
This patch provides a simple fix by using a const reference instead
Attachments (1)
Change History (10)
comment:1 by , 11 years ago
Keywords: | patch review added; performance memory gui removed |
---|
comment:2 by , 11 years ago
Milestone: | Backlog → Alpha 14 |
---|---|
Priority: | Should Have → Must Have |
comment:3 by , 11 years ago
Keywords: | performance memory added |
---|
comment:4 by , 11 years ago
Keywords: | review removed |
---|
by , 11 years ago
Attachment: | CGuiManager.patch added |
---|
Change the tickobjects loop to use indices instead of iterators.
comment:5 by , 11 years ago
Keywords: | review added |
---|
follow-up: 7 comment:6 by , 11 years ago
The solution depends on the intended behavior of changing pages: should it wait until after the tick handlers are called for all pages before the stack is modified (as the current implementation does) or take affect immediately in the middle of a handler, as it would with the patch? Before r7649 it was broken either way, but I don't know whether the fix is deliberately or accidentally making the logic choice. I guess it will take testing and deeper investigation into it, which is annoying :(
comment:7 by , 11 years ago
Replying to historic_bruno: The 'immediate' switch is intended behaviour. No reason to process gui pages that have been actually thrown away. In some aspect this feels like a more correct solution.
comment:8 by , 11 years ago
Milestone: | Alpha 14 → Alpha 15 |
---|
comment:9 by , 11 years ago
Keywords: | review removed |
---|---|
Milestone: | Alpha 15 |
Resolution: | → wontfix |
Status: | new → closed |
Closing these tickets as no further active development is expected. See Philip's megapatch-split git branch for an attempt at splitting megapatch into it's separate parts: http://git.wildfiregames.com/gitweb/?p=0ad.git;a=shortlog;h=refs/heads/projects/philip/megapatch-split
Needs a solution that allows tick handlers to modify the page stack (was introduced in r7649)