Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Defer rollover checks while generating selection decorations #19546

Merged
merged 1 commit into from Aug 4, 2021

Conversation

abcdefg30
Copy link
Member

@abcdefg30 abcdefg30 commented Jul 19, 2021

In a profiling run a few days ago the rollover check took around 3% of the total CPU time. I wasn't able to reproduce that number in subsequent runs however. (It evened out at ~0.5%.) The changes in this PR still more than halved the time spent in the method (~0.2%), but I wouldn't be opposed to just closing this if the code changes seem to "ugly" for that speedup.
Fwiw, it does make sense to me to not perform those checks for people who use AlwaysShow for selection bars. (And with the changes in the PR we'll also not perform them for some cases with DamageShow.)

@pchote
Copy link
Member

pchote commented Jul 19, 2021

This doesn't seem too ugly, IMO.

reaperrr
reaperrr previously approved these changes Jul 24, 2021
@pchote
Copy link
Member

pchote commented Jul 24, 2021

What seems to actually be going on here is that Selection.rolloverActors is being assigned with an enumerator result from a LINQ expression, which ends up being reevaluated every time something wants to check RolloverContains, when it could instead be using a concrete list.

Can you please check whether keeping SelectionDecorationsBase as it is and instead calling SetRollover(rollover.ToList()) gives an even larger perf improvement?

@pchote
Copy link
Member

pchote commented Jul 24, 2021

It may be even better still to pass the IEnumerable but then keep a list inside selection that we clear and repopulate with .AddRange(rollover) instead of reallocating a new list every tick.

@pchote
Copy link
Member

pchote commented Jul 28, 2021

Something like 61a2b84

@abcdefg30
Copy link
Member Author

The RolloverContains function dropped to ~0.0% now.

@penev92
Copy link
Member

penev92 commented Aug 4, 2021

Code looks sensible, the explanations paint a good picture and I'm going to trust your profiling.

@penev92 penev92 merged commit 453d59a into OpenRA:bleed Aug 4, 2021
@penev92
Copy link
Member

penev92 commented Aug 4, 2021

Changelog

@abcdefg30 abcdefg30 deleted the rollover branch August 5, 2021 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants