fixed size transposition table #2291
Conversation
| picked_node.tt_low_node = tt_iter->second.lock(); | ||
| } | ||
| #else | ||
| auto entry = search_->tt_->LookupAndPin(picked_node.hash); |
There was a problem hiding this comment.
We could insert nodes here if insertions avoid random slowdowns of the dynamic version. For the testing purposes it is better to delay insert. That makes it easier to compare how the fixed implementation compares to the dynamic implementation.
src/search/dag_classic/search.cc
Outdated
| #ifdef FIX_TT | ||
| search_->tt_->Insert(node_to_process.hash, | ||
| std::make_unique<std::weak_ptr<LowNode>>( | ||
| node_to_process.tt_low_node)); |
There was a problem hiding this comment.
I think this needs a comment that insertions aren't thread safe if a hash is the same. Current implementation silently ignores insertion if hash is already present in the table.
I was thinking about last inserted eviction policy. It isn't perfect match how search access nodes. We can end up evicting nodes which are old insertion which are still close to the PV line. A better eviction policy would be least recently accessed but that would be expensive to implement without major refactoring.
We could reduce risk of eviction in insertion if garbage collector evicts expired weak pointers. I don't think initial version needs this. It can be an useful change when playing matches against strong engines where both agree on a long line.
The table implementation requires unique_ptr storage. Could we modify storage to support weak_ptr too? I think this is also a change that shouldn't be part of initial version. It could be a future improvement. It could be also redundant work if we choose to give ownership to the hash table to allow cycles in the tree.
There was a problem hiding this comment.
I don't see the thread safety issue, can you elaborate? The insertion failing shouldn't cause any correctness issues.
There was a problem hiding this comment.
It would mean transposition might have different low node. Search would work but transpositions wouldn't share information which would affect evaluation. It should be simple change to make Insert return the existing value or an inserted value. The caller can then use the returned value which would be known to be shared between all transpositions.
GatherMinibatch found the same transposition from different paths fairly frequently when I was experiment putting TT Insertion into GatherMinibatch.
The cache is not updated if the hash is found, so an expired weak_ptr would block new insertions until evicted.
|
I'm having crashes with the last commit when the TT is small, (e.g. Hash=1). Converted to draft while I investigate. |
If insert fails due to the hash already being present, an effort is made to use the cached one instead.
|
I got an idea which may improve or worsen things. I'm thinking that we should move ownership of |
|
This is not a new idea, see 699a40c. |
|
I noticed |
2 participants
Can be enabled by the
FIX_TTbuild option (on by default).Introduces a new
Hashuci option to control the size of the transposition table in MB (default 50).