From e8472b5fbe1eed1cbcdfe06eb8ae9206bac773e0 Mon Sep 17 00:00:00 2001 From: Joost VandeVondele Date: Fri, 18 Sep 2020 20:22:01 +0200 Subject: [PATCH] Fix races in gensfen as detected with thread sanitizer. RootInTB was an incorrectly shared global, probably leading to wrong scoreing Minor: setting TB global state from input by all threads (all threads write same values) setting Limits global state by all threads (idem) thread counting for finalization CI can be enabled once races are fixed in the learner, manually goes like: ``` make clean && make -j2 ARCH=x86-64-modern sanitize=thread optimize=no debug=yes build ../tests/instrumented_learn.sh --sanitizer-thread ``` Needs some review. --- src/learn/multi_think.cpp | 33 ++++++++++++++----- src/learn/multi_think.h | 5 +-- src/search.cpp | 69 ++++++++++++++------------------------- src/search.h | 12 +++++++ src/syzygy/tbprobe.h | 2 -- src/thread.cpp | 2 ++ src/thread.h | 1 + 7 files changed, 66 insertions(+), 58 deletions(-) diff --git a/src/learn/multi_think.cpp b/src/learn/multi_think.cpp index 043238fa..22e49e81 100644 --- a/src/learn/multi_think.cpp +++ b/src/learn/multi_think.cpp @@ -3,6 +3,7 @@ #include "tt.h" #include "uci.h" #include "types.h" +#include "search.h" #include @@ -23,6 +24,27 @@ void MultiThink::go_think() // Call the derived class's init(). init(); + // init global vars + Tablebases::init(); + + // About Search::Limits + // Be careful because this member variable is global and affects other threads. + { + auto& limits = Search::Limits; + + // Make the search equivalent to the "go infinite" command. (Because it is troublesome if time management is done) + limits.infinite = true; + + // Since PV is an obstacle when displayed, erase it. + limits.silent = true; + + // If you use this, it will be compared with the accumulated nodes of each thread. Therefore, do not use it. + limits.nodes = 0; + + // depth is also processed by the one passed as an argument of Learner::search(). + limits.depth = 0; + } + // The loop upper limit is set with set_loop_max(). loop_count = 0; done_count = 0; @@ -32,12 +54,11 @@ void MultiThink::go_think() auto thread_num = (size_t)Options["Threads"]; // Secure end flag of worker thread - thread_finished.resize(thread_num); + threads_finished=0; // start worker thread for (size_t i = 0; i < thread_num; ++i) { - thread_finished[i] = 0; threads.push_back(std::thread([i, this] { // exhaust all processor threads. @@ -47,7 +68,7 @@ void MultiThink::go_think() this->thread_worker(i); // Set the end flag because the thread has ended - this->thread_finished[i] = 1; + this->threads_finished++; })); } @@ -61,11 +82,7 @@ void MultiThink::go_think() // function to determine if all threads have finished auto threads_done = [&]() { - // returns false if no one is finished - for (auto& f : thread_finished) - if (!f) - return false; - return true; + return threads_finished == thread_num; }; // Call back if the callback function is set. diff --git a/src/learn/multi_think.h b/src/learn/multi_think.h index 4b5662aa..e6c436f8 100644 --- a/src/learn/multi_think.h +++ b/src/learn/multi_think.h @@ -96,10 +96,7 @@ private: std::mutex loop_mutex; // Thread end flag. - // vector may not be reflected properly when trying to rewrite from multiple threads... - typedef uint8_t Flag; - std::vector thread_finished; - + std::atomic threads_finished; }; // Mechanism to process task during idle time. diff --git a/src/search.cpp b/src/search.cpp index 7c6f8ace..9f5119a2 100644 --- a/src/search.cpp +++ b/src/search.cpp @@ -43,9 +43,24 @@ namespace Search { namespace Tablebases { int Cardinality; - bool RootInTB; bool UseRule50; Depth ProbeDepth; + + void init() { + + UseRule50 = bool(Options["Syzygy50MoveRule"]); + ProbeDepth = int(Options["SyzygyProbeDepth"]); + Cardinality = int(Options["SyzygyProbeLimit"]); + + // Tables with fewer pieces than SyzygyProbeLimit are searched with + // ProbeDepth == DEPTH_ZERO + if (Cardinality > MaxCardinality) + { + Cardinality = MaxCardinality; + ProbeDepth = 0; + } + } + } namespace TB = Tablebases; @@ -1844,7 +1859,7 @@ string UCI::pv(const Position& pos, Depth depth, Value alpha, Value beta) { size_t pvIdx = pos.this_thread()->pvIdx; size_t multiPV = std::min((size_t)Options["MultiPV"], rootMoves.size()); uint64_t nodesSearched = Threads.nodes_searched(); - uint64_t tbHits = Threads.tb_hits() + (TB::RootInTB ? rootMoves.size() : 0); + uint64_t tbHits = Threads.tb_hits() + (pos.this_thread()->rootInTB ? rootMoves.size() : 0); for (size_t i = 0; i < multiPV; ++i) { @@ -1856,7 +1871,7 @@ string UCI::pv(const Position& pos, Depth depth, Value alpha, Value beta) { Depth d = updated ? depth : depth - 1; Value v = updated ? rootMoves[i].score : rootMoves[i].previousScore; - bool tb = TB::RootInTB && abs(v) < VALUE_MATE_IN_MAX_PLY; + bool tb = pos.this_thread()->rootInTB && abs(v) < VALUE_MATE_IN_MAX_PLY; v = tb ? rootMoves[i].tbScore : v; if (ss.rdbuf()->in_avail()) // Not at first line @@ -1923,10 +1938,8 @@ bool RootMove::extract_ponder_from_tt(Position& pos) { void Tablebases::rank_root_moves(Position& pos, Search::RootMoves& rootMoves) { - RootInTB = false; - UseRule50 = bool(Options["Syzygy50MoveRule"]); - ProbeDepth = int(Options["SyzygyProbeDepth"]); - Cardinality = int(Options["SyzygyProbeLimit"]); + auto& rootInTB = pos.this_thread()->rootInTB; + rootInTB = false; bool dtz_available = true; // Tables with fewer pieces than SyzygyProbeLimit are searched with @@ -1940,17 +1953,17 @@ void Tablebases::rank_root_moves(Position& pos, Search::RootMoves& rootMoves) { if (Cardinality >= popcount(pos.pieces()) && !pos.can_castle(ANY_CASTLING)) { // Rank moves using DTZ tables - RootInTB = root_probe(pos, rootMoves); + rootInTB = root_probe(pos, rootMoves); - if (!RootInTB) + if (!rootInTB) { // DTZ tables are missing; try to rank moves using WDL tables dtz_available = false; - RootInTB = root_probe_wdl(pos, rootMoves); + rootInTB = root_probe_wdl(pos, rootMoves); } } - if (RootInTB) + if (rootInTB) { // Sort moves according to TB rank std::sort(rootMoves.begin(), rootMoves.end(), @@ -1966,6 +1979,7 @@ void Tablebases::rank_root_moves(Position& pos, Search::RootMoves& rootMoves) { for (auto& m : rootMoves) m.tbRank = 0; } + } // --- expose the functions such as fixed depth search used for learning to the outside @@ -1987,39 +2001,6 @@ namespace Learner std::memset(ss - 7, 0, 10 * sizeof(Stack)); - // About Search::Limits - // Be careful because this member variable is global and affects other threads. - { - auto& limits = Search::Limits; - - // Make the search equivalent to the "go infinite" command. (Because it is troublesome if time management is done) - limits.infinite = true; - - // Since PV is an obstacle when displayed, erase it. - limits.silent = true; - - // If you use this, it will be compared with the accumulated nodes of each thread. Therefore, do not use it. - limits.nodes = 0; - - // depth is also processed by the one passed as an argument of Learner::search(). - limits.depth = 0; - - // Set a large value to prevent the draw value from being returned due to the number of moves near the draw. - //limits.max_game_ply = 1 << 16; - - // If you do not include the ball entry rule, it will be a draw and it will be difficult to settle. - //limits.enteringKingRule = EnteringKingRule::EKR_27_POINT; - } - - // Set DrawValue - { - // Because it is not prepared for each thread - // May be overwritten by another thread. There is no help for it. - // If that happens, I think it should be 0. - //drawValueTable[REPETITION_DRAW][BLACK] = VALUE_ZERO; - //drawValueTable[REPETITION_DRAW][WHITE] = VALUE_ZERO; - } - // Regarding this_thread. { diff --git a/src/search.h b/src/search.h index 20dfe909..fd5814ef 100644 --- a/src/search.h +++ b/src/search.h @@ -24,6 +24,7 @@ #include "misc.h" #include "movepick.h" #include "types.h" +#include "uci.h" class Position; @@ -110,6 +111,17 @@ void clear(); } // namespace Search +namespace Tablebases { + +extern int MaxCardinality; +extern int Cardinality; +extern bool UseRule50; +extern Depth ProbeDepth; + +void init(); + +} + namespace Learner { // A pair of reader and evaluation value. Returned by Learner::search(),Learner::qsearch(). diff --git a/src/syzygy/tbprobe.h b/src/syzygy/tbprobe.h index b998989b..6af5d278 100644 --- a/src/syzygy/tbprobe.h +++ b/src/syzygy/tbprobe.h @@ -43,8 +43,6 @@ enum ProbeState { ZEROING_BEST_MOVE = 2 // Best move zeroes DTZ (capture or pawn move) }; -extern int MaxCardinality; - void init(const std::string& paths); WDLScore probe_wdl(Position& pos, ProbeState* result); int probe_dtz(Position& pos, ProbeState* result); diff --git a/src/thread.cpp b/src/thread.cpp index 1aa66a81..ef4cb398 100644 --- a/src/thread.cpp +++ b/src/thread.cpp @@ -192,6 +192,8 @@ void ThreadPool::start_thinking(Position& pos, StateListPtr& states, || std::count(limits.searchmoves.begin(), limits.searchmoves.end(), m)) rootMoves.emplace_back(m); + Tablebases::init(); + if (!rootMoves.empty()) Tablebases::rank_root_moves(pos, rootMoves); diff --git a/src/thread.h b/src/thread.h index 042bc2e9..e0c838c8 100644 --- a/src/thread.h +++ b/src/thread.h @@ -74,6 +74,7 @@ public: CapturePieceToHistory captureHistory; ContinuationHistory continuationHistory[2][2]; Score contempt; + bool rootInTB; };