Page MenuHomeWildfire Games

Improve performance of GetEntityState by caching ApplyValueModificationsToEntity
Changes PlannedPublic

Authored by echotangoecho on Aug 19 2018, 4:05 PM.

Details

Reviewers
None
Summary

Improve performance of GetEntityState by caching ApplyValueModificationsToEntity in the Armour, Attack and Loot components, as originally suggested by @mimo on D1331.
On Combat Demo (Huge) this gives (on my hardware) a ~30% decrease in time consumption by GetEntityState.

Test Plan

Verify that functionally nothing changed, and test that the game still works well (especially in cases like tech upgrades, while units are selected).

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
selections
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6339
Build 10515: Vulcan BuildJenkins
Build 10514: arc lint + arc unit

Unit TestsFailed

TimeTest
0 msJenkins > cxxtest_debug.xml::[failed-to-read]
Failed to read test report file /mnt/data/jenkins-phabricator/workspace/differential/cxxtest_debug.xml org.dom4j.DocumentException: Error on line 1 of document : Content is not allowed in prolog. Nested exception: Content is not allowed in prolog. at org.dom4j.io.SAXReader.read(SAXReader.java:482)
0 msJenkins > TestAllocators::test_da
0 msJenkins > TestAtlasObjectXML::test_parse_attributes1
0 msJenkins > TestAtlasObjectXML::test_parse_attributes2
0 msJenkins > TestAtlasObjectXML::test_parse_basic
View Full Test Results (1 Failed · 312 Passed)

Event Timeline

