Rewrite DeveloperOverlay and TimeWarp to use class syntax and split classes
ClosedPublic
Actions

Authored by elexis on Oct 19 2019, 1:33 AM.

Details

Reviewers: None
Commits: rP23081: Rewrite developer overlay to use class syntax, one class per checkbox, a class…

Summary

rP18399 moved the developer overlay from session.xml to developer_overlay.xml.
In rP22370/D1928, the developer overlay was refactored to use prototype syntax, remove XML duplication and move definitions to JS.

This patch revisits that commit in the course of #5387, addresses the following points:

The checkbox definition is a function getCommands that returns an array of objects where each object returns a checkbox. This approach is problematic in multiple ways. (A) Mods can't extend the array if its the result of a function! (B) Objects are limited to the few hardcoded functions. Class instances can add custom functions, subscribe them to events refs rP23076/D2378. (C) It doesn't provide a this reference, so it is impossible to store a state variable. By using an actual class instance per checkbox, we can store class helper variables per checkbox, such as the TimeWarp class or EntityState class.

The benefit of using classes is to increase cohesion and separation of concerns. Actually take this benefit by splitting things that are logically unrelated. That is
1. the EntityState panel that shows the simstate of the selected entity. That has nothing to do with the checkboxes and shouldn't dilute the class concerned with that.
2. TimeWarp mode is only relevant for one of the option, but the class in rP22370/D1928 now needs to be aware of that since its not split.
3. Splitting the class to handle one checkbox from the class to manage the panel. This allows making the checkbox states more simple, extensible and moddable.

Filenames match their classname.
Code uses class keyword.
The TimeWarp code is moved from input.js from rP8803 to the TimeWarp setting using the hotkey release event from rP19028, refs #3194.
Removes the global references to g_DeveloperOverlay and getters by accessing the players simstate.
Moves EntityState to own XML file.

New classes:

class DeveloperOverlay
class DeveloperOverlayCheckbox
class DeveloperOverlayCheckboxes
class DeveloperOverlayEntityState
class TimeWarp
DeveloperOverlayCheckboxes.prototype.ControlAll = class
DeveloperOverlayCheckboxes.prototype.ChangePerspective = class
DeveloperOverlayCheckboxes.prototype.SelectionEntityState = class
DeveloperOverlayCheckboxes.prototype.PathfinderOverlay = class
DeveloperOverlayCheckboxes.prototype.HierarchicalPathfinderOverlay = class
DeveloperOverlayCheckboxes.prototype.ObstructionOverlay = class
DeveloperOverlayCheckboxes.prototype.UnitMotionOverlay = class
DeveloperOverlayCheckboxes.prototype.RangeOverlay = class
DeveloperOverlayCheckboxes.prototype.BoundingBoxOverlay = class
DeveloperOverlayCheckboxes.prototype.RestrictCamera = class
DeveloperOverlayCheckboxes.prototype.RevealMap = class
DeveloperOverlayCheckboxes.prototype.EnableTimeWarp = class
DeveloperOverlayCheckboxes.prototype.PromoteSelectedUnits = class
DeveloperOverlayCheckboxes.prototype.EnableCulling = class
DeveloperOverlayCheckboxes.prototype.LockCullCamera = class
DeveloperOverlayCheckboxes.prototype.DisplayCameraFrustum = class

