Page MenuHomeWildfire Games

Fix chat performance issue.
Needs ReviewPublic

Authored by nani on Feb 23 2019, 4:18 AM.

Details

Reviewers
vladislavbelov
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Owners Package(Owns No Changed Paths)
Summary

New chat text is added as a copy of all previous chat and new one, use CList so it is not. Also adds a handy JS Interface addItem method to add new comments efficiently.
With this fix you can add a new message every onTick for 5000 ticks and not notice any performance hit (not like the current one that at 500 lines starts to drop fps).

Test Plan

Check text displays correctly when added.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

nani created this revision.Feb 23 2019, 4:18 AM
nani edited the summary of this revision. (Show Details)
Stan added reviewers: vladislavbelov, Restricted Owners Package, Restricted Owners Package.Sat, Feb 23, 11:55 AM
elexis removed a reviewer: elexis.Sat, Feb 23, 4:04 PM

So should optimize both performance and code complexity.
What is the performance critical part where FPS start to be dropped?
Is it the JS string building from the message history array or the passing of a long string?
I.e. is Engine.GetGuiObjectByName("chat").text += newline similarly quick as the proposed diff?

Notice the session chat is not affected by this patch and it has to be rebuilt in case someone selects a different playerview or triggers the diplomacy colors feature (or muted/unmuted someone, or on other events).
I guess the lobby dialog UI performance is relevant FPS-wise because the lobby can be opened ingame.

Shouldn't selecting multiple lines of text become possible eventually? Would CList allow for that?

binaries/data/mods/public/gui/lobby/lobby_panels.xml
259

follow_last_if_visible -> there was some common name for it in chat apps, autoscroll or something

nani marked an inline comment as done.EditedSat, Feb 23, 4:57 PM
nani added a subscriber: elexis.
In D1781#72304, @elexis wrote:

So should optimize both performance and code complexity.

