Increase speed by changing some maps to arrays #21
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I've converted this to a draft, cause I want to fix the API breaks in this PR. |
|
I'm good with breaking the API, the performance increase is certainly worth it.. We can rev to a new major version, that should be fine. |
|
I went ahead and cut a release at the current head as v1.1.0. Feel free to commit the breaking change. If you do, I'll cut another release as v2.0.0 |
jezek
force-pushed
the
speedup
branch
2 times, most recently
from
74f21e5 to
bf1dc1b
Compare
Only test files are modified.
Some tests for non-standard piece colors/types fail due to panics.
Some other small changes were done.
List of changes:
- Remove external dependencies, assert package.
- Add benchmarks to diag and game package
- Add benchmarks to perft suite in diag package.
- Only first 10 tests from perftsuite.epd are used.
- Depth 0 is omitted.
- The BENCH_DEEP_PERFT_SUITE environment variable set to true, uses depth 5.
WARNING: Can take a long time.
- Add benchmarks for games of various lengths to game package.
- Use-short flag to limit number and type of benchmarks.
- Fix "// Output" line in ExampleLegalMoves in game package to use the example
in tests. It works now, because printing map is now sorted by keys and always passes the tests.
- Use package fen in game tests to decode boards in FEN format.
- Add tests and benchmarks to piece.
- Add tests for *Position.Find().
- Add tests and benchmark for *Position.Threatened().
- Add tests in TestRootMoves() with Black and non-standard colors as ActiveColor.
- Add tests and benchmark for *Position.Put().
- Add more tests for *Position.MakeMove().
- Add positionChanges type to track validate changes between moves in tests.
- Add test position changer function for castling rights.
- Add tests for *Position.Check().
- Add tests and benchmark for *Position.QuickPut().
- Add tests for *Position.MailBox().
- Add tests for *Position.MarshalJSON().
Some other changes were made to improve code readability and consistency. For nicer test logs, *Position.String() is introduced. List of changes: - Fix *Position.Put() to pass the non-standard piece or square tests. - Fix *Position.Threatened() panics on non-standard colors or squares. - Fix *Position.Check() panics on non-standard colors. - Fix *Position.QuickPut() panics on non-standard colors or squares. - Add piece.OtherColor array and use where appropriate to increase readability - Add comments to some local functions to warn that for some inputs a panic can be thrown. - Add *Position.String() and tests - Added makeMove positionChanger for testing purposes. - Added clock positionChanger for testing purposes. - Initial position example: ``` a b c d e f g h ┌─────────────────┐ 8│ r n b q k b n r │8 7│ p p p p p p p p │7 6│ . . . . . . . . │6 5│ . . . . . . . . │5 4│ . . . . . . . . │4 3│ . . . . . . . . │3 2│ P P P P P P P P │2 1│ R N B Q K B N R │1 └─────────────────┘ a b c d e f g h MoveNumber: 1 ActiveColor: White CastlingRights: White: O-O-O O-O Black: O-O-O O-O EnPassant: LastMove: FiftyMoveCount: 0 ThreeFoldCount: 0 MovesLeft: White: 0 Black: 0 Clocks: White: 0s Black: 0s ```
- Use `piece.COLOR_COUNT` instead of `2` for color in loops and array declarations to improve readability. - Introduce and use `piece.TYPE_COUNT` constant. - Change `piece.King+1` to `piece.TYPE_COUNT` in position tests. - Use for-range loop instead for;; in *Position.Reset() for consistency. - Make checks before accessing bitBoard color in *Position.Find(), *Position.Moves() to pass tests. - New *Position.bitBoards() function which returns a BitBoards struct. Is used instead of 'BitBoards(p.bitBoard)' retyping, which is currently not possible.
- don't use map in piece.Type.String() - don't use map in piece.Piece.Figurine() - don't use map in piece.Color.String()
|
@andrewbackes OK, I'm done here. This PR should not break the API. I rerun benchmarks, the numbers are the same as before. Cause there is an addition to API (Position.String()), I would recomend increase minor version number, instead of patch number. Edit: Also go.mod and go.sum should be refreshed (tidied) after this merges. There should be no 3rd party packages usage in the project after this PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hello, this PR brings mainly some speedup, without breaking the API (
except the BitBoards type, see Note at the end).Added tests and benchmarks to see the speedup and be sure nothing is broken.
Fixed some exported methods to never panic, added Position.String() function and made some code more consistent and better readable.
The next three commits are actual speedup changes. The increase of performance according to benchstat is:
PerftSuite - geomean before: 8.598m, after: 1.385m, -83.89%
Game - geomean before: 111.2m, after: 17.30m, -84.44%
PositionThreatened, ParseMove, PositionPut, PositionQuickPut - geomean before: 55.05µ, after: 13.10µ, -76.21%
ColorString, TypeString, PieceFigurine - geomean before: 189.6n, after: 2.021n, -98.93%
ParseMove/PCN-testcases: -35.05%
ParseMove/PCN-non-error-testcases: -39.73%
ParseMove/PCN-valid-non-error-testcases: -39.36%
All benchmark and benchstat files are at https://gist.github.com/jezek/eeba7efb5cf01b5c2a35db89977492e8
Note: The golang approach to versioning is to use Major.Minor.Patch and if you break the API, you should increase the Major number. And when increasing the Major number, there are some recommended practicies on how to. I don't know your stance about this, so I tried not to break the API.
Most of the speedup comes from converting to array the Position.bitBoard field, which is not exported, so it wouldn't break the API.
But when writing this text for this PR, I figured out I broke the API, cause I converted the exported BitBoards type to arrays too. I can try to rewrite the commits to keep the BitBoards type as it was, so the major version number doesn't need to be increased (Edit: I've done it, it is in the speedup-noAPIbreak branch).Edit: Now, there is no API break.But I you don't care about the API breaks or versioning, I can leave it like this and I have another 5 commits, which bring more speedup (exported fields mainly in Position struct are changed). I could add them here too.Edit: I'm going to make this PR to not break the API, all other commits with speedup (and API break) I'll add to a standalone PR after this is merged.@andrewbackes What's your take on this?