Paths

Table of Contentst

Differential D2301

Add function to return uniquely selected templates.
AbandonedPublic
Actions

Authored by Freagarach on Sep 17 2019, 6:42 PM.

Details

Reviewers

None

Group Reviewers

Restricted Owners Package

(Owns No Changed Paths)

Summary

This function return templates of which there are one or more selected, thus reducing the call time when replacing getEntityState.
Some preliminary tests show that selectedUniqueTemplates.some(...) is about a third of the call time (4 us) with respect to entityStates.some(...) (14 us) with 100 identical units selected.
I am not fully convinced this works well enough, because caching templates has its limitations (aura's, upgrades and such) and caching entity states as well (all states are different, when is the state different enough?).

ToDo:

Implement, find more use cases.

Test Plan

Verify this function is correct and fast enough.
Discuss whether this is the way to go.

Diff Detail

Repository

rP 0 A.D. Public Repository

Branch

/ps/trunk

Lint

Lint OK

Unit

No Unit Test Coverage

Build Status

Buildable 9502
Build 15863: Vulcan Build	Jenkins
Build 15862: Vulcan Build (Windows)	Jenkins
Build 15861: arc lint + arc unit

Event Timeline

Freagarach created this revision.Sep 17 2019, 6:42 PM

Freagarach created this object with visibility "No One".

Owners added a subscriber: Restricted Owners Package.Sep 17 2019, 6:42 PM

Freagarach mentioned this in D1618: Improve performance of GetEntityState by caching ApplyValueModificationsToEntity..Sep 20 2019, 3:33 PM

Noted some use cases.

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

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

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
|  39|  39| 		return 2;
|  40|  40| 	},
|  41|  41| 	"getItems": function(unitEntStates)
|  42|    |-	{//Could be updated?
|    |  42|+	{// Could be updated?
|  43|  43| 		return unitEntStates.some(state => !!state.alertRaiser) ? ["raise", "end"] : [];
|  44|  44| 	},
|  45|  45| 	"setupButton": function(data)
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
|  82|  82| 	"rowLength": 4,
|  83|  83| 	"conflictsWith": ["Garrison"],
|  84|  84| 	"getItems": function(unitEntStates)
|  85|    |-	{//Could be updated?
|    |  85|+	{// Could be updated?
|  86|  86| 		// If more than `rowLength` resources, don't display icons.
|  87|  87| 		if (unitEntStates.every(state => !state.isBarterMarket) || g_ResourceData.GetCodes().length > this.rowLength)
|  88|  88| 			return [];
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
| 287| 287| 	"rowLength": 4,
| 288| 288| 	"conflictsWith": ["Garrison"],
| 289| 289| 	"getItems": function(unitEntStates)
| 290|    |-	{//Could be updated?
|    | 290|+	{// Could be updated?
| 291| 291| 		if (unitEntStates.some(state => !hasClass(state, "Unit")))
| 292| 292| 			return [];
| 293| 293| 
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
| 342| 342| 	"rowLength": 4,
| 343| 343| 	"conflictsWith": ["Barter"],
| 344| 344| 	"getItems": function(unitEntStates)
| 345|    |-	{//Could be updated?
|    | 345|+	{// Could be updated?
| 346| 346| 		if (unitEntStates.every(state => !state.garrisonHolder))
| 347| 347| 			return [];
| 348| 348| 
|    | [NORMAL] ESLintBear (space-before-function-paren):
|    | Unexpected space before function parentheses.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
| 763| 763| 				addResearchToQueue(data.item.researchFacilityId, t);
| 764| 764| 			})(tech);
| 765| 765| 
| 766|    |-			button.onPressRight = (t => function () {
|    | 766|+			button.onPressRight = (t => function() {
| 767| 767| 				showTemplateDetails(
| 768| 768| 					t,
| 769| 769| 					GetTemplateData(data.unitEntStates.find(state => state.id == data.item.researchFacilityId).template).nativeCiv);
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
| 895| 895| 		return 5;
| 896| 896| 	},
| 897| 897| 	"getItems": function(unitEntStates)
| 898|    |-	{//Could be updated?
|    | 898|+	{// Could be updated?
| 899| 899| 		if (unitEntStates.some(state => !state.unitAI || !hasClass(state, "Unit") || hasClass(state, "Animal")))
| 900| 900| 			return [];
| 901| 901| 

binaries/data/mods/public/gui/session/selection_panels.js
|  48| »   »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
|  59| »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
| 731| »   »   »   »   »   »   switch·(entity.check)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.
Executing section cli...

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

Harbormaster completed remote builds in B9502: Diff 9882.Sep 20 2019, 4:14 PM

Good idea at first, but Im not sure if it will be a big performance boost with the current code and the loops mentioned in the inserted code comments applied,
because the entity states are all obtained prior to the function call ("getItems": function(unitEntStates)).
The greater performance boost would come if we dont need to know all entity states anymore when selecting the units, or otherwise obtaining less information (only the one that is needed).

binaries/data/mods/public/gui/session/selection.js
217	JS Set type has a contains and insert method that fails if it already exists. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set I heard claims it would be faster too, I don't know if its true. I dont object to using good old array.
221	`GetEntityState` is doing some hundred times more work than actually used here: let cmpTemplateManager = Engine.QueryInterface(SYSTEM_ENTITY, IID_TemplateManager); let template = cmpTemplateManager.GetCurrentTemplateName(ent); If anything one could argue that this precaching is not adding overhead since the calls will be 100% ensured to be performed later on. (But even then, wondering if its right to precache this way here.) (Or with the current code it looks like the entity states were obtained already, so theyd be cached already?)

Not useful :)

Revision Contents
Changeset List

Path

Size

binaries/

data/

mods/

public/

gui/

session/

selection.js

23 lines

selection_panels.js

10 lines

Diff 9882

View Options

binaries/data/mods/public/gui/session/selection.js