Adding comment to describe relative_rank. #1
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
No functional change.
|
I am on holiday without access to my computer. Joona, Gary: can anyone test replacing the current code by a simple and run speed measurement with perft. My guess is that despite the extra branch there will be no measurable difference (multiplication is costly if not by a power of 2). Better have self-documenting code without comments, than clever code with comments, if possible. |
|
I assumed the current code is 'Upgrade' of this. |
|
Why did you close this request without do anything about this? |
|
I think we can merge this. Clearly the current code is cryptic and needs to be clarified by a comment. |
|
@zamar: Comments should explain what the code does, not how it does it, unless it is particularly subtle:
Anyway, that was my reasoning for closing the pull request. It sparked no interest form you and gary, so I assumed you didn't want to merge it. |
|
@lucas: Okay, your reasoning is fine with me. |
|
I created this request, because when I tried to understand the "RelativeRank" mean, I didn't understand until I read the code and understand it. So, in my opinion, comment may help. |
Hasimir
pushed a commit
to Hasimir/Stockfish
that referenced
this pull request
Nov 14, 2014
Align TB code to SF coding style
Closed
vondele
referenced
this pull request
in vondele/Stockfish
Jul 26, 2017
Fixes a race when ucinewgame is issued 'after' a search, as seen for a thread-sanitized binary issuing :
```
go depth 5
[wait for bestmove ..]
ucinewgame
```
leading to
```
WARNING: ThreadSanitizer: data race (pid=5148)
Write of size 4 at 0x7fbf5dcf6af8 by main thread:
#0 __gnu_cxx::__enable_if<!std::__is_scalar<Move>::__value, void>::__type std::__fill_a<Move*, Move>(Move*, Move*, Move const&) <null> (stockfish+0x000000462884)
#1 void std::fill<Move*, Move>(Move*, Move*, Move const&) /usr/include/c++/5/bits/stl_algobase.h:747 (stockfish+0x0000004618cc)
#2 StatBoards<16, 64, Move>::fill(Move const&) src/movepick.h:36 (stockfish+0x00000046084f)
#3 Search::clear() src/search.cpp:194 (stockfish+0x000000451732)
#4 newgame src/uci.cpp:145 (stockfish+0x0000004738a0)
#5 UCI::loop(int, char**) src/uci.cpp:200 (stockfish+0x000000473d7d)
#6 main src/main.cpp:47 (stockfish+0x000000433322)
Previous write of size 4 at 0x7fbf5dcf6af8 by thread T1:
#0 update_stats src/search.cpp:1417 (stockfish+0x000000453ecb)
#1 search<(<unnamed>::NodeType)0u> src/search.cpp:1114 (stockfish+0x00000045c4c6)
#2 search<(<unnamed>::NodeType)1u> src/search.cpp:997 (stockfish+0x000000457658)
#3 search<(<unnamed>::NodeType)1u> src/search.cpp:1019 (stockfish+0x000000457a50)
#4 search<(<unnamed>::NodeType)1u> src/search.cpp:1019 (stockfish+0x000000457a50)
#5 Thread::search() src/search.cpp:402 (stockfish+0x000000452e28)
#6 MainThread::search() src/search.cpp:264 (stockfish+0x000000451c32)
#7 Thread::idle_loop() src/thread.cpp:114 (stockfish+0x000000468c90)
```
The solution adopted is to split the uci commands in those that can be issued during search (quit, stop, ponderhit, isready, uci) and those that should not run during search (ucinewgame, setoption, .. ). Waiting for search to finish was previously done only for a 'go'.
This fixes the above race.
There are two side effects, one possibly negative, depending on how a gui deals with uci (or a user at the cli).
- crashes are avoided (e.g. resizing hash during search).
- as earlier commands can block on the search to be completed, 'stop' can be non-effective. This behavior is already present on master :
```
go depth 20
go depth 5
stop
```
will wait for depth 20 to be completed before stopping depth 5. This behavior now shows for other commands as well. As such some gui testing of this patch would be useful.
No functional change.
vondele
referenced
this pull request
in vondele/Stockfish
Jul 27, 2017
limits.ponder was used as a signal between uci and search threads, but is not an atomic variable,
leading to the following race as flagged by a sanitized binary.
Expect input:
```
spawn ./stockfish
send "uci\n"
expect "uciok"
send "setoption name Ponder value true\n"
send "go wtime 4000 btime 4000\n"
expect "bestmove"
send "position startpos e2e4 d7d5\n"
send "go wtime 4000 btime 4000 ponder\n"
sleep 0.01
send "ponderhit\n"
expect "bestmove"
send "quit\n"
expect eof
```
Race:
```
WARNING: ThreadSanitizer: data race (pid=7191)
Read of size 4 at 0x0000005c2260 by thread T1:
#0 MainThread::check_time() src/search.cpp:1488 (stockfish+0x000000454471)
#1 search<(<unnamed>::NodeType)0u> src/search.cpp:566 (stockfish+0x0000004594e0)
#2 search<(<unnamed>::NodeType)0u> src/search.cpp:997 (stockfish+0x00000045bfb6)
#3 search<(<unnamed>::NodeType)0u> src/search.cpp:1006 (stockfish+0x00000045c1a3)
#4 search<(<unnamed>::NodeType)1u> src/search.cpp:997 (stockfish+0x000000457658)
#5 Thread::search() src/search.cpp:402 (stockfish+0x000000452e28)
#6 MainThread::search() src/search.cpp:264 (stockfish+0x000000451c32)
#7 Thread::idle_loop() src/thread.cpp:114 (stockfish+0x000000468c90)
Previous write of size 4 at 0x0000005c2260 by main thread:
#0 UCI::loop(int, char**) src/uci.cpp:193 (stockfish+0x000000473c9b)
#1 main src/main.cpp:47 (stockfish+0x000000433322)
Location is global 'Search::Limits' of size 88 at 0x0000005c2220 (stockfish+0x0000005c2260)
```
The fix is to add an atomic bool to the threads structure to signal the ponder status, letting Search::Limits to reflect just what was passed to 'go'.
No functional change.
vondele
referenced
this pull request
in vondele/Stockfish
Aug 2, 2017
limits.ponder was used as a signal between uci and search threads, but is not an atomic variable,
leading to the following race as flagged by a sanitized binary.
Expect input:
```
spawn ./stockfish
send "uci\n"
expect "uciok"
send "setoption name Ponder value true\n"
send "go wtime 4000 btime 4000\n"
expect "bestmove"
send "position startpos e2e4 d7d5\n"
send "go wtime 4000 btime 4000 ponder\n"
sleep 0.01
send "ponderhit\n"
expect "bestmove"
send "quit\n"
expect eof
```
Race:
```
WARNING: ThreadSanitizer: data race (pid=7191)
Read of size 4 at 0x0000005c2260 by thread T1:
#0 MainThread::check_time() src/search.cpp:1488 (stockfish+0x000000454471)
#1 search<(<unnamed>::NodeType)0u> src/search.cpp:566 (stockfish+0x0000004594e0)
#2 search<(<unnamed>::NodeType)0u> src/search.cpp:997 (stockfish+0x00000045bfb6)
#3 search<(<unnamed>::NodeType)0u> src/search.cpp:1006 (stockfish+0x00000045c1a3)
#4 search<(<unnamed>::NodeType)1u> src/search.cpp:997 (stockfish+0x000000457658)
#5 Thread::search() src/search.cpp:402 (stockfish+0x000000452e28)
#6 MainThread::search() src/search.cpp:264 (stockfish+0x000000451c32)
#7 Thread::idle_loop() src/thread.cpp:114 (stockfish+0x000000468c90)
Previous write of size 4 at 0x0000005c2260 by main thread:
#0 UCI::loop(int, char**) src/uci.cpp:193 (stockfish+0x000000473c9b)
#1 main src/main.cpp:47 (stockfish+0x000000433322)
Location is global 'Search::Limits' of size 88 at 0x0000005c2220 (stockfish+0x0000005c2260)
```
The fix is to add an atomic bool to the threads structure to signal the ponder status, letting Search::Limits to reflect just what was passed to 'go'.
No functional change.
protonspring
referenced
this pull request
in protonspring/Stockfish
Apr 11, 2018
protonspring
referenced
this pull request
in protonspring/Stockfish
Apr 22, 2018
protonspring
referenced
this pull request
in ElbertoOne/Stockfish
Jan 9, 2019
Closed
mstembera
pushed a commit
to mstembera/Stockfish
that referenced
this pull request
Jul 15, 2020
Closed
Closed
Closed
locutus2
referenced
this pull request
in locutus2/Stockfish
Jun 28, 2023
AYESDIE
added a commit
to AYESDIE/Stockfish
that referenced
this pull request
Feb 8, 2025
AYESDIE
added a commit
to AYESDIE/Stockfish
that referenced
this pull request
Feb 9, 2025
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.
That's can save time for someone who try to understand it.
No functional change.