HomeWildfire Games

Alpha 23 "lobby lag" release fix.

Description

Alpha 23 "lobby lag" release fix.

Caches the loaded mod versions, so that GetEngineInfo doesn't read the zip and json files everytime and returns about 1000 times faster.
Adds two missing includes.

The lobby froze multiple times every few seconds on updateGameList().
The gamesetup page was slowed down with every stanza sent and the
load savegame selection page was slowed down per savegame selection,
proportional to the number of installed zipped mods.

Introduced by: rP21239 and rP21301
Differential Revision: https://code.wildfiregames.com/D1518
Reviewed By: wraitii
Comments By: Imarok (in D1512, P121), leper (in the lobby)

Event Timeline

vladislavbelov added inline comments.
/ps/trunk/source/ps/GameSetup/GameSetup.h
85

Why we can't call CacheEnabledModVersions at the end of MountMods, if it's already required? Or the comment seems incomplete.

elexis added inline comments.May 25 2018, 6:26 PM
/ps/trunk/source/ps/GameSetup/GameSetup.h
85

I'm sorry but I have explained this already to three different people. You can see it in the two revision proposals, or by trying to call it at the end of the other function or by looking at the function argument of the new function and the line above the calls to that function.

vladislavbelov added inline comments.May 25 2018, 6:52 PM
/ps/trunk/source/ps/GameSetup/GameSetup.h
85

So, if you explained to three different people, then it means that the comment isn't helpful and can be improved.

Stan added a subscriber: Stan.May 25 2018, 6:55 PM
Stan added inline comments.
/ps/trunk/source/ps/GameSetup/GameSetup.h
85

Wraitii said he'll make a patch for this pecial edge case with the VFS and Spidermonkey.

vladislavbelov added inline comments.May 25 2018, 6:58 PM
/ps/trunk/source/ps/GameSetup/GameSetup.h
85

I don't worry about the code, because I know what happens there. My first question can be asked by someone else when reading the code. So I worry, because the comment seems incomplete, maybe misleading.

elexis added inline comments.May 25 2018, 6:59 PM
/ps/trunk/source/ps/GameSetup/GameSetup.h
85

It means that the second person didn't read my answer to the first person, that the third person didn't read my answer to the first and seond person and that the fourth person didn't read my answer to the first second and third person - nor ever went near to doing something with the code. Anyone who wants to do something with the code, trying to change it in any way will directly notice the matter at hand.

wraitii added inline comments.
/ps/trunk/source/ps/GameSetup/GameSetup.h
85

Truthfully the comment is accurate, and we would like to do it in the function, but we need JS. It could be worth adding "JS engine isn't always initialised when mounting mods" but it's a bit of a detail.

elexis added inline comments.May 26 2018, 11:21 AM
/ps/trunk/source/ps/GameSetup/GameSetup.h
85

To answer Vladislav (still a repetition of a repetition):
Mods are launched first, then the ScriptRuntime is initialized, then we add the call to ParseJSON for mod.json to get the version number which required ScriptRuntime. The call to caching function is the first one after g_ScriptRuntime is initialized (which is the code hint I meant).
It doesn't look harmful to initialize ScriptRuntime before initializing mods, but it's a bad time to try to change it, we can try to consider A24.

vladislavbelov added inline comments.May 28 2018, 5:07 PM
/ps/trunk/source/ps/GameSetup/GameSetup.h
85

Why do you tell me how it works? I want a more detailed comment for newcomers. Because currently we have a lot of not well-documented code. And that is the one of reasons why only a small group of people understand what's going on there.

elexis added inline comments.May 28 2018, 7:30 PM
/ps/trunk/source/ps/GameSetup/GameSetup.h
85

Well because you asked, especially answering why the proposal to call it at the end doesn't work.

