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

Move some code into threads #2720

Closed

Conversation

protonspring
Copy link

@protonspring protonspring commented Jun 7, 2020

This is a non-functional code style change that moves some pure thread code into the threads class.

It is a bit more code, but it makes search.cpp cleaner and easier to read by hiding some thread specific functionality.

STC (SMP)
LLR: 2.93 (-2.94,2.94) {-1.50,0.50}
Total: 75896 W: 12073 L: 12026 D: 51797
Ptnml(0-2): 828, 8224, 19872, 8121, 903
https://tests.stockfishchess.org/tests/view/5ed492e8f29b40b0fc95a74c

@vondele
Copy link
Member

vondele commented Jun 7, 2020

I'm wondering about this th->bestMoveChanges = 0; statement. It is in the same location as master, but it is not quite logical. Can't it be moved to e.g. Thread::search() or ThreadPool::start_thinking() (the latter place seems most logical for anything that is an atomic in Thread).

@xoto10
Copy link
Contributor

xoto10 commented Jun 7, 2020

Should we do a threaded non-regression test? I don't expect it to be a problem, but it would be embarrassing if a threading code tidy up caused a regression.

@vondele
Copy link
Member

vondele commented Jun 7, 2020

@xoto10 the test was threaded, it is not clear from the commit message.

@vondele
Copy link
Member

vondele commented Jun 8, 2020

@protonspring can you have a look at the bestMoveChanges I made before. Now that the code is refactored, it is a good moment to fix it.

@protonspring
Copy link
Author

protonspring commented Jun 8, 2020

I haven't looked much here, but on first glance, I would set bestMoveChanges to 0 on search(EDIT or start_thinking as suggested above), and add another method for ThreadPool::get_bestMoveChanges which gets the total for all threads in the pool. This would further simplify search.

@vondele vondele added the to be merged Will be merged shortly label Jun 9, 2020
@vondele vondele closed this in 1c65310 Jun 9, 2020
@vondele
Copy link
Member

vondele commented Jun 9, 2020

Thanks!

MichaelB7 pushed a commit to MichaelB7/Stockfish that referenced this pull request Jun 13, 2020
This is a code style change that moves some pure thread code into the threads class.

It is a bit more code, but it makes search.cpp cleaner and easier to read by hiding some thread specific functionality.

STC (SMP)
LLR: 2.93 (-2.94,2.94) {-1.50,0.50}
Total: 75896 W: 12073 L: 12026 D: 51797
Ptnml(0-2): 828, 8224, 19872, 8121, 903
https://tests.stockfishchess.org/tests/view/5ed492e8f29b40b0fc95a74c

closes official-stockfish#2720

No functional change.
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

4 participants