A performance optimization added in this patch is to extend the system from 23076/D2378 to support unsubscription from events.
This means we can save about 200 microsecnds each turn if the developer overlay is closed by unscubsribing from onSimulationUpdate if its closed!
(That's signficant, because the turn length may be 100ms eventually, so a no-op shouldnt consume 0.2% of each turn length.)

Measurement method:

diff --git a/binaries/data/mods/public/gui/session/developer_overlay.js b/binaries/data/mods/public/gui/session/developer_overlay.js
index 57e9736fd8..6b1329ae93 100644
--- a/binaries/data/mods/public/gui/session/developer_overlay.js
+++ b/binaries/data/mods/public/gui/session/developer_overlay.js
@@ -210,11 +210,13 @@ DeveloperOverlay.prototype.toggle = function()
 
 DeveloperOverlay.prototype.update = function()
 {
+       let foo = Engine.GetMicroseconds();
        let playerState = g_SimState.players[g_ViewedPlayer];
        this.controlAll = playerState ? playerState.controlsAll : false;
 
        this.updateValues();
        this.updateEntityState();
+       warn(Engine.GetMicroseconds() - foo);
 }
 
 DeveloperOverlay.prototype.updateEntityState = function()

Measuremnet result (the 600microsecond numbers occur when showing the entity selection state):

WARNING: 376
WARNING: 211
WARNING: 205
WARNING: 201
WARNING: 202
WARNING: 212
WARNING: 228
WARNING: 265
WARNING: 207
WARNING: 216
WARNING: 664
WARNING: 175
WARNING: 162
WARNING: 170
WARNING: 181
WARNING: 186
WARNING: 153
WARNING: 158
WARNING: 186
WARNING: 540
WARNING: 291
WARNING: 150
WARNING: 150
WARNING: 153
WARNING: 145
WARNING: 161
WARNING: 386
WARNING: 179
WARNING: 158
WARNING: 141
WARNING: 146
WARNING: 261
WARNING: 136
WARNING: 132
WARNING: 130
WARNING: 138
WARNING: 134
WARNING: 129
WARNING: 141
WARNING: 139
WARNING: 145
WARNING: 280
WARNING: 139
WARNING: 132
WARNING: 145
WARNING: 152
WARNING: 141
WARNING: 215
WARNING: 139
WARNING: 161
WARNING: 139
WARNING: 151
WARNING: 169
WARNING: 138
WARNING: 133
WARNING: 129
WARNING: 140
WARNING: 141
WARNING: 125
WARNING: 137
WARNING: 111
WARNING: 133
WARNING: 132
WARNING: 168
WARNING: 146
WARNING: 135
WARNING: 124
WARNING: 125
WARNING: 125
WARNING: 114
WARNING: 141
WARNING: 126
WARNING: 128
WARNING: 173
WARNING: 126
WARNING: 110
WARNING: 177
WARNING: 151
WARNING: 130
WARNING: 159
WARNING: 148
WARNING: 130
WARNING: 168
WARNING: 146
WARNING: 154
WARNING: 135
WARNING: 196
WARNING: 130
WARNING: 136
WARNING: 165
WARNING: 138
WARNING: 141
WARNING: 137
WARNING: 139
WARNING: 146
WARNING: 131
WARNING: 163
WARNING: 145
WARNING: 131
WARNING: 130
WARNING: 142
WARNING: 156
WARNING: 161
WARNING: 150
WARNING: 138
WARNING: 141
WARNING: 131
WARNING: 156
WARNING: 138
WARNING: 133
WARNING: 129
WARNING: 130
WARNING: 132
WARNING: 135
WARNING: 145
WARNING: 881
WARNING: 645
WARNING: 629
WARNING: 652
WARNING: 641
WARNING: 657
WARNING: 696
WARNING: 628
WARNING: 609
WARNING: 632
WARNING: 641
WARNING: 618
WARNING: 638
WARNING: 628
WARNING: 618
WARNING: 787
WARNING: 638
WARNING: 612
WARNING: 622
WARNING: 889
WARNING: 622
WARNING: 642
WARNING: 661
WARNING: 644
WARNING: 928
WARNING: 854
WARNING: 958
WARNING: 845
WARNING: 829
WARNING: 869
WARNING: 858
WARNING: 823
WARNING: 832
WARNING: 921
WARNING: 1031
WARNING: 844
WARNING: 696
WARNING: 640
WARNING: 751
WARNING: 842
WARNING: 662
WARNING: 672
WARNING: 652
WARNING: 670
WARNING: 653
WARNING: 636
WARNING: 897
WARNING: 212
WARNING: 143
WARNING: 140
WARNING: 139
WARNING: 132
WARNING: 141
WARNING: 151
WARNING: 159
WARNING: 208
WARNING: 137

At last, the patch changes the developer overlay to be openable in case of loading a multiplayer savegame that was played without cheats and replays in observermode before saving, as discovered in the course of F1100948 / #5624, see rP19558, rP19651.

Test Plan

Test that every developer overlay option still works as before.
To test the performance optimization, add a warn to the conditional update functions (ensuring that the function is not called anymore when unsubscribed).
Test that Set iteration and array iteration is either faster or negligible in difference.
To test the selection.js diff, select units of multiple owners in observermode, in singleplayer, and select an enemy unit entering fog of war.
To test warp mode, use the hotkeys.

Diff Detail

Repository

rP 0 A.D. Public Repository

Branch

/ps/trunk

Lint

Lint OK

Unit

No Unit Test Coverage

Build Status

Buildable 9841
Build 16581: Vulcan Build	Jenkins
Build 16580: Vulcan Build (Windows)	Jenkins
Build 16579: arc lint + arc unit

Event Timeline

elexis created this revision.Oct 19 2019, 1:33 AM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/987/display/redirect

Harbormaster failed remote builds in B9841: Diff 10178!Oct 19 2019, 1:33 AM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/472/display/redirect

For the Set vs Array difference, iteration performance is what matters most.

I did the following dirty test:

Index: /home/elexis/code/0ad-svn/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js
===================================================================
--- /home/elexis/code/0ad-svn/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js	(revision 23080)
+++ /home/elexis/code/0ad-svn/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js	(working copy)
@@ -12,6 +12,31 @@
 
 var g_GameSpeeds = getGameSpeedChoices(false);
 
+var g_Foo1 = new Set();
+var g_Foo2 = [];
+
+for (let i = 0; i < 2000; ++i)
+{
+	g_Foo1.add(i);
+	g_Foo2.push(i);
+}
+
+for (let f = 0; f < 20; ++f)
+{
+	{
+		let start = Engine.GetMicroseconds();
+		for (let i of g_Foo1)
+			i;
+		warn("Set: " + (Engine.GetMicroseconds() - start));
+	}
+	{
+		let start = Engine.GetMicroseconds();
+		for (let i of g_Foo2)
+			i;
+		warn("Array: " + (Engine.GetMicroseconds() - start));
+	}
+}
+
 /**
  * Offer users to select playable civs only.
  * Load unselectable civs as they could appear in scenario maps.

The results were odd:

WARNING: Set: 1144
WARNING: Array: 1620
WARNING: Set: 1212
WARNING: Array: 692
WARNING: Set: 1226
WARNING: Array: 191
WARNING: Set: 1330
WARNING: Array: 112
WARNING: Set: 1115
WARNING: Array: 113
WARNING: Set: 1121
WARNING: Array: 107
WARNING: Set: 1082
WARNING: Array: 107
WARNING: Set: 1173
WARNING: Array: 110
WARNING: Set: 1100
WARNING: Array: 109
WARNING: Set: 1100
WARNING: Array: 215
WARNING: Set: 1125
WARNING: Array: 147
WARNING: Set: 1107
WARNING: Array: 107
WARNING: Set: 1095
WARNING: Array: 120
WARNING: Set: 1107
WARNING: Array: 109
WARNING: Set: 1138
WARNING: Array: 104
WARNING: Set: 1119
WARNING: Array: 106
WARNING: Set: 1087
WARNING: Array: 108
WARNING: Set: 1095
WARNING: Array: 108
WARNING: Set: 1148
WARNING: Array: 104
WARNING: Set: 1104
WARNING: Array: 106

Notice for the first call Set and Array have the same performance.
In the second call Array is twice as fast.
For the third and following, 10x as fast.
So it seems like the JIT would know how to optimize the Array but not the Set.

However we can assume that we will have something like 20-50 update functions in onSimulationUpdate, not 2000.

For containers of this size, Set somehow seems to be much faster!?

Index: binaries/data/mods/public/gui/gamesetup/gamesetup.js
===================================================================
--- binaries/data/mods/public/gui/gamesetup/gamesetup.js	(revision 23080)
+++ binaries/data/mods/public/gui/gamesetup/gamesetup.js	(working copy)
@@ -12,6 +12,36 @@
 
 var g_GameSpeeds = getGameSpeedChoices(false);
 
+function goodbyeworld()
+{
+	1 == 2;
+}
+
+var g_Foo1 = new Set();
+var g_Foo2 = [];
+
+for (let i = 0; i < 50; ++i)
+{
+	g_Foo1.add(goodbyeworld.bind(global));
+	g_Foo2.push(goodbyeworld.bind(global));
+}
+
+for (let f = 0; f < 20; ++f)
+{
+	{
+		let start = Engine.GetMicroseconds();
+		for (let i of g_Foo1)
+			i();
+		warn("Set: " + (Engine.GetMicroseconds() - start));
+	}
+	{
+		let start = Engine.GetMicroseconds();
+		for (let i of g_Foo2)
+			i();
+		warn("Array: " + (Engine.GetMicroseconds() - start));
+	}
+}
+
 /**
  * Offer users to select playable civs only.
  * Load unselectable civs as they could appear in scenario maps.

WARNING: Set: 140
WARNING: Array: 222
WARNING: Set: 69
WARNING: Array: 63
WARNING: Set: 65
WARNING: Array: 65
WARNING: Set: 64
WARNING: Array: 59
WARNING: Set: 64
WARNING: Array: 59
WARNING: Set: 64
WARNING: Array: 63
WARNING: Set: 75
WARNING: Array: 59
WARNING: Set: 64
WARNING: Array: 63
WARNING: Set: 64
WARNING: Array: 65
WARNING: Set: 64
WARNING: Array: 176
WARNING: Set: 67
WARNING: Array: 69
WARNING: Set: 62
WARNING: Array: 57
WARNING: Set: 60
WARNING: Array: 54
WARNING: Set: 60
WARNING: Array: 53
WARNING: Set: 60
WARNING: Array: 54
WARNING: Set: 60
WARNING: Array: 53
WARNING: Set: 60
WARNING: Array: 53
WARNING: Set: 61
WARNING: Array: 54
WARNING: Set: 60
WARNING: Array: 53
WARNING: Set: 59
WARNING: Array: 141

So at least for the expected container size, the performance difference is not observable and we can chose the container that has the non-ugly remover.

elexis edited the summary of this revision. (Show Details)Oct 19 2019, 1:58 AM

elexis edited the summary of this revision. (Show Details)Oct 19 2019, 2:09 AM

This revision was not accepted when it landed; it landed in state Needs Review.Oct 19 2019, 2:27 AM

This revision was landed with ongoing or failed builds.

Closed by commit rP23081: Rewrite developer overlay to use class syntax, one class per checkbox, a class… (authored by elexis). · Explain Why

This revision was automatically updated to reflect the committed changes.

elexis added a commit: rP23081: Rewrite developer overlay to use class syntax, one class per checkbox, a class….

Owners added a subscriber: Restricted Owners Package.Oct 19 2019, 2:27 AM

elexis mentioned this in rP23098: Fix wrong playerID used for the controlAll developer overlay option in….Oct 27 2019, 3:15 PM