Page MenuHomeWildfire Games

Fix mod version comparison lag in lobby.js, gamesetup.js, load.js and any other place
ClosedPublic

Authored by elexis on May 23 2018, 12:30 PM.

Details

Summary

Prior to rP21239, GetEngineInfo() only returned the names of loaded mods and the engine version, i.e. preinitialized global C++ variables.
The commit introduced code to launch the mods and parse the mod.json files to read the version numbers of these launched mods.
While this is performant for unzipped mods, it is low-performant for zipped mods.
The call consumes 170milliseconds on my machine for only public.zip
If there are 10 such mods, it will freeze for 1.7 seconds each time a savegame is selected.

Since rP21301, lobby.js updateGameList and gamsetup.js sendRegisterGameStanzaImmediate call Engine.GetEngineInfo() too.
The former one calls it once per game, which results in the application freezing for multiple seconds every few seconds!
This is reported every day by lobby players and the performance problem could not be observed until there were many games running simultaenously.

leper claimed to fix this by patching only lobby.js. I hope the other readers are at least convinced now that patching the other GUI pages is necessary too.
Imarok formerly proposed a JS only fix in D1512, a global JS 'cache' in P121.

As repeated (numerous times) already:

  • it doesn't actually fix the underlying problem
  • each time a new GUI page is opened, the common/ code is reloaded and the cache initialized from scratch. This means each time a new GUI page is opened (f.i. message box), the 170ms delay is added! Even if Engine.GetEngineInfo is never used!
  • It adds tons of code that isn't necessary.

Instead we can just cache it in C++ and reduce the performance from 170000 microseconds * numMods to 20 microseconds by adding about a handful of statements in C++.

https://wildfiregames.com/forum/index.php?/topic/24382-mp-lobby-lag-in-alpha-23/

Test Plan

You can reproduce the lag by renaming public to public_ in a working copy, creating a public/ folder and putting public.zip into it.
Add i64 start = JS_Now(); to the top of the GetEngineInfo function and debug_printf("%d\n", JS_Now() - start); to the bottom.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.May 23 2018, 12:30 PM
elexis added inline comments.May 23 2018, 12:32 PM
source/ps/GameSetup/GameSetup.h
85 ↗(On Diff #6625)

It were better to de-decouple this, but it requires the ScriptRuntime to be initialized prior to launching the VFS.
We can consider investigating to change that in Alpha 24 if we are really careful, but not now.

Vulcan added a subscriber: Vulcan.May 23 2018, 12:42 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/554/display/redirect

Agreed that after much debate this is the best solution.
I'll compile out of doing-the-right-thingness for accepting but this is OK by me.

it doesn't actually fix the underlying problem

Well technically neither does this since it doesn't fix the VFS being slow :p . I'll make a patch for that for A24.

source/ps/GameSetup/GameSetup.h
85 ↗(On Diff #6625)

Not necessarily if we split the mounting of mods from the mounting of "core" stuff and we mount mods at a later point, either, which is imo a preferable alternative.

source/ps/Mod.cpp
106 ↗(On Diff #6625)

You might pass this by reference here perhaps.

Well technically neither does this since it doesn't fix the VFS being slow :p . I'll make a patch for that for A24.

I don't know if there is a bug in the VFS. If there is, it should be fixed.
But no matter how good the code is, it has to open multiple files, possibly large fragmented zip files, reading mod.json from it, parsing the JSON data, possibly dozens of mods, so I doubt that this can't be optimized to microseconds without caching it.

source/ps/GameSetup/GameSetup.h
85 ↗(On Diff #6625)

Given

	// Do this as soon as possible, because it chdirs
	// and will mess up the error reporting if anything
	// crashes before the working directory is set.
	InitVfs(args, flags);

to me it appears more stable to initialize SM first rather than initializing VFS later. We may or may not see later.

source/ps/Mod.cpp
106 ↗(On Diff #6625)

Correct. There are about 30 occurrences where some of them do pass it by ref while many don't.

I don't know if there is a bug in the VFS. If there is, it should be fixed.

Well zip files are supposed to be faster than non-zip-files, so this sounds like a bug to me. I'll make a patch as said before

But no matter how good the code is, it has to open multiple files, possibly large fragmented zip files, reading mod.json from it, parsing the JSON data, possibly dozens of mods, so I doubt that this can't be optimized to microseconds without caching it.

Agreed, I was just messing around.

source/ps/GameSetup/GameSetup.h
85 ↗(On Diff #6625)

Agreed, but I think both should be done really. I'll make a patch.

wraitii accepted this revision.May 23 2018, 2:37 PM

I tested' yesterday's version and it worked and was very similar.

This revision is now accepted and ready to land.May 23 2018, 2:37 PM
elexis added a subscriber: Imarok.May 23 2018, 2:46 PM

Also for completeness just wanted to mention an alternative to splitting the existing function into two functions and initializing it in these two files.

It could assume that there is always one mod enabled.
So if the vector is empty, it could init the cache, otherwise it could return the cache.
But that seems like a worse alternative to me, because it assumes g_ScriptRuntime to be initialized and assumes there is always one mod.

Thanks for the feedback.

@Imarok do you want to review too?

wanna add that minimal lobby.js diff, so that we don't call Engine.GetEngineInfo() number_of_lobby_games times per guiTick?
(else looks good)

In D1518#62052, @Imarok wrote:

wanna add that minimal lobby.js diff, so that we don't call Engine.GetEngineInfo() number_of_lobby_games times per guiTick?

Not for 20 microseconds * numGames.
I was rather becoming interested to go the other way and remove the cache of that call in the replaymenu.

elexis retitled this revision from Fix mod version comparison lag in lobby.js, gamesetup.js and any other place to Fix mod version comparison lag in lobby.js, gamesetup.js, load.js and any other place.May 24 2018, 12:50 PM
This revision was automatically updated to reflect the committed changes.
elexis edited the summary of this revision. (Show Details)May 25 2018, 10:57 PM
In D1518#62053, @elexis wrote:
In D1518#62052, @Imarok wrote:

wanna add that minimal lobby.js diff, so that we don't call Engine.GetEngineInfo() number_of_lobby_games times per guiTick?

Not for 20 microseconds * numGames.
I was rather becoming interested to go the other way and remove the cache of that call in the replaymenu.

Ok, I thought we were talking about milliseconds...