Skip to content

Commit

Permalink
Make just one reductions array be used by all workers instead of havi…
Browse files Browse the repository at this point in the history 
…ng separate instances

No functional change
bench: 2180675
  • Loading branch information
@mstembera
mstembera committed May 7, 2024
1 parent 070e564 commit d04a538
Show file tree
Hide file tree
Showing 4 changed files with 15 additions and 9 deletions.
12 changes: 8 additions & 4 deletions src/search.cpp
View file
Expand Up @@ -57,6 +57,9 @@ namespace {
static constexpr double EvalLevel[10] = {0.981, 0.956, 0.895, 0.949, 0.913,
0.942, 0.933, 0.890, 0.984, 0.941};

// Reductions lookup table initialized on thread count change
std::array<int, MAX_MOVES> reductions; // [depth or moveNumber]

// Futility margin
Value futility_margin(Depth d, bool noTtCutNode, bool improving, bool oppWorsening) {
Value futilityMult = 126 - 46 * noTtCutNode;
Expand Down Expand Up @@ -504,12 +507,13 @@ void Search::Worker::clear() {
for (auto& h : to)
h->fill(-60);

for (size_t i = 1; i < reductions.size(); ++i)
reductions[i] = int((18.93 + std::log(size_t(options["Threads"])) / 2) * std::log(i));

refreshTable.clear(networks);
}

void Search::Worker::init_search() {
for (size_t i = 1; i < reductions.size(); ++i)
reductions[i] = int((18.93 + std::log(size_t(options["Threads"])) / 2) * std::log(i));
}

// Main search function for both PV and non-PV nodes.
template<NodeType nodeType>
Expand Down Expand Up @@ -1641,7 +1645,7 @@ Value Search::Worker::qsearch(Position& pos, Stack* ss, Value alpha, Value beta,
return bestValue;
}

Depth Search::Worker::reduction(bool i, Depth d, int mn, int delta) {
Depth Search::Worker::reduction(bool i, Depth d, int mn, int delta) const {
int reductionScale = reductions[d] * reductions[mn];
return (reductionScale + 1318 - delta * 760 / rootDelta) / 1024 + (!i && reductionScale > 1066);
}
Expand Down
10 changes: 5 additions & 5 deletions src/search.h
View file
Expand Up @@ -247,7 +247,10 @@ class Worker {

bool is_mainthread() const { return thread_idx == 0; }

// Public because they need to be updatable by the stats
// Called by the main thread to initialize search when thread count changes
void init_search();

// Public because they need to be updatible by the stats
CounterMoveHistory counterMoves;
ButterflyHistory mainHistory;
CapturePieceToHistory captureHistory;
Expand All @@ -266,7 +269,7 @@ class Worker {
template<NodeType nodeType>
Value qsearch(Position& pos, Stack* ss, Value alpha, Value beta, Depth depth = 0);

Depth reduction(bool i, Depth d, int mn, int delta);
Depth reduction(bool i, Depth d, int mn, int delta) const;

// Get a pointer to the search manager, only allowed to be called by the
// main thread.
Expand All @@ -293,9 +296,6 @@ class Worker {

size_t thread_idx;

// Reductions lookup table initialized at startup
std::array<int, MAX_MOVES> reductions; // [depth or moveNumber]

// The main thread has a SearchManager, the others have a NullSearchManager
std::unique_ptr<ISearchManager> manager;

Expand Down
1 change: 1 addition & 0 deletions src/thread.cpp
View file
Expand Up @@ -148,6 +148,7 @@ void ThreadPool::set(Search::SharedState sharedState,
clear();

main_thread()->wait_for_search_finished();
main_thread()->init_search();

// Reallocate the hash with the new threadpool size
sharedState.tt.resize(sharedState.options["Hash"], requested);
Expand Down
1 change: 1 addition & 0 deletions src/thread.h
View file
Expand Up @@ -50,6 +50,7 @@ class Thread {
void idle_loop();
void start_searching();
void wait_for_search_finished();
void init_search() { worker->init_search(); }
size_t id() const { return idx; }

std::unique_ptr<Search::Worker> worker;
Expand Down

8 comments on commit d04a538

@vondele
Copy link

@vondele vondele commented on d04a538 May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the reason it is not like this is that this would be a problem with the 'library' development, since different instances of the engine, with different number of threads would have different values in the array. It would probably need to be stored at the 'engine' level. @Disservin ?

@mstembera
Copy link
Owner Author

@mstembera mstembera commented on d04a538 May 7, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked here official-stockfish#4968 (comment) but yes I'm not sure what place is best for the library version. Maybe for the library version Search will be an actual class instead of a namespace?

@Disservin
Copy link

@Disservin Disservin commented on d04a538 May 7, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that version wont do it for the lib version. This might fit into the SharedState struct. Tbh the array isn't really that large and the current version might allow
for modifications if someone wants to create different reductions for the different threads. Bit unsure

@mstembera
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm ok but it looks like the SharedState isn't really stored anywhere. It's just temporarily constructed when setting the ThreadPool.
void Engine::resize_threads() { threads.set({options, threads, tt, networks}, updateContext); }
Seems like we would want to have someplace to store stuff shared by threads eventually so it would be good to figure out where?
Regardless of this patch I mean.

@Disservin
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mh that is true, im open for ideas otherwise ill have a look in the coming days

@mstembera
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Maybe Search would become a class instead of a namespace so the library version can have multiple instances. We could also keep the SharedState around instead of it being temporary. Not sure what is best though. Sorry if I'm not up to speed on all the functionality the library version is intended to support.

@Disservin
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are kinda figuring that out as we make progress right now. One thing we want to avoid is exposing the internal chess logic to user, because otherwise feature requests and what not would start appearing from people who use stockfish as a chess library instead of just using it to communicate with stockfish and full chess support is way more than we need in stockfish really.

Maybe Search would become a class instead of a namespace so the library version can have multiple instances

Which parts are you referring here to, we already have this class around most of the search related logic
https://github.com/official-stockfish/Stockfish/blob/master/src/search.h#L236

@mstembera
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was referring to the Search namespace which could become a class and have one instance per SF instance. The Worker is per thread. Anyway best ignore my suggestions on this right now as I haven't familiarized myself w/ the library stuff much so far.

Please sign in to comment.