andrewbackes / chess

Multipurpose chess package for Go/Golang.
21 stars 5 forks source link

Increase speed by changing some maps to arrays #21

Open jezek opened 4 weeks ago

jezek commented 4 weeks ago

Hello, this PR brings mainly some speedup, without breaking the API (except the BitBoards type, see Note at the end).

The next three commits are actual speedup changes. The increase of performance according to benchstat is:

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?

jezek commented 4 weeks ago

I've converted this to a draft, cause I want to fix the API breaks in this PR.

andrewbackes commented 4 weeks ago

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.

andrewbackes commented 4 weeks ago

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 commented 3 weeks ago

@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.