From 5a88c5bb9b3e5ee431ac85abb8981b1571b68b2d Mon Sep 17 00:00:00 2001 From: Stefano Di Martino Date: Sat, 7 Jan 2023 01:08:30 +0100 Subject: [PATCH] Modernize code base a little bit Removed sprintf() which generated a warning, because of security reasons. Replace NULL with nullptr Replace typedef with using Do not inherit from std::vector. Use composition instead. optimize mutex-unlocking closes https://github.com/official-stockfish/Stockfish/pull/4327 No functional change --- AUTHORS | 1 + src/material.h | 2 +- src/misc.cpp | 8 ++++---- src/nnue/evaluate_nnue.cpp | 31 ++++++++++++------------------- src/thread.cpp | 38 +++++++++++++++++++------------------- src/thread.h | 14 +++++++++++--- src/thread_win32_osx.h | 6 +++--- 7 files changed, 51 insertions(+), 49 deletions(-) diff --git a/AUTHORS b/AUTHORS index 998399b9..87fed7b8 100644 --- a/AUTHORS +++ b/AUTHORS @@ -192,6 +192,7 @@ Shawn Varghese (xXH4CKST3RXx) Siad Daboul (Topologist) Stefan Geschwentner (locutus2) Stefano Cardanobile (Stefano80) +Stefano Di Martino (StefanoD) Steinar Gunderson (sesse) Stéphane Nicolet (snicolet) Syine Mineta (MinetaS) diff --git a/src/material.h b/src/material.h index f6db85c4..73c83100 100644 --- a/src/material.h +++ b/src/material.h @@ -28,7 +28,7 @@ namespace Stockfish::Material { /// Material::Entry contains various information about a material configuration. /// It contains a material imbalance evaluation, a function pointer to a special -/// endgame evaluation function (which in most cases is NULL, meaning that the +/// endgame evaluation function (which in most cases is nullptr, meaning that the /// standard evaluation function will be used), and scale factors. /// /// The scale factors are used to scale the evaluation score up or down. For diff --git a/src/misc.cpp b/src/misc.cpp index 1cb515c3..be7abba5 100644 --- a/src/misc.cpp +++ b/src/misc.cpp @@ -413,7 +413,7 @@ static void* aligned_large_pages_alloc_windows([[maybe_unused]] size_t allocSize if (!OpenProcessToken(GetCurrentProcess(), TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY, &hProcessToken)) return nullptr; - if (LookupPrivilegeValue(NULL, SE_LOCK_MEMORY_NAME, &luid)) + if (LookupPrivilegeValue(nullptr, SE_LOCK_MEMORY_NAME, &luid)) { TOKEN_PRIVILEGES tp { }; TOKEN_PRIVILEGES prevTp { }; @@ -432,10 +432,10 @@ static void* aligned_large_pages_alloc_windows([[maybe_unused]] size_t allocSize // Round up size to full pages and allocate allocSize = (allocSize + largePageSize - 1) & ~size_t(largePageSize - 1); mem = VirtualAlloc( - NULL, allocSize, MEM_RESERVE | MEM_COMMIT | MEM_LARGE_PAGES, PAGE_READWRITE); + nullptr, allocSize, MEM_RESERVE | MEM_COMMIT | MEM_LARGE_PAGES, PAGE_READWRITE); // Privilege no longer needed, restore previous state - AdjustTokenPrivileges(hProcessToken, FALSE, &prevTp, 0, NULL, NULL); + AdjustTokenPrivileges(hProcessToken, FALSE, &prevTp, 0, nullptr, nullptr); } } @@ -453,7 +453,7 @@ void* aligned_large_pages_alloc(size_t allocSize) { // Fall back to regular, page aligned, allocation if necessary if (!mem) - mem = VirtualAlloc(NULL, allocSize, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE); + mem = VirtualAlloc(nullptr, allocSize, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE); return mem; } diff --git a/src/nnue/evaluate_nnue.cpp b/src/nnue/evaluate_nnue.cpp index a06c1978..06281da0 100644 --- a/src/nnue/evaluate_nnue.cpp +++ b/src/nnue/evaluate_nnue.cpp @@ -26,7 +26,6 @@ #include "../evaluate.h" #include "../position.h" -#include "../misc.h" #include "../uci.h" #include "../types.h" @@ -245,14 +244,15 @@ namespace Stockfish::Eval::NNUE { } - // format_cp_aligned_dot() converts a Value into (centi)pawns and writes it in a buffer, - // always keeping two decimals. The buffer must have capacity for at least 7 chars. - static void format_cp_aligned_dot(Value v, char* buffer) { + // format_cp_aligned_dot() converts a Value into (centi)pawns, always keeping two decimals. + static void format_cp_aligned_dot(Value v, std::stringstream &stream) { + const double cp = 1.0 * std::abs(int(v)) / UCI::NormalizeToPawnValue; - buffer[0] = (v < 0 ? '-' : v > 0 ? '+' : ' '); - - double cp = 1.0 * std::abs(int(v)) / UCI::NormalizeToPawnValue; - sprintf(&buffer[1], "%6.2f", cp); + stream << (v < 0 ? '-' : v > 0 ? '+' : ' ') + << std::setiosflags(std::ios::fixed) + << std::setw(6) + << std::setprecision(2) + << cp; } @@ -332,17 +332,10 @@ namespace Stockfish::Eval::NNUE { for (std::size_t bucket = 0; bucket < LayerStacks; ++bucket) { - char buffer[3][8]; - std::memset(buffer, '\0', sizeof(buffer)); - - format_cp_aligned_dot(t.psqt[bucket], buffer[0]); - format_cp_aligned_dot(t.positional[bucket], buffer[1]); - format_cp_aligned_dot(t.psqt[bucket] + t.positional[bucket], buffer[2]); - - ss << "| " << bucket << " " - << " | " << buffer[0] << " " - << " | " << buffer[1] << " " - << " | " << buffer[2] << " " + ss << "| " << bucket << " "; + ss << " | "; format_cp_aligned_dot(t.psqt[bucket], ss); ss << " " + << " | "; format_cp_aligned_dot(t.positional[bucket], ss); ss << " " + << " | "; format_cp_aligned_dot(t.psqt[bucket] + t.positional[bucket], ss); ss << " " << " |"; if (bucket == t.correctBucket) ss << " <-- this bucket is used"; diff --git a/src/thread.cpp b/src/thread.cpp index 7e71edf1..ca1a7c85 100644 --- a/src/thread.cpp +++ b/src/thread.cpp @@ -73,9 +73,9 @@ void Thread::clear() { /// Thread::start_searching() wakes up the thread that will start the search void Thread::start_searching() { - - std::lock_guard lk(mutex); + mutex.lock(); searching = true; + mutex.unlock(); // Unlock before notifying saves a few CPU-cycles cv.notify_one(); // Wake up the thread in idle_loop() } @@ -125,20 +125,20 @@ void Thread::idle_loop() { void ThreadPool::set(size_t requested) { - if (size() > 0) // destroy any existing thread(s) + if (threads.size() > 0) // destroy any existing thread(s) { main()->wait_for_search_finished(); - while (size() > 0) - delete back(), pop_back(); + while (threads.size() > 0) + delete threads.back(), threads.pop_back(); } if (requested > 0) // create new thread(s) { - push_back(new MainThread(0)); + threads.push_back(new MainThread(0)); - while (size() < requested) - push_back(new Thread(size())); + while (threads.size() < requested) + threads.push_back(new Thread(threads.size())); clear(); // Reallocate the hash with the new threadpool size @@ -154,7 +154,7 @@ void ThreadPool::set(size_t requested) { void ThreadPool::clear() { - for (Thread* th : *this) + for (Thread* th : threads) th->clear(); main()->callsCnt = 0; @@ -187,7 +187,7 @@ void ThreadPool::start_thinking(Position& pos, StateListPtr& states, Tablebases::rank_root_moves(pos, rootMoves); // After ownership transfer 'states' becomes empty, so if we stop the search - // and call 'go' again without setting a new position states.get() == NULL. + // and call 'go' again without setting a new position states.get() == nullptr. assert(states.get() || setupStates.get()); if (states.get()) @@ -198,7 +198,7 @@ void ThreadPool::start_thinking(Position& pos, StateListPtr& states, // be deduced from a fen string, so set() clears them and they are set from // setupStates->back() later. The rootState is per thread, earlier states are shared // since they are read-only. - for (Thread* th : *this) + for (Thread* th : threads) { th->nodes = th->tbHits = th->nmpMinPly = th->bestMoveChanges = 0; th->rootDepth = th->completedDepth = 0; @@ -212,12 +212,12 @@ void ThreadPool::start_thinking(Position& pos, StateListPtr& states, Thread* ThreadPool::get_best_thread() const { - Thread* bestThread = front(); + Thread* bestThread = threads.front(); std::map votes; Value minScore = VALUE_NONE; // Find minimum score of all threads - for (Thread* th: *this) + for (Thread* th: threads) minScore = std::min(minScore, th->rootMoves[0].score); // Vote according to score and depth, and select the best thread @@ -225,10 +225,10 @@ Thread* ThreadPool::get_best_thread() const { return (th->rootMoves[0].score - minScore + 14) * int(th->completedDepth); }; - for (Thread* th : *this) + for (Thread* th : threads) votes[th->rootMoves[0].pv[0]] += thread_value(th); - for (Thread* th : *this) + for (Thread* th : threads) if (abs(bestThread->rootMoves[0].score) >= VALUE_TB_WIN_IN_MAX_PLY) { // Make sure we pick the shortest mate / TB conversion or stave off mate the longest @@ -251,8 +251,8 @@ Thread* ThreadPool::get_best_thread() const { void ThreadPool::start_searching() { - for (Thread* th : *this) - if (th != front()) + for (Thread* th : threads) + if (th != threads.front()) th->start_searching(); } @@ -261,8 +261,8 @@ void ThreadPool::start_searching() { void ThreadPool::wait_for_search_finished() const { - for (Thread* th : *this) - if (th != front()) + for (Thread* th : threads) + if (th != threads.front()) th->wait_for_search_finished(); } diff --git a/src/thread.h b/src/thread.h index 680da209..7566322c 100644 --- a/src/thread.h +++ b/src/thread.h @@ -101,13 +101,13 @@ struct MainThread : public Thread { /// parking and, most importantly, launching a thread. All the access to threads /// is done through this class. -struct ThreadPool : public std::vector { +struct ThreadPool { void start_thinking(Position&, StateListPtr&, const Search::LimitsType&, bool = false); void clear(); void set(size_t); - MainThread* main() const { return static_cast(front()); } + MainThread* main() const { return static_cast(threads.front()); } uint64_t nodes_searched() const { return accumulate(&Thread::nodes); } uint64_t tb_hits() const { return accumulate(&Thread::tbHits); } Thread* get_best_thread() const; @@ -116,13 +116,21 @@ struct ThreadPool : public std::vector { std::atomic_bool stop, increaseDepth; + auto cbegin() const noexcept { return threads.cbegin(); } + auto begin() noexcept { return threads.begin(); } + auto end() noexcept { return threads.end(); } + auto cend() const noexcept { return threads.cend(); } + auto size() const noexcept { return threads.size(); } + auto empty() const noexcept { return threads.empty(); } + private: StateListPtr setupStates; + std::vector threads; uint64_t accumulate(std::atomic Thread::* member) const { uint64_t sum = 0; - for (Thread* th : *this) + for (Thread* th : threads) sum += (th->*member).load(std::memory_order_relaxed); return sum; } diff --git a/src/thread_win32_osx.h b/src/thread_win32_osx.h index 01ff1c77..330a8341 100644 --- a/src/thread_win32_osx.h +++ b/src/thread_win32_osx.h @@ -41,7 +41,7 @@ void* start_routine(void* ptr) P* p = reinterpret_cast(ptr); (p->first->*(p->second))(); // Call member function pointer delete p; - return NULL; + return nullptr; } class NativeThread { @@ -56,7 +56,7 @@ public: pthread_attr_setstacksize(attr, TH_STACK_SIZE); pthread_create(&thread, attr, start_routine, new P(obj, fun)); } - void join() { pthread_join(thread, NULL); } + void join() { pthread_join(thread, nullptr); } }; } // namespace Stockfish @@ -65,7 +65,7 @@ public: namespace Stockfish { -typedef std::thread NativeThread; +using NativeThread = std::thread; } // namespace Stockfish -- 2.39.2