(On code comment quantity:
When one has to lookup something across multiple files before coming to a conclusion, that can be added, for instance in D1519 I proposed adding a comment for that reason.
But this diff already adds two lines of comments that direct someone looking at this and the comment stating the init order would take about as long as reading the init order.
The idea to call the caching function after launching the mods is the first idea someone has looking at this and when trying to move it up there the first thing one notices is that we don't have a g_ScriptRuntime to pass to that function, the second thing one notices is that g_Scriptruntime is initialized right before this call. Then the reader receives the conclusion. Comments should be 1-2 lines per function, if it can also cause people to not read at all, think that it's complex, don't develop own ideas how to change it.
If someone is a newcomer and is unable to understand this context, he shouldn't think about touching that code to begin with.
The more verbose the code, the more readercost and maintenance. Finding an information must cost considerably more time to comprehend than reading 5-20 lines of code. 100 lines of code and 10 lines of comment are better to comprehend than 100 lines of code 50 lines of comments.
I guess it's subjective where to draw the line, other than that meh.)

vladislavbelov added inline comments.May 29 2018, 2:52 AM
/ps/trunk/source/ps/GameSetup/GameSetup.h
85

Did you read my comment above?:

I don't worry about the code, because I know what happens there. My first question can be asked by someone else when reading the code. So I worry, because the comment seems incomplete, maybe misleading.

Comments should be 1-2 lines per function

What is it based on?

It depends on the function complexity. We need to share main idea and what's going, but not simply comments to were. Example: https://cs.chromium.org/chromium/src/ui/views/view.h?q=view.h&sq=package:chromium&g=0&l=361, the file has comments with different number of lines.

The more verbose the code, the more readercost and maintenance. Finding an information must cost considerably more time to comprehend than reading 5-20 lines of code.

Split the header and the implementation, they were made for different things. Header has much less code and presents the interface.
Also do you suggest to spend time for every newcomer instead of add 1-2 additional line if you already know the answer?

100 lines of code and 10 lines of comment are better to comprehend than 100 lines of code 50 lines of comments

Where did you get these numbers? As I said it depends on the code. There is no the best number of comments lines. Only how far you understand. Sometimes you need much more number of comment lines than code lines. I.e. 0x5f3759df for fast inverse square root. With 1-2 lines you're only able to say, that it was specially chosen. But it's nothing for the understanding.

elexis added inline comments.May 29 2018, 5:07 AM
/ps/trunk/source/ps/GameSetup/GameSetup.h
85

To me this is really simple and I think the readers who didn't understand this are simply lazy.
Yes, readers should read 4 lines of code rather than getting the answer from 1 line of comment.
I know many answers, but that doesn't mean that I'll write them all here.
Code is good if it speaks for itself and it does here, just noone cared to actually look at the code.

ffffffff added a subscriber: ffffffff.EditedJun 5 2018, 1:40 PM

Problem directly after release investigated by fpre.

Directly explained to the staff coders and acknowledged by leper...

And of course directly temporarely fixed in fgod mod.

1 day after Release!

elexis added a comment.Jun 5 2018, 2:05 PM

And of course directly temporarely fixed in fgod mod.

1 day after Release!

Release May 17th https://play0ad.com/new-release-0-a-d-alpha-23-ken-wood/
Your diff May 20th https://github.com/fraizy22/fgodmod/commit/68eb5755b1e1ea9f5d739d080467744203af8119
Our first diff that was as limited (doesn't fix gamesetup and loading screen, nor any other future reoccurrence) on May 21st https://code.wildfiregames.com/D1512
Finishing the discussing with more than 6 people who don't read what I said before May 24th.
Other than that your mod helped a lot of OSX players, thanks for that.

ffffffff added a comment.EditedJun 6 2018, 2:23 AM

sounds like a thanks for investigation
also the archive builder problem....
(zipped, unzipped)
thanks for investigation fpre

wraitii raised a concern with this commit.Dec 12 2020, 5:36 PM

Assuming we want to keep caching, a much better approach would probably to cache lazily in GetEngineInfo instead of having to call CacheEnabledModVersions everywhere.

See also my comment on rP15677 for the source of the lag, which went unexplained, and probably a better solution in general (or at least I'm hoping, something that makes this irrelevant).

This commit now has outstanding concerns.Dec 12 2020, 5:36 PM
wraitii resigned from this commit.May 20 2021, 4:42 PM

Fixed in rP25474

This commit no longer requires audit.May 20 2021, 4:42 PM