Patch by smiley fixing rP21026
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
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
Patch by smiley
Is it really? ax -> in hardly counts as a change, let alone a patch.
Is there legal meaning behind that?
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!
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.