Refactor Global Variables

[Disservin]

This aims to remove some of the annoying global structure which Stockfish has.

Overall there is no major elo regression to be expected.

Non regression SMP STC (paused, early version):
https://tests.stockfishchess.org/tests/view/65983d7979aa8af82b9608f1
LLR: 0.23 (-2.94,2.94) <-1.75,0.25>
Total: 76232 W: 19035 L: 19096 D: 38101
Ptnml(0-2): 92, 8735, 20515, 8690, 84

Non regression STC (early version):
https://tests.stockfishchess.org/tests/view/6595b3a479aa8af82b95da7f
LLR: 2.93 (-2.94,2.94) <-1.75,0.25>
Total: 185344 W: 47027 L: 46972 D: 91345
Ptnml(0-2): 571, 21285, 48943, 21264, 609

Non regression SMP STC:
https://tests.stockfishchess.org/tests/view/65a0715c79aa8af82b96b7e4
LLR: 2.94 (-2.94,2.94) <-1.75,0.25>
Total: 142936 W: 35761 L: 35662 D: 71513
Ptnml(0-2): 209, 16400, 38135, 16531, 193

These global structures/variables add hidden dependencies and allow data to be mutable from where it shouldn't it be (i.e. options). They also prevent Stockfish from internal selfplay, which would be a nice thing to be able to do, i.e. instantiate two Stockfish instances and let them play against each other.
It will also allow us to make Stockfish a library, which can be easier used on other platforms.

For consistency with the old search code, thisThread has been kept, even though it is not strictly necessary anymore.
This the first major refactor of this kind (in recent time), and future changes are required,
to achieve the previously described goals. This includes cleaning up the dependencies, transforming the network to be self contained and coming up with a plan to deal with proper tablebase memory management (see comments for more information on this).

The removal of these global structures has been discussed in parts with Vondele and Sopel.

No functional change.

[Disservin]

I've also decoupled the Thread from the Search itself, so that in the future it should be easier to create a standalone search which isn't depending on a running thread, although the Search still depends currently on a threadpool and a mainthread (yikes).

[Disservin]

Additionally I want to also mention that this a branch on Stockfish itself so that people can contribute to it as well. The goal right now is to have some solid baseline which has (almost) all globals removed and instead uses dependency injection. So that we dont have to do such a big refactoring in the future again. The naming is not final yet and I'd appreciate any feedback on the general code structure from this version vs before.

[dsmsgms]

I don't see why UCIHandler shouldn't just be called UCI.

[Disservin]

I don't see why UCIHandler shouldn't just be called UCI.

Sure can do, as I said the naming is not final, there are some other names to be cleaned up.

[vondele]

Thanks for working on this, removing the globals is an important step, as you describe.

In order to avoid the patch to get too large, I would suggest to merge it once a good intermediate state is reached. Do we have an understanding / list of the global variables that have been made more local, and what is remaining ?

[Disservin]

Do we have an understanding / list of the global variables that have been made more local, and what is remaining ?

So before we had these global ones:

extern std::string binaryDirectory;   // path of the executable directory
extern std::string workingDirectory;  // path of the working directory
extern LimitsType Limits;
extern ThreadPool Threads;
extern TimeManagement Time;
extern TranspositionTable TT;
extern UCI::OptionsMap Options;
extern std::unordered_map<NNUE::NetSize, EvalFile> EvalFiles;

extern int MaxCardinality;
TBTables TBTables;

// embedded net data
// movegen stuff

std::string fileName[2];
std::string netDescription[2];

int Reductions[MAX_MOVES];

this is now down to

extern int MaxCardinality;
TBTables TBTables;

// embedded net data
// movegen stuff

Movegen and embedded net data is not that critical...
and the tablebases are a bit more difficult to refactor due to the mmapping which Sopel mentioned on discord.

[Sopel97]

I like how this is starting to look and the emerging abstractions.

Since Reductions depend on thread count they should be a local part of the search.

Regarding movegen stuff. It's all effectively constant, just needs to be initialized at launch. This is fine to stay. It should just be atomically initialized when creating an engine instance.

Regarding the net stuff. It will need to be moved to a repository of some sort. Something that will manage lifetimes of multiple networks currently being in use (by more than one potential instance of stockfish) and will make sure there's no duplicates loaded into memory at any time. A sort of a centrally managed shared_ptr if you will.

Regarding the situation with TBs. It's gonna be messy. I don't remember the conclusion of my rambling on discord last time regarding this, but at it's base it will need to be a similar solution as for the networks, but with an addition of monitoring memory usage and flushing [presumed] OS cached data in regular intervals based on some approximate limits. I guess we could start with the first part of this, just so we don't have the same TB file opened in multiple places. The second part could be considered a quality-of-implementation thing that can be addressed later. Of course we still won't have complete control as multiple processes fighting for TBs would ruin any memory-preservation strategy we could come up with, but that's the best we can do here without resorting to an external OS-wide service.

[Disservin]

Next things todo:

• Remove friend class Stockfish::UCI; from worker
• Refactor evaluate call to not depend on worker
• Abstract networks
• Refactor options, mainly divide the necessary options into smaller components which

probably forgot something else

[Coolchessguykevin]

Hello,

I noticed that after adding this patch to the master, the “Clear hash” option appeared in the Stockfish UCI options and enabled by default. See the image. It didn't exist before the recent commits.

This is strange, because this patch was non-functional change and should not change the UCI-options, as far as I understand.

Was this intended? What does this option do for now? I would be grateful if someone answers (if necessary, I can create a separate issue).

Best regards,
Ivan.

[Disservin]

Ah you are right, the uci option was accidentally changed to a checkbox instead of button. Will fix this in a few hours. The option should have been present before though.

[Disservin]

@Coolchessguykevin sorry for the inconvenience, pushed a fix. It should be of type button now again.

[mstembera]

@Disservin I just noticed that each Worker has it's own instance of std::array<int, MAX_MOVES> reductions; but they all get initialized identically. Seems we should just have one copy for all the workers?

[Disservin]

@Disservin I just noticed that each Worker has it's own instance of std::array<int, MAX_MOVES> reductions; but they all get initialized identically. Seems we should just have one copy for all the workers?

ye probably