Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Zobrist::noPawns which is no longer necessary #4344

Closed

Conversation

stouset
Copy link
Contributor

@stouset stouset commented Jan 17, 2023

This Zobrist key is only used once, to initialize a Position's si->pawnKey. The si->pawnKey is then updated with pawn information as board state is changed.

The si->pawnKey is itself only used in one place: Entry::probe, where it is used to look into the current thread's pawnsTable. This is likewise the only place that the thread's pawnsTable is used.

As a result of the above, we can be confident that this particular key can be removed. It's only ever mixed into a key once, and that key is only ever used to index into a hash table that no other key is used for. Therefore its removal cannot possibly lead to colliding with another key from another source.

Bench: 4106793

https://tests.stockfishchess.org/tests/view/63c5cb8418c20f4929c5fd7e

This Zobrist key is only used once, to initialize a `Position`'s `si->pawnKey`.
The `si->pawnKey` is then updated with pawn information as board state is
changed.

The `si->pawnKey` is itself only used in one place: `Entry::probe`, where it is
used to look into the current thread's `pawnsTable`. This is likewise the only
place that the thread's `pawnsTable` is used.

As a result of the above, we can be confident that this particular key can be
removed. It's only ever mixed into a key once, and that key is only ever used to
index into a hash table that no other key is used for. Therefore its removal
cannot possibly lead to colliding with another key from another source.

Bench: 4106793
@vondele vondele added the to be merged Will be merged shortly label Jan 22, 2023
@joergoster
Copy link
Contributor

This 9eccba7 commit and the mentioned bugs may be of interest.

@vondele vondele removed the to be merged Will be merged shortly label Jan 22, 2023
@vondele
Copy link
Member

vondele commented Jan 22, 2023

So, I guess that needs some more checking to see if this is a problem with this commit.

@dubslow
Copy link
Contributor

dubslow commented Jan 22, 2023

This 9eccba7 commit and the mentioned bugs may be of interest.

Fascinating! If indeed this PR turns out to not be mergeable, that means then that the code is more than a little bit under commented -- we should add this documentation as an alternative to this PR.

@w1wwwwww
Copy link
Contributor

After 18fdc2d was merged, Zobrist::noPawns is no longer used anywhere. This should be merged.

@snicolet snicolet closed this in 027713c Jul 24, 2023
@snicolet
Copy link
Member

Merged via 027713c, thanks!

@snicolet snicolet added the to be merged Will be merged shortly label Jul 24, 2023
mstembera referenced this pull request in ehsanrashid/Stockfish May 27, 2024
Bench: 1717838
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
to be merged Will be merged shortly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants