Skip to content

Avoids fetching the same position many times in a batch - #2417

Open
Menkib64 wants to merge 6 commits into
LeelaChessZero:masterfrom
Menkib64:memcache_delayed_cache_hit
Open

Avoids fetching the same position many times in a batch#2417
Menkib64 wants to merge 6 commits into
LeelaChessZero:masterfrom
Menkib64:memcache_delayed_cache_hit

Conversation

@Menkib64

@Menkib64 Menkib64 commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

No description provided.

Copilot AI review requested due to automatic review settings June 1, 2026 18:44

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds intra-batch deduplication of NN evaluation requests: when multiple positions in the same batch (or across overlapping batches) hash to the same cache key, only one is sent to the underlying backend; other queriers wait on a per-entry state machine for the result. Cache slots are reserved up-front via HashKeyedCache::Insert, and a new FETCHED_DELAYED AddInput result is plumbed through both classic and dag_classic search workers so the wait is accounted as out-of-order rather than a full network evaluation.

Changes:

  • Introduce a WaitableAtomic<T> shim (src/utils/atomic.h) that falls back to a Mutex + std::condition_variable when __cpp_lib_atomic_wait is unavailable.
  • Rework MemCacheComputation to insert a placeholder CachedValue with a 4-state machine (NOT_QUEUED / NO_WAITERS / WAITERS / READY) and over-allocate entries_ to batch * (1 + max-out-of-order-evals-factor).
  • Add FETCHED_DELAYED to BackendComputation::AddInputResult and a corresponding is_delayed_cache_hit flag that decrements minibatch_size and skips network_evaluations_++ in both search variants.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/utils/atomic.h New WaitableAtomic<T> with fallback wait/notify via mutex+condvar.
src/utils/cache.h HashKeyedCache::Insert now takes std::unique_ptr<V>& so the caller can tell whether ownership was actually transferred.
src/neural/backend.h Adds FETCHED_DELAYED to AddInputResult.
src/neural/memcache.cc Implements in-flight dedup with a 4-state machine, waits on WaitableAtomic, sizes entries_ using classic search params.
src/search/classic/search.{h,cc} Adds is_delayed_cache_hit, treats it as out-of-order, and excludes it from network_evaluations_.
src/search/dag_classic/search.{h,cc} Same plumbing for the dag-classic search.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/utils/cache.h Outdated
Comment thread src/neural/memcache.cc Outdated
Comment thread src/neural/memcache.cc
Comment thread src/neural/memcache.cc Outdated
@Menkib64

Menkib64 commented Jun 1, 2026

Copy link
Copy Markdown
Contributor Author

Testing using cuda-fp16 on 5080.
master:

Total time (ms) : 340963
Nodes searched  : 6950695
Nodes/second    : 20385

this pull request:

Total time (ms) : 340887
Nodes searched  : 8334170
Nodes/second    : 24448

It looks like I have to still check a little more code to avoid adding some bugs.

@Menkib64

Menkib64 commented Jun 1, 2026

Copy link
Copy Markdown
Contributor Author

dag-preview may gain a little because positions are inserted to tt only after evaluation.

I used go nodes 123456 from start position:
master:

info depth 11 seldepth 46 time 14670 nodes 123479 score cp 12 nps 9988 eps 7499

branch:

info depth 11 seldepth 45 time 14519 nodes 123570 score cp 12 nps 10036 eps 7496

@Menkib64

Menkib64 commented Jun 7, 2026

Copy link
Copy Markdown
Contributor Author

Test results dag 8+0.08, dag 40+0.4, classic 8+0.08, and classic 40+0.4

@Kovax007

Kovax007 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Also passes nonregression on datageneration setup with fixed nodes: https://bench-direct.lczero.org/test/1019/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants