Fix replay menu loading time by using a cache file
ClosedPublic

Authored by Imarok on Jan 5 2017, 1:04 PM.

Details

Summary

Add a replay cache

Test Plan

Ensured new replays get added to the cache
Ensured loading the replay menu now is faster
Ensured deleted replays are handled correctly

Use this command on unix to drop the HDD cache:

echo 3 | sudo tee /proc/sys/vm/drop_caches

In order to test for the bug described in #4320, change GetDirectoryName function to return replêys (i.e. ^+e).

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.
There are a very large number of changes, so older changes are hidden. Show Older Changes
elexis edited the test plan for this revision. (Show Details)May 18 2017, 5:09 PM

Tested every case.
Besides the missing OsString calls and few style improvements, the implementation leaves a rock solid appearance.
If we have a running game in one instance and another instance in the replay menu and press the reload cache button, the file will be updated with the most recent entry :-)

Besides replacing the // Remove entries without data with a condition when adding those entries, I've applied the style remarks and hence would accept this patch:

VFS reasoning:
The great performance issue is caused by having hundreds if not thousands of files around, in the worst case scattered across a mechanical disk.
Even if defragmented, still requires a massive overhead of individual file open, read and close calls.
Even with a VFS system these calls would be present if the file isn't stored in a single file. i.e. one huge zip archive with all files as we have with public.zip, see also https://trac.wildfiregames.com/wiki/Virtual_File_System.
But this is inadvisable, because we want players to easily share replays. Also storing everything in single file can mean that the entire replay database is lost if writing that file fails in some way.
Therefore if/when VFS support is added to this file, it will still have to use this separate cache file.