A little hard to add features without adding complexity. (can be refactored later tho but that's not the scope of this patch)

What is the performance critical part where FPS start to be dropped?

Having the lobby open for 1h (lots of comments on chat)

Is it the JS string building from the message history array or the passing of a long string?

Creates a big string from array.join("/n)

I.e. is Engine.GetGuiObjectByName("chat").text += newline similarly quick as the proposed diff?

First: += doesn't work as there is a JS <-> C++ conversion going on.
Second: even if it worked the C++ implementation would anyways delete all the list and make a new one.

Notice the session chat is not affected by this patch and it has to be rebuilt in case someone selects a different playerview or triggers the diplomacy colors feature (or muted/unmuted someone, or on other events).

Yep. Nothing can be done there (unless if <objects type="list"> is used for each filter but that sounds ugly.

I guess the lobby dialog UI performance is relevant FPS-wise because the lobby can be opened in-game.

Is not about the opening (first load) is about the accumulated text being added as if it were a whole new text each time.

Shouldn't selecting multiple lines of text become possible eventually? Would CList allow for that?

For the moment selected setting tag is a int type so no (an array of ints or a set could).

binaries/data/mods/public/gui/lobby/lobby_panels.xml
259

auto_scroll is a different concept as it follows the selected one (e.g. when you move with key_up key_down)
follow_last_if_visible always "follows" the last item (the bottom-most one) if visible. Doesn't really follow the last item but gets current scroll position and if it is 20px near of the max scroll position then it sets it to to the max position (as a message message could be very long and make knowing the text size useless).

elexis added a comment.EditedSat, Feb 23, 6:00 PM

The question is why this should become a CList to begin with if one doesn't need to change C++ code in order to remove the .join call. text .= newline should work, even if it will go through the JS interface once to read and once to write.

Edit: Could even add a .pushText method to the CText or CInput or whatever this should become. Would need a bit of coding, but I don't see the reason for making this a CList.

elexis added inline comments.Sat, Feb 23, 6:11 PM
source/gui/scripting/JSInterface_IGUIObject.cpp
81

The mess with adding new entries here is that these properties are available to every GUIObject type, not only the one type that it was implemented with so far. So there ought to be GUIObject-type specific JSInterfaces (while possibly keeping the properties that share the same code in a shared JSInterface file)

So if one adds such a property here, then only knowingly that it's increasing the mess.

(The alternative was adding a "Setting" in the CFoo.cpp, but that is even worse because functions aren't settings and we don't want a setter for that)

nani marked 2 inline comments as done.Sat, Feb 23, 6:22 PM
nani added inline comments.
source/gui/scripting/JSInterface_IGUIObject.cpp
81

I know. When I started I thought of different strategies but for the meanwhile this is the cleanest of all without doing a massive changes on how GUI code works. :/

nani marked an inline comment as done.Sat, Feb 23, 6:24 PM
nani added inline comments.
source/gui/scripting/JSInterface_IGUIObject.cpp
81

Also having random macro definitions spread all over the place doesn't help knowing what can be changed.

nani added a comment.Sat, Feb 23, 7:29 PM
In D1781#72322, @elexis wrote:

The question is why this should become a CList to begin with

CList could store metadata of each text in data_list settings (which CText could not)
and well, usually all chats (just look any webpage code) are designed as a list of text (<divs>) and a chat message is like a unit of independent text so the reasoning would be why have it as CTextOwner and not CList.

nani updated this revision to Diff 7498.Sat, Feb 23, 10:01 PM
nani edited the test plan for this revision. (Show Details)

Remove setting follow_last_if_visible as is orthogonal to the patch (will add in another patch if this goes through).

So the title "Fix chat performance issue" is actually not the purpose of this patch, but to allow another patch to add the possibility of metadata?

If the slow part is the JS .join call concatenating and sprintf-formatting + 500 strings, we should remove that first and foremost (simply using .caption += new_line) before considering any C++ change.
It could be that this is already sufficient enough performance to provide sufficient performance.

Then if one wants to consider any C++ change, it's also not inherently clear whether creating a new CList item is actually faster than appending a string in the CText (or CInput for selectable text).
That one won't be able to select multiple lines with a CList seems like, undesirable.
The only reason why one can't use a CInput for chat currently is that the GUI tags (color, font) won't be applied to the text. (We should have selectable text for the terms and conditions).

nani added a comment.EditedTue, Feb 26, 9:33 PM
In D1781#72462, @elexis wrote:

If the slow part is the JS .join call concatenating and sprintf-formatting + 500 strings, we should remove that first and foremost (simply using .caption += new_line) before considering any C++ change.
It could be that this is already sufficient enough performance to provide sufficient performance.

I must say i haven't tried that (will try) but I'm 99% sure the problem is C++ given it needlessly deletes all computations as text parsing text sprite creation (expensive) and makes new ones for something which was already done.

ffffffff added a subscriber: ffffffff.EditedSun, Mar 3, 3:34 AM

i feel fear in every elexis sentence

very dangerous

i dont see where this should get more complex.

complex != bigger

its just a new function feature

dont get urself insane about that

its just new nice stuff

xD

easy

Peace

elexis added a comment.Sun, Mar 3, 3:50 PM
binaries/data/mods/public/gui/lobby/lobby.js
1408

Engine.GetMicroseconds() can be used to measure

nani added a comment.Sun, Mar 3, 8:06 PM

@elexis chat from the lobby for 2.5 h at sunday.

Code measured:

        // the chat join("/n") text code is not takken into account
	let time1 = Engine.GetMicroseconds();
	Engine.GetGUIObjectByName("chatText").caption = textSmashup;
	let time2 = Engine.GetMicroseconds();

versus

	let time1 = Engine.GetMicroseconds();
	Engine.GetGUIObjectByName("chatText").addItem(formatted);
	let time2 = Engine.GetMicroseconds();

For reference at 60 fps, 1 frame takes 16666 microseconds

eae what a nice chart

elexis eae

elexis added a comment.Mon, Mar 4, 4:00 PM

// the chat join("/n") text code is not takken into account
let time1 = Engine.GetMicroseconds();
Engine.GetGUIObjectByName("chatText").caption = textSmashup;
let time2 = Engine.GetMicroseconds();

That means there is a textSmashup = chatHistory.join("\n") prior to that code, i.e. that the only thing you measure is the caption assignment?

So it takes 5 milliseconds for .caption = text if text is 500 lines long and 5ms equate to 1/200th of a second.
That is JSI_IGUIObject::setProperty which calls ScriptInterface::FromJSVal and GUI<CStrW>::SetSetting, the latter calls CText::SetupText(), which calls GetGUI()->GenerateText and computes the scrollbar properties.

So it's the GenerateText call on a 500line string that consumes 5ms?
But reading CList::AddItem, that also calls CList::SetupText which calls GenerateText for every item, but your diff replaces that call to a call to SetupTextLastItem removing all GenerateText calls except the one for the last item.
If there were numbers to back that up that would demonstrate that claim.
For instance one could call GenerateText to a 500 line string and see how much time that costs.
I guess it's the parsing of the GUI tags that might take so long (linear complexity).
So if the analysis is correct, the SetupTextLastItem part of the diff might be correct to fix that removed TODO in CList.cpp (should just verify that one really doesnt need to rebuild texts or whatever else is rebuilt upon AddItem).

Now the remaining quesiton would be whether the same performance improvement could be achieved for CText and CInput.
Perhaps they could store a vector of GeneratedTexts to implement an AddCaption with time complexity that has less than linear relation to the total caption length;
and then some day enable GUITag parsing for read-only CInputs, and thus make the chat both selectable and high-performant / O(1).

source/gui/CList.cpp
213

Sounds like copypasta, at least the TODO from 2004 is, helper function preferable if possible

nani added a comment.Mon, Mar 4, 9:39 PM

That means there is a textSmashup = chatHistory.join("\n") prior to that code, i.e. that the only thing you measure is the caption assignment?

Yes. I'm 100% sure c++ code is the problem.

So it takes 5 milliseconds for .caption = text if text is 500 lines long and 5ms equate to 1/200th of a second.
That is JSI_IGUIObject::setProperty which calls ScriptInterface::FromJSVal and GUI<CStrW>::SetSetting, the latter calls CText::SetupText(), which calls GetGUI()->GenerateText and computes the scrollbar properties.

So it's the GenerateText call on a 500line string that consumes 5ms?
But reading CList::AddItem, that also calls CList::SetupText which calls GenerateText for every item, but your diff replaces that call to a call to SetupTextLastItem removing all GenerateText calls except the one for the last item.
If there were numbers to back that up that would demonstrate that claim.

I did a chart in the previous demonstrating that is indeed GenerateText (what else could be? is clear as water that that for loop in GenerateText is unnecessary to add only another item-.-)

For instance one could call GenerateText to a 500 line string and see how much time that costs.

Already done in the chart.

I guess it's the parsing of the GUI tags that might take so long (linear complexity).

Parsing + text sprite generation

So if the analysis is correct, the SetupTextLastItem part of the diff might be correct to fix that removed TODO in CList.cpp (should just verify that one really doesnt need to rebuild texts or whatever else is rebuilt upon AddItem).

Now the remaining quesiton would be whether the same performance improvement could be achieved for CText and CInput.

Yes it can be done.

Perhaps they could store a vector of GeneratedTexts to implement an AddCaption with time complexity that has less than linear relation to the total caption length;
and then some day enable GUITag parsing for read-only CInputs, and thus make the chat both selectable and high-performant / O(1).

They already do have a vector, just needs someone to implement the JSinterface + methods for each case. :)

elexis added a comment.Tue, Mar 5, 4:05 PM

Yes. I'm 100% sure c++ code is the problem.

That 'Yes' was the sought answer. (Being sure oneself is good, but it doesn't demonstrate the statement is the case.)

I did a chart in the previous demonstrating that is indeed GenerateText

The chart measures "caption" property change time, not GenerateText performance.

what else could be?

Given that GenerateText is only one of many statements called, it's not trivial that this statement is the bottleneck.

is clear as water that that for loop in GenerateText is unnecessary to add only another item-.-

So it's as clear as the source code at best, and the source code itself only tells us the complexity (but different statements can also cost different time to perform.)
While didding through it I find IGUITextOwner::AddText which sounds inviting for according changes to other GUI object types.
The performance defect in question was the CText.cpp one, not CList (so "to add only another line" that is.)

(Notice that addItem would be problematic for COList since an item consists of multiple cells there. I had a patch that changes the assignment, then one could make it an addItem that receives a JS::Value, a JSObject for COList and a String for CList.)
(As mentioned, it would be cleaner IMO to name it addText for CText and use specific functions per GUI object type.)

They already do have a vector, just needs someone to implement the JSinterface + methods for each case. :)

Seems like a vector with exactly one item for CText, so it would also need to revisit CText.cpp.

Yes it can be done.

If it can be done, the question is should it be done? i.e. why should this become a CList when it isn't one? I guess it's because the use case analysis was capped to what is currently implemented.
As mentioned some polished version of the CList diff seems appropriate given that it relates to the TODO.

source/gui/scripting/JSInterface_IGUIObject.cpp
728

(IIRC JS_THIS_OBJECT is replaced in some future SM version)

nani added a comment.Wed, Mar 6, 12:26 AM
In D1781#72577, @elexis wrote:

As mentioned some polished version of the CList diff seems appropriate given that it relates to the TODO.

So can we have middle ground? The new method can be polished and use it at face value for the meanwhile so the chat doesn't hog the lobby.
All chat features will still be the same (needs that new extra tag to follow text bottom given that CList doesn't have it).
Nothing will change for the end user except 0ad being a little bit more performant. Not bad deal IMHO.

elexis added a comment.Wed, Mar 6, 6:57 PM

I didn't investigate the code specifically, but in principle sure. The argument would be that this thing is written now and writing the better thing is too boring.
The problem is that selectable text isn't unlikely to happen in the next years, and then someone still has to write the patch the way it should be and make the current code unused.
And the same method would still be missing from the other GUI object types - ideally every GUI type would have an according append function.
Should comment on the affected GUI objects that this uses the list type for the complexity, since otherwie the next best reader may think it's a mistake and revert it.

Also if one identified such a unique problem in one case like you did here, one should abstract and see if one can spot the same problem in other parts of the code.
I.e. do we have other GUI objects that replace text repeatedly and possibly contain so many GUI tags that it can affect the performance?

For instance the gamesetup refreshes all UI objects, i.e. many GUI objects with maybe one GUI tag. And the gamesetup also contains the gamedescriptions.js thing listing all selected matchsettings, i.e. many GUI tags.
The replay menu uses a table (COList.cpp), contains many tags too.
So it would be good to check a bit whether one can add similar fixes for GUI object types in general.
A low hanging fruit for example would be not regenerating the text if the caption is still the same as before. Might cut 90% of the time if that isn't already the case.

nani added a comment.Wed, Mar 6, 8:06 PM

The thing is I did look into to changing CText CTextOwnder and similar files but the current inheritance model that is made with is quite difficult to expand so I went for the most straight forward/short solution. Maybe composition over inheritance would give more leverage but that means a rewrite of the GUI.

elexis added a comment.Wed, Mar 6, 8:15 PM

Isn't it possible to edit CText.cpp and CInput.cpp like you did with CList.cpp here, without messing with the text owner class at all? An addText or appendLine function in these GUI objects that just adds to m_GeneratedTexts and the caption string without rebuilding the existing code.

nani added a comment.Wed, Mar 6, 8:26 PM

Technically yes but I'm not sure it won't break something else (like the charts).

elexis added a comment.Wed, Mar 6, 8:48 PM

CChart doesn't inherit anything from CText or CInput, does it? And if we can't tell for CText or CInput whether something might break somewhere, how can we tell for CList?

nani added a comment.Wed, Mar 6, 8:59 PM

Not sure. But does look deceptively easy.

nani added a comment.Wed, Mar 6, 9:01 PM

Still, it doesn't solve the problem of bloating the base object if we want to make a JS interface for each gui type.

elexis added a comment.Wed, Mar 6, 9:37 PM

It'd probably be the cleanest to have one small JSInterface_GUIObjectType class that contains the functions and properties specific to that GUI type, while inheriting or calling a JSInterface_GUIObject with the functions and properties common to all GUI Object types (as mentioned in D1781#72323).
If it's done before, that would leave clean insertions of the proposed new JSInterface GUIObject function/s, but I suppose it can also be done afterwards (never) if it's not a requirement for the CList diff to straighten that. (That commit would also be accompanied by other commits IIRC.)

Still, it doesn't solve the problem of bloating the base object if we want to make a JS interface for each gui type.

Otherwise not sure what base object actually you're refering to.

Also shouldn't forget to check

not regenerating the text if the caption is still the same as before. Might cut 90% of the time if that isn't already the case.

I guess that's what you refered to when not being sure about the consequences of changing the IGUITextOwner? (Since that's the place where it checks for caption property changes).
It looks like it currently regenerates the caption even if it didn't change, and would probably cut a lot of performance in some of the mentioned cases. (And another Gee TODO from 2004 running gag there.)
The consequences of not regenerating the texts might be foreseeable for all child classes and one might conclude whether it's good or not to do so. I expect that all GUI Object types that inherit the TextOwner are expected to keep the caption property in sync with the generated texts, thus it would be harmless and useful with the current code, and a code where CInput and CText receive an AddLine function that extend the caption property and add an item to the m_GeneratedTexts vector.

Stan added a subscriber: Stan.Thu, Mar 7, 11:42 AM
Stan added inline comments.
source/gui/CList.cpp
163

Still accurate ?

440

Any way to make a helper function with the one below ?

481

Might want to add doxygen here as well.

source/gui/CList.h
46

C++ uses Doxygen, not JSDoc, so I'm not sure this works.

https://stackoverflow.com/questions/11991616/use-of-see-or-link-in-doxygen

In case it doesn't, you should add @param

source/gui/IGUIObject.h
133

@paramsmissing :)

135

Typo in warning.

source/gui/scripting/JSInterface_IGUIObject.cpp
756

Add one ;) If you are using vscode, you can configure it to do it for you :)