Fix lag and persist penalties #59
Conversation
Add instrumentation to code potentially causing lag. Prefix our metrics with crg_. Differentiate the two sendUpdates functions.
Due to variable shadowing, this code was checking every passed update against every passed update, for every WS connection - which is to say O(updates * updates * connections). Due to the gameSnapshot logic unneccessarily sending everything it has (which can easily be hundreds of updates) several times per jam start/end this can cause massive lag which has been observed to get into the seconds range. Also remove dead code.
This brings us closer to having penalties consistent with other game data. It also avoids having the game info logic send the complete team data structure as an update on every jam start/end/penalty etc.
At jam start/end the lastscore, lead and starpass events happen, and previously this would result in sending the entire team including skaters and their penalties on every jam start/end. By fast pathing these, this no longer happens.
These need updating for the new WS paths.
|
Fixes #57 |
JeneralPain
requested review from
JeneralPain
and removed request for
JeneralPain
There was a problem hiding this comment.
I'm not convinced your removal of the argument to sendUpdates is the correct fix.
If I understand the code correctly, there are three key sets:
- The set of all keys, aka state.keySet()
- The set of updated keys, aka the removed argument keys
- The set of keys that the client has registered, aka Conn.paths
The original code checked 1 against 2, which is bad. The new code checks 1 against 3, which is better.
The correct solution would be to check 2 against 3.
Also this change has the potential to uncover previously hidden bugs in the clients: Unless there is another filter down the line, the old code sent all updates to all clients, while the new code only sends updates the client has registered. If a client forgets to register a path it needs, this would be hidden by the old code but result in missing updates with the new code.
That may be better, but it requires a rewrite of how this entire data flow works as 2 doesn't exist currently. Keep in mind that
This is a possibility, however I didn't spot this in any of the code in the screens and none of the alpha testers reported issues. |
|
2 does exist. It is in the removed argument That being said, I am on board with getting this improvement shipped (assuming there are no severe issues with the later commits - I haven't reviewed them yet). We just should not consider the issue completely fixed yet and should have the possible uncovering of bugs in mind in case any reports come in. |
|
That change is just one part of the fix, the other is massively reducing the amount of data this code is handling in the first place. Between them it should be pretty hard for the performance to regress here. |
|
And I agree that reducing the size of updates removes the impact in terms of efficiency. But having analyzed the code, I now have a strong urge to make it a. more efficient and b. easier to follow. I'll probably put in some effort to that end after the release is out. |
|
The existing code style is quite hard to work with, given the breadth of the changes I gave up trying to match it. I'm planning on running a formatter over it at some point, the number of brace-less if/for statements is a risk. |
|
@brian-brazil yeah some of the core code is 10+ years old. There's a plan to tackle that but no time. |
This is only a partial solution. To complete it the penalty logic should be implemented in the same way everything else is and all
updatecalls ingame/*.javaremoved, with the relevant penalty and penalty update logic moving to theDefaultSkaterModel.This PR also persists penalties.