echotangoecho created this revision.Aug 19 2018, 4:05 PM
Vulcan added a subscriber: Vulcan.Aug 19 2018, 4:10 PM

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

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Loot.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Loot.js
|  11|  11| Loot.prototype.Init = function()
|  12|  12| {
|  13|  13| 	this.UpdateProperties();
|  14|    |-}
|    |  14|+};
|  15|  15| 
|  16|  16| Loot.prototype.Serialize = null; // we have no dynamic state to save
|  17|  17| 
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Loot.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Loot.js
|  18|  18| // Cache some properties that are expensive to compute.
|  19|  19| Loot.prototype.UpdateProperties = function()
|  20|  20| {
|  21|    |-	this.xp = Math.floor(ApplyValueModificationsToEntity("Loot/xp", +(this.template.xp || 0), this.entity))
|    |  21|+	this.xp = Math.floor(ApplyValueModificationsToEntity("Loot/xp", +(this.template.xp || 0), this.entity));
|  22|  22| 
|  23|  23| 	this.resources = {};
|  24|  24| 	for (let res of Resources.GetCodes())

binaries/data/mods/public/simulation/components/Loot.js
|  14| }
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/simulation/components/Loot.js
|  21| »   this.xp·=·Math.floor(ApplyValueModificationsToEntity("Loot/xp",·+(this.template.xp·||·0),·this.entity))
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Armour.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Armour.js
|  80|  80| 	this.armourStrengths = {};
|  81|  81| 	for (let damageType of DamageTypes.GetTypes())
|  82|  82| 		this.armourStrengths[damageType] = applyMods(damageType, foundation);
|  83|    |-}
|    |  83|+};
|  84|  84| 
|  85|  85| Armour.prototype.GetArmourStrengths = function()
|  86|  86| {

binaries/data/mods/public/simulation/components/Armour.js
|  83| }
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 484| 484| 	}
| 485| 485| 
| 486| 486| 	this.properties = attack;
| 487|    |-}
|    | 487|+};
| 488| 488| 
| 489| 489| Attack.prototype.GetTimers = function(type)
| 490| 490| {
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 518| 518| Attack.prototype.GetProperties = function()
| 519| 519| {
| 520| 520| 	return this.properties;
| 521|    |-}
|    | 521|+};
| 522| 522| 
| 523| 523| Attack.prototype.GetBonusTemplate = function(type)
| 524| 524| {
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 550| 550| 
| 551| 551| 		let horizSpeed = +this.template[type].ProjectileSpeed;
| 552| 552| 		let gravity = +this.template[type].Gravity;
| 553|    |-		//horizSpeed /= 2; gravity /= 2; // slow it down for testing
|    | 553|+		// horizSpeed /= 2; gravity /= 2; // slow it down for testing
| 554| 554| 
| 555| 555| 		let cmpPosition = Engine.QueryInterface(this.entity, IID_Position);
| 556| 556| 		if (!cmpPosition || !cmpPosition.IsInWorld())
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'else'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 670| 670| 			});
| 671| 671| 	}
| 672| 672| 	else
| 673|    |-	{
|    | 673|+	
| 674| 674| 		// Melee attack - hurt the target immediately
| 675| 675| 		cmpDamage.CauseDamage({
| 676| 676| 			"strengths": this.GetAttackStrengths(type),
| 680| 680| 			"type": type,
| 681| 681| 			"attackerOwner": attackerOwner
| 682| 682| 		});
| 683|    |-	}
|    | 683|+	
| 684| 684| };
| 685| 685| 
| 686| 686| /**

binaries/data/mods/public/simulation/components/Attack.js
| 540| ·»   let·cmpDamage·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_Damage);
|    | [NORMAL] ESLintBear (no-mixed-spaces-and-tabs):
|    | Mixed spaces and tabs.

binaries/data/mods/public/simulation/components/Attack.js
| 487| }
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/simulation/components/Attack.js
| 521| }
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/simulation/components/Attack.js
| 645| »   »   cmpTimer.SetTimeout(SYSTEM_ENTITY,·IID_Damage,·"MissileHit",·timeToTarget·*·1000·+·+this.template.Ranged.Delay,·data);
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|  66|  66| 		let phase = "";
|  67|  67| 		let cmpTechnologyManager = QueryPlayerIDInterface(i, IID_TechnologyManager);
|  68|  68| 		if (cmpTechnologyManager)
|  69|    |-		{
|    |  69|+		
|  70|  70| 			if (cmpTechnologyManager.IsTechnologyResearched("phase_city"))
|  71|  71| 				phase = "city";
|  72|  72| 			else if (cmpTechnologyManager.IsTechnologyResearched("phase_town"))
|  73|  73| 				phase = "town";
|  74|  74| 			else if (cmpTechnologyManager.IsTechnologyResearched("phase_village"))
|  75|  75| 				phase = "village";
|  76|    |-		}
|    |  76|+		
|  77|  77| 
|  78|  78| 		// store player ally/neutral/enemy data as arrays
|  79|  79| 		let allies = [];
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
| 394| 394| 			let max = ret.attack[type].maxRange;
| 395| 395| 
| 396| 396| 			if (cmpUnitAI && cmpPosition && cmpPosition.IsInWorld())
| 397|    |-			{
|    | 397|+			
| 398| 398| 				// For units, take the range in front of it, no spread. So angle = 0
| 399| 399| 				ret.attack[type].elevationAdaptedRange = cmpRangeManager.GetElevationAdaptedRange(cmpPosition.GetPosition(), cmpPosition.GetRotation(), max, ret.attack[type].elevationBonus, 0);
| 400|    |-			}
|    | 400|+			
| 401| 401| 			else if(cmpPosition && cmpPosition.IsInWorld())
| 402| 402| 			{
| 403| 403| 				// For buildings, take the average elevation around it. So angle = 2*pi
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
| 399| 399| 				ret.attack[type].elevationAdaptedRange = cmpRangeManager.GetElevationAdaptedRange(cmpPosition.GetPosition(), cmpPosition.GetRotation(), max, ret.attack[type].elevationBonus, 0);
| 400| 400| 			}
| 401| 401| 			else if(cmpPosition && cmpPosition.IsInWorld())
| 402|    |-			{
|    | 402|+			
| 403| 403| 				// For buildings, take the average elevation around it. So angle = 2*pi
| 404| 404| 				ret.attack[type].elevationAdaptedRange = cmpRangeManager.GetElevationAdaptedRange(cmpPosition.GetPosition(), cmpPosition.GetRotation(), max, ret.attack[type].elevationBonus, 2*Math.PI);
| 405|    |-			}
|    | 405|+			
| 406| 406| 			else
| 407| 407| 			{
| 408| 408| 				// not in world, set a default?
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'else'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
| 404| 404| 				ret.attack[type].elevationAdaptedRange = cmpRangeManager.GetElevationAdaptedRange(cmpPosition.GetPosition(), cmpPosition.GetRotation(), max, ret.attack[type].elevationBonus, 2*Math.PI);
| 405| 405| 			}
| 406| 406| 			else
| 407|    |-			{
|    | 407|+			
| 408| 408| 				// not in world, set a default?
| 409| 409| 				ret.attack[type].elevationAdaptedRange = ret.attack[type].maxRange;
| 410|    |-			}
|    | 410|+			
| 411| 411| 		}
| 412| 412| 	}
| 413| 413| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
| 775| 775| 		updateEntityColor(data.showAllStatusBars && (i == player || player == -1) ?
| 776| 776| 			[IID_Minimap, IID_RangeOverlayRenderer, IID_RallyPointRenderer, IID_StatusBars] :
| 777| 777| 			[IID_Minimap, IID_RangeOverlayRenderer, IID_RallyPointRenderer],
| 778|    |-			cmpRangeManager.GetEntitiesByPlayer(i));
|    | 778|+		cmpRangeManager.GetEntitiesByPlayer(i));
| 779| 779| 	}
| 780| 780| 	updateEntityColor([IID_Selectable, IID_StatusBars], data.selected);
| 781| 781| 	Engine.QueryInterface(SYSTEM_ENTITY, IID_TerritoryManager).UpdateColors();
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'else'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|1294|1294| 		}
|1295|1295| 	}
|1296|1296| 	else
|1297|    |-	{
|    |1297|+	
|1298|1298| 		// Didn't snap to an existing entity, add the starting tower manually. To prevent odd-looking rotation jumps
|1299|1299| 		// when shift-clicking to build a wall, reuse the placement angle that was last seen on a validly positioned
|1300|1300| 		// wall piece.
|1315|1315| 			"pos": start.pos,
|1316|1316| 			"angle": previewEntities.length > 0 ? previewEntities[0].angle : this.placementWallLastAngle
|1317|1317| 		});
|1318|    |-	}
|    |1318|+	
|1319|1319| 
|1320|1320| 	if (end.pos)
|1321|1321| 	{
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|1318|1318| 	}
|1319|1319| 
|1320|1320| 	if (end.pos)
|1321|    |-	{
|    |1321|+	
|1322|1322| 		// Analogous to the starting side case above
|1323|1323| 		if (end.snappedEnt && end.snappedEnt != INVALID_ENTITY)
|1324|1324| 		{
|1356|1356| 				"pos": end.pos,
|1357|1357| 				"angle": previewEntities.length > 0 ? previewEntities[previewEntities.length-1].angle : this.placementWallLastAngle
|1358|1358| 			});
|1359|    |-	}
|    |1359|+	
|1360|1360| 
|1361|1361| 	let cmpTerrain = Engine.QueryInterface(SYSTEM_ENTITY, IID_Terrain);
|1362|1362| 	if (!cmpTerrain)
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|1534|1534| 
|1535|1535| 		let cmpVisual = Engine.QueryInterface(ent, IID_Visual);
|1536|1536| 		if (cmpVisual)
|1537|    |-		{
|    |1537|+		
|1538|1538| 			if (!allPiecesValid || !canAfford)
|1539|1539| 				cmpVisual.SetShadingColor(1.4, 0.4, 0.4, 1);
|1540|1540| 			else
|1541|1541| 				cmpVisual.SetShadingColor(1, 1, 1, 1);
|1542|    |-		}
|    |1542|+		
|1543|1543| 
|1544|1544| 		++entPool.numUsed;
|1545|1545| 	}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 6.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|1608|1608| 			{
|1609|1609| 				minDist2 = dist2;
|1610|1610| 				minDistEntitySnapData = {
|1611|    |-						"x": pos.x,
|    |1611|+					"x": pos.x,
|1612|1612| 						"z": pos.z,
|1613|1613| 						"angle": cmpPosition.GetRotation().y,
|1614|1614| 						"ent": ent
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 6.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|1609|1609| 				minDist2 = dist2;
|1610|1610| 				minDistEntitySnapData = {
|1611|1611| 						"x": pos.x,
|1612|    |-						"z": pos.z,
|    |1612|+					"z": pos.z,
|1613|1613| 						"angle": cmpPosition.GetRotation().y,
|1614|1614| 						"ent": ent
|1615|1615| 				};
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 6.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|1610|1610| 				minDistEntitySnapData = {
|1611|1611| 						"x": pos.x,
|1612|1612| 						"z": pos.z,
|1613|    |-						"angle": cmpPosition.GetRotation().y,
|    |1613|+					"angle": cmpPosition.GetRotation().y,
|1614|1614| 						"ent": ent
|1615|1615| 				};
|1616|1616| 			}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 6.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|1611|1611| 						"x": pos.x,
|1612|1612| 						"z": pos.z,
|1613|1613| 						"angle": cmpPosition.GetRotation().y,
|1614|    |-						"ent": ent
|    |1614|+					"ent": ent
|1615|1615| 				};
|1616|1616| 			}
|1617|1617| 		}
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|1756|1756| 			result.gain = cmpEntityTrader.GetGoods().amount;
|1757|1757| 	}
|1758|1758| 	else if (data.target === secondMarket)
|1759|    |-	{
|    |1759|+	
|1760|1760| 		result = {
|1761|1761| 			"type": "is second",
|1762|1762| 			"gain": cmpEntityTrader.GetGoods().amount,
|1763|1763| 		};
|1764|    |-	}
|    |1764|+	
|1765|1765| 	else if (!firstMarket)
|1766|1766| 	{
|1767|1767| 		result = { "type": "set first" };
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|1763|1763| 		};
|1764|1764| 	}
|1765|1765| 	else if (!firstMarket)
|1766|    |-	{
|    |1766|+	
|1767|1767| 		result = { "type": "set first" };
|1768|    |-	}
|    |1768|+	
|1769|1769| 	else if (!secondMarket)
|1770|1770| 	{
|1771|1771| 		result = {
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|1767|1767| 		result = { "type": "set first" };
|1768|1768| 	}
|1769|1769| 	else if (!secondMarket)
|1770|    |-	{
|    |1770|+	
|1771|1771| 		result = {
|1772|1772| 			"type": "set second",
|1773|1773| 			"gain": cmpEntityTrader.CalculateGain(firstMarket, data.target),
|1774|1774| 		};
|1775|    |-	}
|    |1775|+	
|1776|1776| 	else
|1777|1777| 	{
|1778|1778| 		// Else both markets are not null and target is different from them
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'else'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|1774|1774| 		};
|1775|1775| 	}
|1776|1776| 	else
|1777|    |-	{
|    |1777|+	
|1778|1778| 		// Else both markets are not null and target is different from them
|1779|1779| 		result = { "type": "set first" };
|1780|    |-	}
|    |1780|+	
|1781|1781| 	return result;
|1782|1782| };
|1783|1783|

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

echotangoecho planned changes to this revision.Aug 19 2018, 4:11 PM

Hmm, so apparently parts don't really work as it is now. Will investigate.