Page MenuHomeWildfire Games
Differential D1618

Improve performance of GetEntityState by caching ApplyValueModificationsToEntity.
Needs ReviewPublic
Actions

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

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

Improve performance of GetEntityState by caching ApplyValueModificationsToEntity in the Armour, Attack, Loot and ResourceSupply 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
View All

TimeTest
0 msJenkins > cxxtest_debug.xml::Unknown Unit Message ("")
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::Unknown Unit Message ("")
0 msJenkins > TestAtlasObjectXML::Unknown Unit Message ("")
0 msJenkins > TestAtlasObjectXML::Unknown Unit Message ("")
0 msJenkins > TestAtlasObjectXML::Unknown Unit Message ("")
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

Harbormaster failed remote builds in B6339: Diff 6867!Aug 19 2018, 4:10 PM
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.

Freagarach updated this revision to Diff 9721.Sep 12 2019, 9:46 AM
Freagarach retitled this revision from Improve performance of GetEntityState by caching ApplyValueModificationsToEntity to Improve performance of GetEntityState by caching ApplyValueModificationsToEntity..
Freagarach edited the summary of this revision. (Show Details)
Freagarach added a reviewer: Restricted Owners Package.
Freagarach edited subscribers, added: Freagarach; removed: Vulcan, mimo.

Rebased, extended.

wraitii added a subscriber: wraitii.Sep 12 2019, 9:48 AM

TBH, given our current GUI, I'm wondering if it wouldn't be far more efficient to call this once per template type when we have multiple entities selected, and not care when we have only one entity selected.

Vulcan added a comment.Sep 12 2019, 9:48 AM

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

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

Vulcan added a comment.Sep 12 2019, 9:52 AM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Loot.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Loot.js
|  16|  16| // Cache some properties that are expensive to compute.
|  17|  17| Loot.prototype.UpdateProperties = function()
|  18|  18| {
|  19|    |-	this.xp = Math.floor(ApplyValueModificationsToEntity("Loot/xp", +(this.template.xp || 0), this.entity))
|    |  19|+	this.xp = Math.floor(ApplyValueModificationsToEntity("Loot/xp", +(this.template.xp || 0), this.entity));
|  20|  20| 
|  21|  21| 	this.resources = {};
|  22|  22| 	for (let res of Resources.GetCodes())

binaries/data/mods/public/simulation/components/Loot.js
|  19| »   this.xp·=·Math.floor(ApplyValueModificationsToEntity("Loot/xp",·+(this.template.xp·||·0),·this.entity))
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
Executing section cli...

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

Harbormaster completed remote builds in B9328: Diff 9721.Sep 12 2019, 9:53 AM
Stan added a subscriber: Stan.Sep 12 2019, 10:08 AM
In D1618#94941, @wraitii wrote:

TBH, given our current GUI, I'm wondering if it wouldn't be far more efficient to call this once per template type when we have multiple entities selected, and not care when we have only one entity selected.

Why not both ?

wraitii added a comment.Sep 12 2019, 10:16 AM
In D1618#94946, @Stan wrote:
In D1618#94941, @wraitii wrote:

TBH, given our current GUI, I'm wondering if it wouldn't be far more efficient to call this once per template type when we have multiple entities selected, and not care when we have only one entity selected.

Why not both ?

Caching values is annoying, because you have to make sure to invalidate the cache at the correct time, and cache it also at the correct time. See the complexity of Attack.prototype.UpdateProperties for example.
Further, with caching you pay the cost of caching whenever you cache, which can actually be more often than you would call the function if you didn't cache at all, so you could be making some bad case better but the general game slower, or improve some cases but worsen others.

In particular, I'm wary of entity creation. It already takes a fair few µs per entity, and components that cache on unit creation are a big part of that.

Anyways, what I'm saying is that caching should be a last resort of sort, and I think other avenues should be explored before, in order:

  1. Optimising existing calls so that this is no longer a problem
  2. Changing algorithms so that this is no longer a problem
  3. ???
  4. Caching
ffffffff added a subscriber: ffffffff.Sep 12 2019, 12:39 PM

op!

Freagarach added a comment.EditedSep 12 2019, 4:53 PM
In D1618#94947, @wraitii wrote:

In particular, I'm wary of entity creation. It already takes a fair few µs per entity, and components that cache on unit creation are a big part of that.

I thought about that as well, thats why I first asked on IRC (should have tagged you).

In D1618#94941, @wraitii wrote:

TBH, given our current GUI, I'm wondering if it wouldn't be far more efficient to call this once per template type when we have multiple entities selected, and not care when we have only one entity selected.

I think I have part of a patch for that lying around on Phab. somewhere.

[EDIT}

In D2140#89975, @Freagarach wrote:

Created a variable for unique templates in a selection, thus reducing the call time of g_SelectionPanels.PrefAttack.GetItems. This function can probably used for more functions, but that's out of scope.

[EDIT2] D2301

Revision Contents
Changeset List

PathSize
binaries/
data/
mods/
public/
simulation/
components/
46 lines
144 lines
26 lines
49 lines

Diff 6867

View Options

binaries/data/mods/public/simulation/components/Armour.js

Loading...
View Options

binaries/data/mods/public/simulation/components/Attack.js

Loading...
View Options

binaries/data/mods/public/simulation/components/GuiInterface.js

Loading...
View Options

binaries/data/mods/public/simulation/components/Loot.js

Loading...
Wildfire Games · Phabricator