perf: halve what an open center HTML menu costs per tick - #1401
Conversation
| public CenterHtmlMenuInstance(BasePlugin plugin, CCSPlayerController player, IMenu menu) : base(player, menu) | ||
| { | ||
| _plugin = plugin; | ||
| _previousLabel = Application.Localizer["menu.button.previous"]; |
There was a problem hiding this comment.
I think caching these will cause some buggy behaviour if a user changes language when they have a menu open. I don't think these are a source of a significant performance uplift, I would think most of the gains come from the event re-use and not creating a new string builder on tick. Could you remove the label caching and re-test the performance?
… one per tick Display runs on every tick for as long as the menu is open, and each call built a fresh StringBuilder. Keep one per menu instance and clear it instead.
The panel has to be resent on every tick to stay on screen, and each send built a game event and freed it again. Keep the event for as long as the menu is open and refill it instead, freeing it when the menu closes.
SteamID rejects a zero id, and a bot passes the validity check with one, so player.GetLanguage() threw ArgumentOutOfRangeException for any fake client. Fall back to the server language for them, matching how AdminPermissions already treats bots and HLTV.
…er tick Display runs on every tick and each label goes through the localization stack. Read them under the player's own language and keep them until it changes. Display runs outside of command handling, where the thread culture is the server language rather than the player's, so this also gets the labels into the language the player picked.
|
Removed the label caching and re-measured. You were right that the event reuse carries a real part of this, and right about the risk in caching the labels, though checking that second point turned up something I did not expect. Measured on a live server with the paths interleaved, alternating on every render so all of them see identical conditions. Nine option menu, averaged over five runs:
So the event and builder reuse is a third of it on its own, and the labels are the rest. On the caching risk, I set a player to The labels have never been in the player's language, so freezing them at construction would have been a step forward rather than a regression. There is a better option though, so I did not do that either. They are now read under the player's own language and kept until it changes, which is a reference comparison per tick. Same capture on this branch, switching language with the menu open: Right language, updates without reopening, still 58% faster than master. One thing fell out of this. The menu is now the first thing to call |
68ad9fa to
37b585b
Compare
2 participants
CenterHtmlMenuInstance.Displayis registered as anOnTicklistener, and the panel only stays on screen while it is being resent, so an open menu rebuilds and resends its markup 64 times a second for every player looking at it. Two things in that path do not need to happen more than once.Measurements
Taken on a live CS2 server with a 12 item menu, timing the two halves of
Displayseparately.With twenty players holding a menu open that is 0.66ms of every tick instead of 1.34ms.
What changed
Labels and the builder — the previous, next and close labels each go through the localization stack, and a
StringBuilderis allocated, on every one of those 64 renders a second. None of it changes while the menu is open, so the labels are read when the menu is opened and one builder is reused.The panel event —
PrintToCenterHtmlcreates a game event, fires it and frees it. Doing that every tick is what most of an open menu costs. The event is kept for as long as the menu is and refilled with the new markup instead, then freed inClose. Sending is guarded the same wayPrintToCenterHtmlguards it.The second measurement was taken by alternating the two send paths on every other render, so both meet the same conditions: 36.8us over 904 sends against 29.0us over 903. I first measured them one after the other and got a similar looking gain, but the untouched build time moved just as much between those two runs, so that pair said nothing and the interleaved one is what the number above comes from.
Things I tried that did not work
Worth writing down so nobody repeats them.
Resending less often than every tick. The panel drops off screen almost immediately, at any interval above 1. Every tick really is required, so the resend itself cannot be avoided — only made cheaper, which is what the second commit does.
Changing the duration passed to the event. No observable difference at 1, 2, 5, 10 or 30, so whatever that field does it is not keeping the panel alive.
Two things noticed while working on this, not addressed here
The panel flickers. It does so before these changes as well, at any resend rate, and it does the same on public servers running their own menu implementations, so it looks like it is how the game draws that panel when it is refreshed every tick rather than anything on this side.
Menus are not closed when a player disconnects.
ActiveMenusis only cleaned byCloseActiveMenu, so an instance for a player who left keeps itsOnTicklistener and keeps rendering. That is pre-existing, and the event added here is freed inCloselike the rest of the instance state, so it is not made worse, but it is why the guard is there.Happy to split these into separate PRs if you would rather take them one at a time.