Page MenuHomeWildfire Games

Optimize GetIdentityClasses in Templates.js
Needs ReviewPublic

Authored by Stan on Nov 4 2019, 10:52 PM.

Details

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

While looking for things to optimize I looked at the identity component, as apparently it's one of the slowest to be created because of the caching.

That component calls GetIdentityClasses which according to my profiling takes about 17-23µs to execute. This patch cuts this by half.

This function is called a lot on map generation (to create all entities) but also by the AI in entity.js, each time you open a detail window on a unit (some times more than 20 times) to put stuff in cache. So we might be able able to get back a few ms here and there to do other things

Test Plan

Make sure it's faster.
Check the output is the same, especially when using empty classes.
Notice that rank is passed as an array while it's always a single value.

Diff Detail

Event Timeline

Stan created this revision.Nov 4 2019, 10:52 PM

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

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

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

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

Stan added a comment.Nov 4 2019, 11:08 PM

Running the file following file

with something like

node app.js

I get the following results:

0.0029000000000000002 ms.
0.002 ms.
0.0018 ms. <
0.0019 ms.
0.0028 ms.
0.0015 ms.
0.0015 ms. <
0.0013 ms.
0.0019 ms.
0.0014 ms.
0.0013 ms. <
0.0013 ms.

50000000 iterations

0.00194 ms.
0.0013599999999999999 ms.
0.00134 ms.
0.00138 ms.
Stan added a comment.Nov 4 2019, 11:40 PM

14000000 iterations

Original
0.001952441350057987 ms.
Push for rank
0.0013839924571470226 ms.
String
0.0014404163143730588 ms.
String no regex
0.0011586697571411995 ms.
Only push + For Loops
0.000828526207129471 ms.
Array.prototype.push.apply
0.0008673805714352056 ms.
Poor man's for
0.0008252551714124691 ms.
Poor man's while
0.0008407956429291516 ms.

200 iterations

Original
0.006944495253264904 ms.
Push for rank
0.010242999996989965 ms.
String
0.0030414946377277374 ms.
String no regex
0.003874499816447496 ms.
Only push + For Loops
0.0029770040418952703 ms.
Array.prototype.push.apply
0.004580500535666943 ms.
Poor man's for
0.004935504402965307 ms.
Poor man's while
0.0042970001231879 ms.

5000

Original
0.0032503800000995395 ms.
Push for rank
0.0017564602196216584 ms.
String
0.0021596400067210196 ms.
String no regex
0.0017216000240296125 ms.
Only push + For Loops
0.0020414799917489288 ms.
Array.prototype.push.apply
0.0014430799987167119 ms.
Poor man's for
0.0019078597892075778 ms.
Poor man's while
0.0015446002129465342 ms.

10000

Original
0.0028737799962982534 ms.
Push for rank
0.0019261399982497096 ms.
String
0.0016841801116243005 ms.
String no regex
0.0018637700006365774 ms.
Only push + For Loops
0.0015006100991740822 ms.
Array.prototype.push.apply
0.0011772299883887173 ms.
Poor man's for
0.0011110499966889619 ms.
Poor man's while
0.0012838701019063591 ms.
Stan updated this revision to Diff 10269.Nov 4 2019, 11:41 PM

For in seems to be faster in most cases and safer.

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

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

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

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

Stan updated this revision to Diff 10270.Nov 4 2019, 11:56 PM

Didn't know push returned the array size :D now I do.

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

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

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-extra-semi):
|    | Unnecessary semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/Templates.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/Templates.js
|  45|  45| 		classList.push(template.Rank);
|  46|  46| 
|  47|  47| 	return classList;
|  48|    |-};
|    |  48|+}
|  49|  49| 
|  50|  50| /**
|  51|  51|  * Gets an array with all classes for this identity template
|    | [NORMAL] ESLintBear (operator-linebreak):
|    | '||' should be placed at the end of the line.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/Templates.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/Templates.js
|  90|  90| 		// If the elements are still strings, split them by space or by '+'
|  91|  91| 		if (typeof sublist == "string")
|  92|  92| 			sublist = sublist.split(/[+\s]+/);
|  93|    |-		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1)
|  94|    |-		                    || (c[0] != "!" && classes.indexOf(c) != -1)))
|    |  93|+		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1) ||
|    |  94|+		                    (c[0] != "!" && classes.indexOf(c) != -1)))
|  95|  95| 			return true;
|  96|  96| 	}
|  97|  97| 

binaries/data/mods/public/globalscripts/Templates.js
|  48| };
|    | [NORMAL] JSHintBear:
|    | Unnecessary semicolon.

binaries/data/mods/public/globalscripts/Templates.js
|  94| »   »   ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.
Executing section cli...

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

Freagarach added inline comments.
binaries/data/mods/public/globalscripts/Templates.js
31

+.

bb added a subscriber: bb.Mon, Jan 13, 7:48 PM
bb added inline comments.
binaries/data/mods/public/globalscripts/Templates.js
39

did you also try classList.push(...template.Classes._string.split(/\s+/))?

Stan added inline comments.Mon, Jan 13, 7:50 PM
binaries/data/mods/public/globalscripts/Templates.js
39

No but I thought that the spread operator was bad for performance, and in my tests, for push was always faster than everything else.

bb added a comment.Tue, Jan 14, 10:41 PM

Testing with SM gave that string without regex is fastest (about 20x), difference between other options is small.

As de class list is defined as a space-separated list, doing `split(" ") should work. Testing with adding more spaces tabs newlines and such doesn't make the front fall. So it seems to work, maybe I missed something?
The regex has been implemented by rP15195 (addition of visibleClasses).