Page MenuHomeWildfire Games
Paste P121

Simple launched mod version caching in js
ActivePublic

Authored by Imarok on May 22 2018, 1:01 PM.
Index: binaries/data/mods/mod/gui/common/mod.js
===================================================================
--- binaries/data/mods/mod/gui/common/mod.js (Revision 21822)
+++ binaries/data/mods/mod/gui/common/mod.js (Arbeitskopie)
@@ -1,4 +1,10 @@
/**
+ * Cache the result of Engine.GetEngineInfo()
+ */
+
+const g_EngineInfo = Engine.GetEngineInfo();
+
+/**
* Check the mod compatibility between the saved game to be loaded and the engine
*/
function hasSameMods(modsA, modsB)
Index: binaries/data/mods/public/gui/gamesetup/gamesetup.js
===================================================================
--- binaries/data/mods/public/gui/gamesetup/gamesetup.js (Revision 21822)
+++ binaries/data/mods/public/gui/gamesetup/gamesetup.js (Arbeitskopie)
@@ -1851,8 +1851,8 @@
if (!data || !data.attributes || !data.attributes.settings)
return;
- if (data.engine_info.engine_version != Engine.GetEngineInfo().engine_version ||
- !hasSameMods(data.engine_info.mods, Engine.GetEngineInfo().mods))
+ if (data.engine_info.engine_version != g_EngineInfo.engine_version ||
+ !hasSameMods(data.engine_info.mods, g_EngineInfo.mods))
return;
g_IsInGuiUpdate = true;
@@ -1911,7 +1911,7 @@
Engine.ConfigDB_GetValue("user", "persistmatchsettings") == "true" ?
g_GameAttributes :
{},
- "engine_info": Engine.GetEngineInfo()
+ "engine_info": g_EngineInfo
});
}
@@ -2696,7 +2696,7 @@
"players": clients.list,
"stunIP": g_StunEndpoint ? g_StunEndpoint.ip : "",
"stunPort": g_StunEndpoint ? g_StunEndpoint.port : "",
- "mods": JSON.stringify(Engine.GetEngineInfo().mods),
+ "mods": JSON.stringify(g_EngineInfo.mods),
};
// Only send the stanza if the relevant settings actually changed
Index: binaries/data/mods/public/gui/loadgame/load.js
===================================================================
--- binaries/data/mods/public/gui/loadgame/load.js (Revision 21822)
+++ binaries/data/mods/public/gui/loadgame/load.js (Arbeitskopie)
@@ -9,11 +9,8 @@
{
let savedGames = Engine.GetSavedGames();
- // Get current game version and loaded mods
- let engineInfo = Engine.GetEngineInfo();
-
if (Engine.GetGUIObjectByName("compatibilityFilter").checked)
- savedGames = savedGames.filter(game => isCompatibleSavegame(game.metadata, engineInfo));
+ savedGames = savedGames.filter(game => isCompatibleSavegame(game.metadata, g_EngineInfo));
let gameSelection = Engine.GetGUIObjectByName("gameSelection");
gameSelection.enabled = !!savedGames.length;
@@ -60,9 +57,9 @@
});
let list = g_SavedGamesMetadata.map(metadata => {
- let isCompatible = isCompatibleSavegame(metadata, engineInfo);
+ let isCompatible = isCompatibleSavegame(metadata, g_EngineInfo);
return {
- "date": generateSavegameDateString(metadata, engineInfo),
+ "date": generateSavegameDateString(metadata, g_EngineInfo),
"mapName": compatibilityColor(translate(metadata.initAttributes.settings.Name), isCompatible),
"mapType": compatibilityColor(translateMapType(metadata.initAttributes.mapType), isCompatible),
"description": compatibilityColor(metadata.description, isCompatible)
@@ -79,7 +76,7 @@
// Change these last, otherwise crash
// list strings used in the delete dialog
- gameSelection.list = g_SavedGamesMetadata.map(metadata => generateSavegameLabel(metadata, engineInfo));
+ gameSelection.list = g_SavedGamesMetadata.map(metadata => generateSavegameLabel(metadata, g_EngineInfo));
gameSelection.list_data = g_SavedGamesMetadata.map(metadata => metadata.id);
let selectedGameIndex = g_SavedGamesMetadata.findIndex(metadata => metadata.id == selectedGameId);
@@ -116,7 +113,7 @@
Engine.GetGUIObjectByName("savedVictory").caption = metadata.initAttributes.settings.VictoryConditions.map(victoryConditionName => translateVictoryCondition(victoryConditionName)).join(translate(", "));
let caption = sprintf(translate("Mods: %(mods)s"), { "mods": modsToString(metadata.mods) });
- if (!hasSameMods(metadata.mods, Engine.GetEngineInfo().mods))
+ if (!hasSameMods(metadata.mods, g_EngineInfo.mods))
caption = coloredText(caption, "orange");
Engine.GetGUIObjectByName("savedMods").caption = caption;
@@ -133,9 +130,8 @@
let metadata = g_SavedGamesMetadata[gameSelection.selected];
// Check compatibility before really loading it
- let engineInfo = Engine.GetEngineInfo();
- let sameMods = hasSameMods(metadata.mods, engineInfo.mods);
- let sameEngineVersion = hasSameEngineVersion(metadata, engineInfo);
+ let sameMods = hasSameMods(metadata.mods, g_EngineInfo.mods);
+ let sameEngineVersion = hasSameEngineVersion(metadata, g_EngineInfo);
if (sameEngineVersion && sameMods)
{
@@ -150,7 +146,7 @@
if (metadata.engine_version)
message += sprintf(translate("This savegame needs 0 A.D. version %(requiredVersion)s, while you are running version %(currentVersion)s."), {
"requiredVersion": metadata.engine_version,
- "currentVersion": engineInfo.engine_version
+ "currentVersion": g_EngineInfo.engine_version
}) + "\n";
else
message += translate("This savegame needs an older version of 0 A.D.") + "\n";
@@ -161,7 +157,7 @@
metadata.mods = [];
message += translate("This savegame needs a different sequence of mods:") + "\n" +
- comparedModsString(metadata.mods, engineInfo.mods) + "\n";
+ comparedModsString(metadata.mods, g_EngineInfo.mods) + "\n";
}
message += translate("Do you still want to proceed?");
Index: binaries/data/mods/public/gui/lobby/lobby.js
===================================================================
--- binaries/data/mods/public/gui/lobby/lobby.js (Revision 21822)
+++ binaries/data/mods/public/gui/lobby/lobby.js (Arbeitskopie)
@@ -1017,7 +1017,7 @@
Math.round(playerRatings.reduce((sum, current) => sum + current) / playerRatings.length) :
g_DefaultLobbyRating;
- if (!hasSameMods(JSON.parse(game.mods), Engine.GetEngineInfo().mods))
+ if (!hasSameMods(JSON.parse(game.mods), g_EngineInfo.mods))
game.state = "incompatible";
return game;
@@ -1178,7 +1178,7 @@
messageBox(
400, 200,
translate("Your active mods do not match the mods of this game.") + "\n\n" +
- comparedModsString(JSON.parse(game.mods), Engine.GetEngineInfo().mods) + "\n\n" +
+ comparedModsString(JSON.parse(game.mods), g_EngineInfo.mods) + "\n\n" +
translate("Do you want to switch to the mod selection page?"),
translate("Incompatible mods"),
[translate("No"), translate("Yes")],
Index: binaries/data/mods/public/gui/replaymenu/replay_menu.js
===================================================================
--- binaries/data/mods/public/gui/replaymenu/replay_menu.js (Revision 21822)
+++ binaries/data/mods/public/gui/replaymenu/replay_menu.js (Arbeitskopie)
@@ -1,9 +1,4 @@
/**
- * Used for checking replay compatibility.
- */
-const g_EngineInfo = Engine.GetEngineInfo();
-
-/**
* Needed for formatPlayerInfo to show the player civs in the details.
*/
const g_CivData = loadCivData(false, false);
Index: binaries/data/mods/public/gui/savegame/save.js
===================================================================
--- binaries/data/mods/public/gui/savegame/save.js (Revision 21822)
+++ binaries/data/mods/public/gui/savegame/save.js (Arbeitskopie)
@@ -36,8 +36,7 @@
for (let game of savedGames)
g_Descriptions[game.id] = game.metadata.description || "";
- let engineInfo = Engine.GetEngineInfo();
- gameSelection.list = savedGames.map(game => generateSavegameLabel(game.metadata, engineInfo));
+ gameSelection.list = savedGames.map(game => generateSavegameLabel(game.metadata, g_EngineInfo));
gameSelection.list_data = savedGames.map(game => game.id);
gameSelection.selected = Math.min(gameSelection.selected, gameSelection.list.length - 1);

Event Timeline

Imarok edited the content of this paste. (Show Details)May 22 2018, 1:01 PM
Imarok changed the title of this paste from untitled to Masterwork From Distant Lands.
Imarok updated the paste's language from autodetect to autodetect.
Imarok changed the title of this paste from Masterwork From Distant Lands to Simple caching in js.May 22 2018, 1:02 PM
Imarok added subscribers: elexis, wraitii.

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.

In P121#915, @elexis wrote:

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.

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.

In P121#917, @elexis wrote:

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.

Should be ok with adding a let in lobby.js

I think the lobby change is still OK, but I'm fine with not adding it for slight readability reasons.

In P121#920, @wraitii wrote:

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.

elexis changed the visibility from "All Users" to "Public (No Login Required)".May 22 2018, 7:47 PM
elexis changed the title of this paste from Simple caching in js to Simple launched mod version caching in js.Jun 3 2018, 2:22 PM