Page MenuHomeWildfire Games

Replace an incorrect math.max() by math.min() in tileclass
ClosedPublic

Authored by FeXoR on Jun 25 2019, 6:09 PM.

Details

Reviewers
elexis
Stan
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22635: Replace an incorrect math.max() by math.min() in tileclass
Trac Tickets
#4992
Summary

Patch by smiley fixing rP21026

Test Plan

Check that is the correct fix.
Measure how badly it affected performance.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/TileClass.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/TileClass.js
|   1|    |-//////////////////////////////////////////////////////////////////////
|    |   1|+// ////////////////////////////////////////////////////////////////////
|   2|   2| //	RangeOp
|   3|   3| //
|   4|   4| //	Class for efficiently finding number of points within a range
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/TileClass.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/TileClass.js
|   3|   3| //
|   4|   4| //	Class for efficiently finding number of points within a range
|   5|   5| //
|   6|    |-//////////////////////////////////////////////////////////////////////
|    |   6|+// ////////////////////////////////////////////////////////////////////
|   7|   7| 
|   8|   8| function RangeOp(size)
|   9|   9| {
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'while' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/TileClass.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/TileClass.js
|   9|   9| {
|  10|  10| 	// Get smallest power of 2 which is greater than or equal to size
|  11|  11| 	this.nn = 1;
|  12|    |-	while (this.nn < size) {
|    |  12|+	while (this.nn < size) 
|  13|  13| 		this.nn *= 2;
|  14|    |-	}
|    |  14|+	
|  15|  15| 
|  16|  16| 	this.vals = new Int16Array(2*this.nn);	// int16
|  17|  17| }
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'for' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/TileClass.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/TileClass.js
|  38|  38| 
|  39|  39| 	// Count from start to end by powers of 2
|  40|  40| 	for (; start+i <= end; i *= 2)
|  41|    |-	{
|    |  41|+	
|  42|  42| 		if (start & i)
|  43|  43| 		{	// For each bit in start
|  44|  44| 			ret += this.vals[nn/i + Math.floor(start/i)];
|  45|  45| 			start += i;
|  46|  46| 		}
|  47|    |-	}
|    |  47|+	
|  48|  48| 
|  49|  49| 	//
|  50|  50| 	while(i >= 1)
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/TileClass.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/TileClass.js
|  71|  71| 
|  72|  72| 	for (let i=0; i < size; ++i)
|  73|  73| 	{
|  74|    |-		this.inclusionCount[i] = new Int16Array(size); //int16
|    |  74|+		this.inclusionCount[i] = new Int16Array(size); // int16
|  75|  75| 		this.rangeCount[i] = new RangeOp(size);
|  76|  76| 	}
|  77|  77| }
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/TileClass.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/TileClass.js
|  79|  79| TileClass.prototype.has = function(position)
|  80|  80| {
|  81|  81| 	return !!this.inclusionCount[position.x] && !!this.inclusionCount[position.x][position.y];
|  82|    |-}
|    |  82|+};
|  83|  83| 
|  84|  84| TileClass.prototype.add = function(position)
|  85|  85| {
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/TileClass.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/TileClass.js
| 132| 132| 			{
| 133| 133| 				let dx = ix - position.x;
| 134| 134| 				if (Math.square(dx) + Math.square(dy) <= radius2)
| 135|    |-				{
|    | 135|+				
| 136| 136| 					if (this.inclusionCount[ix] && this.inclusionCount[ix][iy] && this.inclusionCount[ix][iy] > 0)
| 137| 137| 						++members;
| 138| 138| 					else
| 139| 139| 						++nonMembers;
| 140|    |-				}
|    | 140|+				
| 141| 141| 			}
| 142| 142| 		}
| 143| 143| 	}
|    | [NORMAL] ESLintBear (no-else-return):
|    | Unnecessary 'else' after 'return'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/TileClass.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/TileClass.js
| 144| 144| 
| 145| 145| 	if (returnMembers)
| 146| 146| 		return members;
| 147|    |-	else
| 148|    |-		return nonMembers;
|    | 147|+	return nonMembers;
| 149| 148| };
| 150| 149| 
| 151| 150| TileClass.prototype.countMembersInRadius = function(position, radius)

binaries/data/mods/public/maps/random/rmgen/TileClass.js
|  12| »   while·(this.nn·<·size)·{
|    | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
|    | Opening curly brace appears on the same line as controlling statement.

binaries/data/mods/public/maps/random/rmgen/TileClass.js
|  82| }
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
Executing section cli...

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

lyv added a subscriber: lyv.Jun 25 2019, 7:46 PM

Patch by smiley

Is it really? ax -> in hardly counts as a change, let alone a patch.
Is there legal meaning behind that?

FeXoR accepted this revision.Jul 9 2019, 8:26 PM
This revision is now accepted and ready to land.Jul 9 2019, 8:26 PM
Stan added a comment.Aug 5 2019, 12:14 PM

@FeXoR Can you commit this on my behalf ?

FeXoR added a comment.EditedAug 5 2019, 8:18 PM

I did some performance tests.

Settings:

  • Map Size: Medium
  • Everything else as default in Atlas

The time (in ms) spend in the outer for loop in countInRadius():

Map		before	R2010	Total(R2010)
aegean_sea	3808	1181	3435.030
african_plains	4163	1258	2470.635
alpine_lakes	7064	2151	3869.511

So this is a factor of 3 to 4 faster.
Also this shows that the time spend in this loop is 1/2 to 2/3 of the entire generation time!

elexis edited the summary of this revision. (Show Details)Aug 5 2019, 8:22 PM
elexis edited the test plan for this revision. (Show Details)
elexis updated the Trac tickets for this revision.
elexis accepted this revision.Aug 5 2019, 8:28 PM

Hope you have tested the same map generation too, otherwise it may be inconclusive.

From my quick test:

Before:

Generating Mainland of size 384 and 8 players.
Creating playerbases... 0.182092s.
Creating bumps... 1.997998s.
Creating mountains... 1.206145s.
Creating forests... 5.800654s.
Creating dirt patches... 1.185021s.
Creating grass patches... 0.741720s.
Creating stone mines... 0.013721s.
Creating metal mines... 0.003485s.
Creating decoration... 0.074522s.
Creating food... 0.050805s.
Creating food... 0.029630s.
Creating straggler trees... 0.356664s.
Total map generation time: 11.678406s.

After:

Generating Mainland of size 384 and 8 players.
Creating playerbases... 0.155651s.
Creating bumps... 1.521598s.
Creating mountains... 0.523554s.
Creating forests... 2.042138s.
Creating dirt patches... 0.495626s.
Creating grass patches... 0.315888s.
Creating stone mines... 0.007279s.
Creating metal mines... 0.001821s.
Creating decoration... 0.063093s.
Creating food... 0.018779s.
Creating food... 0.010725s.
Creating straggler trees... 0.131207s.
Total map generation time: 5.325894s.

So it means that the "number 27" optimization in TileClass is by a factor of 2 less important than assumed.

And apologies again for breaking it in rP21026.

The code is the equivalent to what it was prior to rP21026.

Also performed a replay-hash-test with and without the diff and it's the same.

FeXoR commandeered this revision.Aug 9 2019, 11:28 AM
FeXoR edited reviewers, added: Stan; removed: FeXoR.
This revision was automatically updated to reflect the committed changes.