Event Timeline
What speaks against this approach for the release?
It is global.
It needs no maintenance.
It changes only simple things, so probability of breaking is quite low.
Of course this does not cache C++ invocations of Engine.GetEngineInfo(), but those aren't called that often.
This is intended as a quick robust hotfix for the rerelease. (We should not do complex changes for the rerelease, as it's easy to forget about an edge case there)
What speaks against this approach for the release?
See irclogs yesterday. Recaping:
My main reason for prefering a C++ solution is that it is done once, rather than N times and that it is implementable with the same amount of code in a different place. So why pick the worse alternative if the alternative has presumably the same cost. Prefering quantity over quality doesn't pay off in the long term.
A second weaker argument is that const g_EngineInfo = Engine.GetEngineInfo(); is called each time a new GUI page is opened, every message box, so you get 170ms delay (on my machine) each time even if not used.
A third weaker argument is that new pages don't have it cached, we need to go through all GUI pages to check it for completeness.
Hmm, that's a point..
A third weaker argument is that new pages don't have it cached, we need to go through all GUI pages to check it for completeness.
IMO Globals are something to be avoided where possible. We have so many of them already and the ideal number of total globals is somewhere near 2.
After implementing the C++ cache, it consumes 10-30ms instead of 170ms.
30 games * 20ms per call = 0.6 seconds every few seconds or more often.
So I'd be ok with paying the cost of 1 added line of code in the updateGameList function of lobby.js as you had proposed once, while leaving the rest in JS as is?
Reportedly the gamesetup is slow too when sending stanzas, but that must be a problem of a factor 10 less than in the lobby page (as the problem is multiplied with the number of games there, while gamesetup stanzas are only sent all 3 seconds or upon client join / leave.
Uh, wrong!
20 Microseconds, not 20 milliseconds! No JS changes needed at all!
167971 microseconds for the function as is currently!
So it's by 4 orders of magnitude faster now.
I think the lobby change is still OK, but I'm fine with not adding it for slight readability reasons.
At minimum we should add a local var, so that GetEngineInfo is only called once per guiTick
20 microseconds per call, thats 0.4 milliseconds for 20 games, and 4 milliseconds for 200 games.
The benefit seems questionable in comparison to the other code that is executed in that function (sorting, comparing, filtering, ...) albeit it not being wrong to increase the complexity for those dozens of microseconds.
It's more of a "it's good practice to not call constant things in function bodies", but I agree that it ever so slightly lowers readability here.