Future Work:

  1. a. The file doesn't need to be indented (even though it passes false in the stringify call, it still becomes intented for any reason).
  2. b. Better zip that file altogether, so that it's only some kilobytes instead of a handful of megabytes when having many replays.
  3. The cache file should contain relative file paths (so that the cache file could be moved to another directory without becoming useless):
{
  "file": "/home/elexis/.local/share/0ad/replays/0.0.22/2016-11-21_0009/commands.txt",
  "directory": "2016-11-21_0009",
  "fileSize": 2643,
  "attribs": {
    "settings": {
      "PlayerData": [
        {
          "Name": "elexis",
          "Civ": "brit",
          "Color": {
            "r": 46,
            "g": 46,
            "b": 200
          },
          "AI": "",
          "AIDiff": 5,
          "Team": -1
        },
        {
          "Name": "Player 2",
          "Civ": "ptol",
          "Color": {
            "r": 150,
            "g": 20,
            "b": 20
          },
          "AI": "",
          "AIDiff": 3,
          "Team": -1
        },
        {
          "Name": "Cleon",
          "Civ": "athen",
          "Color": {
            "r": 50,
            "g": 165,
            "b": 5
          },
          "AI": "petra",
          "AIDiff": 5,
          "Team": -1
        },
        {
          "Name": "Ptolemy Philometor",
          "Civ": "ptol",
          "Color": {
            "r": 230,
            "g": 230,
            "b": 75
          },
          "AI": "petra",
          "AIDiff": 5,
          "Team": -1
        },
        {
          "Name": "Dasaratha Maurya",
          "Civ": "maur",
          "Color": {
            "r": 50,
            "g": 170,
            "b": 170
          },
          "AI": "petra",
          "AIDiff": 5,
          "Team": -1
        }
      ],
      "CheatsEnabled": true,
      "CircularMap": true,
      "Size": 128,
      "GameType": "conquest",
      "VictoryScripts": [
        "scripts/TriggerHelper.js",
        "scripts/ConquestCommon.js",
        "scripts/Conquest.js"
      ],
      "WonderDuration": 20,
      "PopulationCap": 300,
      "Ceasefire": 0,
      "StartingResources": 300,
      "Name": "Survival of the Fittest",
      "Script": "survivalofthefittest.js",
      "Description": "[color=\"red\"]IMPORTANT NOTE: AI PLAYERS DO NOT WORK WITH THIS MAP[/color]\n\nProtect your base against endless waves of enemies. Use your woman citizen to collect the treasures at the center of the map before others do, and try to build your base up. The last player remaining will be the winner!",
      "BaseTerrain": [
        "medit_sea_depths"
      ],
      "BaseHeight": 30,
      "Keywords": [
        "trigger"
      ],
      "Preview": "survivalofthefittest.png",
      "RatingEnabled": false,
      "DisabledTemplates": [
        "structures/ptol_lighthouse"
      ],
      "LockTeams": false,
      "LastManStanding": true,
      "TriggerScripts": [
        "scripts/TriggerHelper.js",
        "scripts/ConquestCommon.js",
        "scripts/Conquest.js",
        "scripts/TriggerHelper.js",
        "random/survivalofthefittest_triggers.js"
      ],
      "mapType": "random",
      "Seed": 2454904821,
      "AISeed": 4164250586
    },
    "map": "maps/random/survivalofthefittest",
    "mapType": "random",
    "mapPath": "maps/random/",
    "mapFilter": "all",
    "gameSpeed": 2,
    "matchID": "CC79D21F5329982B",
    "script": "survivalofthefittest.js",
    "timestamp": "1479746954",
    "engine_version": "0.0.22",
    "mods": [
      "mod",
      "public"
    ]
  },
  "duration": 9
},
binaries/data/mods/public/gui/replaymenu/replay_actions.js
133 ↗(On Diff #1646)

Why is 0 excluded? selected != -1

binaries/data/mods/public/gui/replaymenu/replay_menu.js
77 ↗(On Diff #1646)

Should have a comment for the loadReplays proposed in some other inline comment.

@param while adding weird new stuff

binaries/data/mods/public/gui/replaymenu/replay_menu.xml
251 ↗(On Diff #1646)

just Reload, but add a tooltip to explain what this is actually about.

<translatableAttribute id="tooltip">Rebuild the replay cache from scratch. Potentially slow!</translatableAttribute>

binaries/data/mods/public/gui/session/session.js
671 ↗(On Diff #1646)

Correct check, because replayDirectory also returns the filename of the currently played replay

672 ↗(On Diff #1646)

It's probably a time-saving feature that this isn't called upon Alt+F4.

source/ps/VisualReplay.cpp
69 ↗(On Diff #1646)

Can be removed IMO, too noisy, only opening of one file (which may hang if the HDD hangs. Still nothing we can do about that with code then.)
Since the performance effects of file reading should be well understood in theory, I don't see any other performance issues arising here anytime soon.

76 ↗(On Diff #1646)

not really needed

77 ↗(On Diff #1646)

That sounds a lot like suffering from the same breakage reported in #4320, i.e. should use an OsString().

Should actually be tested by returning something like replêys in GetDirectoryName.

89 ↗(On Diff #1646)

Bit awkward to have one yellow warning and two red errors, but this still isn't an actual error.

ERROR: JS_ParseJSON failed!
ERROR: SyntaxError: JSON.parse: unexpected character at line 1 column 1 of the JSON data
WARNING: The replay cache file is corrupted, it will be deleted
90 ↗(On Diff #1646)

OsString()

96 ↗(On Diff #1646)

Unneeded and noisy too IMO

101 ↗(On Diff #1646)

OsString

105 ↗(On Diff #1646)

OsString

106 ↗(On Diff #1646)

OsString

114 ↗(On Diff #1646)

(
Since this is only called locally, could be made 'private' of that file (i.e. not a namespace 'member' like goBackToLineBeginning). But that will have the ugly side-effect of having to call VisualReplay::GetDirectoryName() instead of GetDirectoryName() and technically it could be called from outside for any reason.

Merging those 2 functions doesn't seem to be nicely doable because this one contains return statements which would have to be compensated with ugly scopes.
)

116 ↗(On Diff #1646)

That can be kept since it is guaranteed to consume a lot of time if many unknown replays are added.

119 ↗(On Diff #1646)

Comment not needed anymore when using the readable types

120 ↗(On Diff #1646)

Since the type is used at least twice, a typedef would be nice, like typedef std::map<OsPath, std::pair<u32, off_t>> replayCacheMap;

126 ↗(On Diff #1646)

Unneeded, noisy

128 ↗(On Diff #1646)

Correct to use u32, because 2^32-1 is the greatest number of elements a JS array can hold.

141 ↗(On Diff #1646)

This C++11 doesn't appear to be saving anything in comparison to fileList[fileName] = ...

153 ↗(On Diff #1646)

// Specifies whether the next replay file should be kept

156 ↗(On Diff #1646)

Not needed

161 ↗(On Diff #1646)

don't want to lose -> want to save
pretty fast part not needed

165 ↗(On Diff #1646)

u32 filesize:
Better use the actual type returned than converting things needlessly (and assuming it never encounters a replay file larger than 4GB).

In file_system.h we see the filesize returned is of the type off_t, not exactly specified by posix.

Using std::time_t and then converting it to double in the scriptInterface.SetProperty calls is what we do in the savegame & replay timestamp code, because double is equivalent to the only JS number type used. The greatest number is 2^53 iirc (i.e. holds much more than 2^32, but still less than 2^64).

ps/Replay.cpp:	m_ScriptInterface.SetProperty(attribs, "timestamp", (double)std::time(nullptr));
ps/SavedGame.cpp:	simulation.GetScriptInterface().SetProperty(metadata, "time", (double)now);
165 ↗(On Diff #1646)

Why not remove the`string8` conversion and use the OsPath ref from that DirectoryNames vector?

166 ↗(On Diff #1646)

should be self-evident

173 ↗(On Diff #1646)

Remove the conversion then

201 ↗(On Diff #1646)

zu is both the correcŧ entry for a printf using std::size_t and an interesting band to recommend while at it.

201 ↗(On Diff #1646)

Three lines feel a bit noisy, why not all in one:

	debug_printf(
		"Loading %zu cached replays, removed %zu outdated entries, loaded %i new entries\n",
		fileList.size(),
		fileList.size() - copyFromOldCache.size(),
		i);
211 ↗(On Diff #1646)

not

230 ↗(On Diff #1646)

unneeded comment

232 ↗(On Diff #1646)

How about not adding files without attribs instead of removing them?

233 ↗(On Diff #1646)

replayNonEmpty?

331 ↗(On Diff #1646)

Remove this TIMER call, since we want only one call for the loading of all replay files, not one for each IMO

407 ↗(On Diff #1646)

Use (double), the whole JS number type parts takes place at D205

449 ↗(On Diff #1646)

Since this is only called when ending the game, maybe.

121 ↗(On Diff #160)

Actually we don't have doxygen comments here, but in the header file (unless the function only exists in the cpp file). Also this doxygen comment differs from the one in the header.

source/ps/VisualReplay.h
66 ↗(On Diff #1646)

As mentioned in another inline comment, could be renamed to compareFiles to become agnostic of the way how to achieve that

68 ↗(On Diff #1646)

Remove the default

74 ↗(On Diff #1646)
  • Dash: @param reload - reload....
  • "the whole cache" sounds like it reloads only a part of the cache otherwise.
  • The boolean is passed down to compareFileSize. So basically we decide whether it will be a fast load (assuming that same filesize means same file contents) or a slow load (compare file contents exactly). Better phrase it compareFiles to distinguish it from the amibguous reload and make it abstract from the way how it is achieved (compareFileSize)
115 ↗(On Diff #1646)

period

source/ps/scripting/JSInterface_VisualReplay.cpp
56 ↗(On Diff #1646)

(Would have suggested OsPath is it wasn't the case everywhere else in this file)

Imarok marked 20 inline comments as done.May 22 2017, 10:52 PM

updated diff coming soon

source/ps/VisualReplay.cpp
120 ↗(On Diff #1646)

Don't think we really need a typedef here ^^

elexis added inline comments.May 22 2017, 11:22 PM
source/ps/VisualReplay.cpp
120 ↗(On Diff #1646)

No, but it were nice to not repeat the type right? And the compiler will inline it anyway

Imarok updated this revision to Diff 2194.May 25 2017, 11:42 PM
Imarok marked 16 inline comments as done.

Apply all remarks besides OsPath (D518)

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1355/ for more details.

Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/replaymenu/replay_menu.js
|  99| »   »   if·(g_MapNames.indexOf(replay.attribs.settings.Name)·==·-1·&&·replay.attribs.settings.Name·!=·"")
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with ''.

binaries/data/mods/public/gui/replaymenu/replay_menu.js
| 180| »   if·(attribs.settings.PlayerData.length·&&·attribs.settings.PlayerData[0]·==·null)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with 'null'.
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '+'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|1590|1590| 
|1591|1591| 		playerStatistics.economyScore += total + ",";
|1592|1592| 		playerStatistics.militaryScore += Math.round((player.sequences.enemyUnitsKilledValue[maxIndex] +
|1593|    |-			player.sequences.enemyBuildingsDestroyedValue[maxIndex]) / 10)  + ",";
|    |1593|+			player.sequences.enemyBuildingsDestroyedValue[maxIndex]) / 10) + ",";
|1594|1594| 		playerStatistics.totalScore += (total + Math.round((player.sequences.enemyUnitsKilledValue[maxIndex] +
|1595|1595| 			player.sequences.enemyBuildingsDestroyedValue[maxIndex]) / 10)) + ",";
|1596|1596| 

binaries/data/mods/public/gui/session/session.js
| 407| »   »   colorizeHotkey("%(hotkey)s"·+·"·",·"selection.idleworker")·+
|    | [NORMAL] ESLintBear (no-useless-concat):
|    | Unexpected string concatenation of literals.

binaries/data/mods/public/gui/session/session.js
| 822| »   »   i·==·0·||
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/session.js
| 824| »   »   g_GameAttributes.settings.PlayerData[i].AI·!=·"");
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with ''.

binaries/data/mods/public/gui/session/session.js
| 926| »   if·(direction·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/session.js
| 950| »   »   let·panelEnt·=·g_PanelEntities.find(panelEnt·=>·ent·==·panelEnt.ent);
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/session.js
| 974| »   g_PanelEntities·=·g_PanelEntities.sort((panelEntA,·panelEntB)·=>·panelEntIndex(panelEntA.ent)·-·panelEntIndex(panelEntB.ent))
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/gui/session/session.js
|1054| »   »   button.hidden·=·g_Groups.groups[i].getTotalCount()·==·0;
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/session.js
|1055| »   »   button.onpress·=·(function(i)·{·return·function()·{·performGroup((Engine.HotkeyIsPressed("selection.add")·?·"add"·:·"select"),·i);·};·})(i);
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/session.js
|1056| »   »   button.ondoublepress·=·(function(i)·{·return·function()·{·performGroup("snap",·i);·};·})(i);
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/session.js
|1057| »   »   button.onpressright·=·(function(i)·{·return·function()·{·performGroup("breakUp",·i);·};·})(i);
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/session.js
|1062| »   »   »   let·icon·=·GetTemplateData(GetEntityState(g_Groups.groups[i].getEntsGrouped().reduce((pre,·cur)·=>·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/session.js
|1116| »   »   if·(player·!=·0·&&
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/gui/session/session.js
|1212| »   »   button.onpress·=·(function(e)·{·return·function()·{·selectAndMoveTo(e);·};·})(researchStarted[tech].researcher);
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/session.js
|1400| »   »   if·(+playerID·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/42/ for more details.

elexis accepted this revision.May 26 2017, 6:13 AM

Should test whether this compiles under windows, but it's not like we don't have a release blocker that requires changing most path calls anyway.

2016-07-19-QuakeNet-#0ad-dev.log
15:48 < Imarok> Any new tickets to suggest me working on? ;)
15:53 < elexis> Imarok: if you want to be really really useful, implement the replay menu cache

Said and done, thanks! The endless loading times have been discouraging me from opening the developer overlay since it's introduction!

binaries/data/mods/public/gui/replaymenu/replay_menu.js
77 ↗(On Diff #1646)

@param {boolean} reload - if the cache should be relaoded.

Why not name it compareFiles too?

I had written this one since the above description is a bit arbitrary "data contains data"

* @param {object} replaySelectionData - Currently selected filters and item to be restored after the loading.
* @param {bool} compareFiles - If true, compares files briefly (which might be slow with optical harddrives),
*                              otherwise blindly trusts the replay cache.
source/ps/VisualReplay.cpp
121 ↗(On Diff #160)

See above, no comments in .cpp but .h

39 ↗(On Diff #2194)

That doesn't need to be global, only in the scope that uses it (see my proposal for examplE).

135 ↗(On Diff #2194)

Why is the filesize u32? It should be double so that we are not bound to 4GB but to 2^53 IMO and it's consistent with the other places.

153 ↗(On Diff #2194)

Why this scope? seems unneeded. Sounds like it came from having had a TIMER in there

157 ↗(On Diff #2194)

We

184 ↗(On Diff #2194)

Didn't we say something about the 4GB limitation and what c++ type resembles the JS number type?

source/ps/VisualReplay.h
49 ↗(On Diff #2194)

false otherwise should be self-evident

57 ↗(On Diff #2194)

might want to add the period here and in the other places for consistency

source/ps/scripting/JSInterface_VisualReplay.cpp
36 ↗(On Diff #2194)

compare flies

source/ps/scripting/JSInterface_VisualReplay.h
28 ↗(On Diff #2194)

compare filet

This revision is now accepted and ready to land.May 26 2017, 6:13 AM
Imarok added inline comments.May 26 2017, 11:30 AM
source/ps/scripting/JSInterface_VisualReplay.h
28 ↗(On Diff #2194)

Beef filet is much more tasty than chicken filet. Though Chicken is mostly cheaper.

Imarok marked 10 inline comments as done.May 26 2017, 11:52 AM
Imarok updated this revision to Diff 2215.May 26 2017, 11:52 AM

Apply remarks

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1367/ for more details.

Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/replaymenu/replay_menu.js
| 100| »   »   if·(g_MapNames.indexOf(replay.attribs.settings.Name)·==·-1·&&·replay.attribs.settings.Name·!=·"")
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with ''.

binaries/data/mods/public/gui/replaymenu/replay_menu.js
| 181| »   if·(attribs.settings.PlayerData.length·&&·attribs.settings.PlayerData[0]·==·null)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with 'null'.
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '+'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|1597|1597| 
|1598|1598| 		playerStatistics.economyScore += total + ",";
|1599|1599| 		playerStatistics.militaryScore += Math.round((player.sequences.enemyUnitsKilledValue[maxIndex] +
|1600|    |-			player.sequences.enemyBuildingsDestroyedValue[maxIndex]) / 10)  + ",";
|    |1600|+			player.sequences.enemyBuildingsDestroyedValue[maxIndex]) / 10) + ",";
|1601|1601| 		playerStatistics.totalScore += (total + Math.round((player.sequences.enemyUnitsKilledValue[maxIndex] +
|1602|1602| 			player.sequences.enemyBuildingsDestroyedValue[maxIndex]) / 10)) + ",";
|1603|1603| 

binaries/data/mods/public/gui/session/session.js
| 412| »   »   colorizeHotkey("%(hotkey)s"·+·"·",·"selection.idleworker")·+
|    | [NORMAL] ESLintBear (no-useless-concat):
|    | Unexpected string concatenation of literals.

binaries/data/mods/public/gui/session/session.js
| 829| »   »   i·==·0·||
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/session.js
| 831| »   »   g_GameAttributes.settings.PlayerData[i].AI·!=·"");
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with ''.

binaries/data/mods/public/gui/session/session.js
| 933| »   if·(direction·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/session.js
| 957| »   »   let·panelEnt·=·g_PanelEntities.find(panelEnt·=>·ent·==·panelEnt.ent);
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/session.js
| 981| »   g_PanelEntities·=·g_PanelEntities.sort((panelEntA,·panelEntB)·=>·panelEntIndex(panelEntA.ent)·-·panelEntIndex(panelEntB.ent))
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/gui/session/session.js
|1061| »   »   button.hidden·=·g_Groups.groups[i].getTotalCount()·==·0;
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/session.js
|1062| »   »   button.onpress·=·(function(i)·{·return·function()·{·performGroup((Engine.HotkeyIsPressed("selection.add")·?·"add"·:·"select"),·i);·};·})(i);
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/session.js
|1063| »   »   button.ondoublepress·=·(function(i)·{·return·function()·{·performGroup("snap",·i);·};·})(i);
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/session.js
|1064| »   »   button.onpressright·=·(function(i)·{·return·function()·{·performGroup("breakUp",·i);·};·})(i);
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/session.js
|1069| »   »   »   let·icon·=·GetTemplateData(GetEntityState(g_Groups.groups[i].getEntsGrouped().reduce((pre,·cur)·=>·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/session.js
|1123| »   »   if·(player·!=·0·&&
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/gui/session/session.js
|1219| »   »   button.onpress·=·(function(e)·{·return·function()·{·selectAndMoveTo(e);·};·})(researchStarted[tech].researcher);
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/session.js
|1407| »   »   if·(+playerID·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/53/ for more details.

Sandarac added a comment.EditedMay 26 2017, 4:15 PM

On Windows, this diff causes a crash deep in SpiderMonkey when opening the replay menu (not if there is nothing to load in the replay menu though).

 	msvcr120.dll!521e46a9()	Unknown
 	[Frames below may be incorrect and/or missing, no symbols loaded for msvcr120.dll]	
 	[External Code]	
 	mozjs38-ps-release.dll!js::irregexp::ActionNode::Emit(js::irregexp::RegExpCompiler * compiler, js::irregexp::Trace * trace) Line 4418	C++
 	mozjs38-ps-release.dll!js::irregexp::Trace::PerformDeferredActions(js::LifoAlloc * alloc, js::irregexp::RegExpMacroAssembler * assembler, int max_register, js::irregexp::OutSet & 

affected_registers, js::irregexp::OutSet * registers_to_pop, js::irregexp::OutSet * registers_to_clear) Line 2617	C++
>	mozjs38-ps-release.dll!js::irregexp::Trace::Flush(js::irregexp::RegExpCompiler * compiler, js::irregexp::RegExpNode * successor) Line 2688	C++
 	[External Code]
Sandarac added inline comments.May 27 2017, 4:46 AM
source/ps/VisualReplay.cpp
201 ↗(On Diff #1646)

This C99 format specifier %zu seems to be what causes the crash on windows (it's not used anywhere else in the code base), likely because of the limited C99 support in VS2013.

elexis added inline comments.May 27 2017, 9:21 AM
source/ps/VisualReplay.cpp
201 ↗(On Diff #1646)

Thanks for figuring that out!

(03:13:03) Sandarac: https://blogs.msdn.microsoft.com/vcblog/2013/07/19/c99-library-support-in-visual-studio-2013/
(03:13:12) Sandarac: "Several format specifiers in the printf family are not yet supported."

Imarok updated this revision to Diff 2251.May 27 2017, 9:24 PM

Fix for windows, by replacing not supported printf format specifier.

elexis accepted this revision.May 27 2017, 9:58 PM
source/ps/VisualReplay.cpp
191 ↗(On Diff #2251)

(unsigned long)fileList.size()

Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/replaymenu/replay_menu.js
| 100| »   »   if·(g_MapNames.indexOf(replay.attribs.settings.Name)·==·-1·&&·replay.attribs.settings.Name·!=·"")
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with ''.

binaries/data/mods/public/gui/replaymenu/replay_menu.js
| 181| »   if·(attribs.settings.PlayerData.length·&&·attribs.settings.PlayerData[0]·==·null)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with 'null'.
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '+'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|1597|1597| 
|1598|1598| 		playerStatistics.economyScore += total + ",";
|1599|1599| 		playerStatistics.militaryScore += Math.round((player.sequences.enemyUnitsKilledValue[maxIndex] +
|1600|    |-			player.sequences.enemyBuildingsDestroyedValue[maxIndex]) / 10)  + ",";
|    |1600|+			player.sequences.enemyBuildingsDestroyedValue[maxIndex]) / 10) + ",";
|1601|1601| 		playerStatistics.totalScore += (total + Math.round((player.sequences.enemyUnitsKilledValue[maxIndex] +
|1602|1602| 			player.sequences.enemyBuildingsDestroyedValue[maxIndex]) / 10)) + ",";
|1603|1603| 

binaries/data/mods/public/gui/session/session.js
| 412| »   »   colorizeHotkey("%(hotkey)s"·+·"·",·"selection.idleworker")·+
|    | [NORMAL] ESLintBear (no-useless-concat):
|    | Unexpected string concatenation of literals.

binaries/data/mods/public/gui/session/session.js
| 432| »   »   let·panelEnt·=·g_PanelEntities.find(panelEnt·=>·panelEnt.slot·!==·undefined·&&·panelEnt.slot·==·slot);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'panelEnt' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
| 443| »   »   let·panelEnt·=·g_PanelEntities.find(panelEnt·=>·panelEnt.slot·!==·undefined·&&·panelEnt.slot·==·slot);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'panelEnt' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
| 957| »   »   let·panelEnt·=·g_PanelEntities.find(panelEnt·=>·ent·==·panelEnt.ent);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'panelEnt' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
| 986| »   let·getPanelEntNameTooltip·=·panelEntState·=>·"[font=\"sans-bold-16\"]"·+·template.name.specific·+·"[/font]";
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'panelEntState' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1062| »   »   button.onpress·=·(function(i)·{·return·function()·{·performGroup((Engine.HotkeyIsPressed("selection.add")·?·"add"·:·"select"),·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1063| »   »   button.ondoublepress·=·(function(i)·{·return·function()·{·performGroup("snap",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1064| »   »   button.onpressright·=·(function(i)·{·return·function()·{·performGroup("breakUp",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
| 829| »   »   i·==·0·||
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/session.js
| 831| »   »   g_GameAttributes.settings.PlayerData[i].AI·!=·"");
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with ''.

binaries/data/mods/public/gui/session/session.js
| 933| »   if·(direction·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/session.js
| 957| »   »   let·panelEnt·=·g_PanelEntities.find(panelEnt·=>·ent·==·panelEnt.ent);
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/session.js
| 981| »   g_PanelEntities·=·g_PanelEntities.sort((panelEntA,·panelEntB)·=>·panelEntIndex(panelEntA.ent)·-·panelEntIndex(panelEntB.ent))
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/gui/session/session.js
|1061| »   »   button.hidden·=·g_Groups.groups[i].getTotalCount()·==·0;
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/session/session.js
|1062| »   »   button.onpress·=·(function(i)·{·return·function()·{·performGroup((Engine.HotkeyIsPressed("selection.add")·?·"add"·:·"select"),·i);·};·})(i);
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/session.js
|1063| »   »   button.ondoublepress·=·(function(i)·{·return·function()·{·performGroup("snap",·i);·};·})(i);
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/session.js
|1064| »   »   button.onpressright·=·(function(i)·{·return·function()·{·performGroup("breakUp",·i);·};·})(i);
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/session.js
|1069| »   »   »   let·icon·=·GetTemplateData(GetEntityState(g_Groups.groups[i].getEntsGrouped().reduce((pre,·cur)·=>·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/session.js
|1123| »   »   if·(player·!=·0·&&
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/gui/session/session.js
|1219| »   »   button.onpress·=·(function(e)·{·return·function()·{·selectAndMoveTo(e);·};·})(researchStarted[tech].researcher);
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/session/session.js
|1407| »   »   if·(+playerID·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.
Executing section XML GUI...
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/replaymenu/replay_menu.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/replaymenu/replay_menu.xml
|   1|   1| <?xml version="1.0" encoding="utf-8"?>
|   2|    |-
|   3|   2| <objects>
|   4|   3| 
|   5|   4| 	<!-- Used to display game info. -->
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/replaymenu/replay_menu.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/replaymenu/replay_menu.xml
|   3|   3| <objects>
|   4|   4| 
|   5|   5| 	<!-- Used to display game info. -->
|   6|    |-	<script file="gui/common/color.js" />
|   7|    |-	<script file="gui/common/functions_civinfo.js" />
|   8|    |-	<script file="gui/common/functions_utility.js" />
|    |   6|+	<script file="gui/common/color.js"/>
|    |   7|+	<script file="gui/common/functions_civinfo.js"/>
|    |   8|+	<script file="gui/common/functions_utility.js"/>
|   9|   9| 	<script file="gui/common/gamedescription.js"/>
|  10|    |-	<script file="gui/common/settings.js" />
|    |  10|+	<script file="gui/common/settings.js"/>
|  11|  11| 
|  12|  12| 	<!-- Used to display message boxes. -->
|  13|  13| 	<script file="gui/common/functions_global_object.js" />
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/replaymenu/replay_menu.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/replaymenu/replay_menu.xml
|  10|

http://jw:8080/job/phabricator_lint/62/ for more details.

This revision was automatically updated to reflect the committed changes.

Also thanks Sandarac for testing the last iterations on windows!

Executing section Default...
Executing section Source...
Executing section JS...
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/65/ for more details.

Build has FAILED

Link to build: http://jw:8080/job/phabricator/1383/
See console output for more information: http://jw:8080/job/phabricator/1383/console

Build has FAILED

Link to build: http://jw:8080/job/phabricator/1386/
See console output for more information: http://jw:8080/job/phabricator/1386/console