instrument: TT lookup/hit counters (hot-path) + env-gated summary

[wopdevries]

Minimal, low-risk instrumentation to measure the transposition-table hit rate on the search hot path.

Changes:

• Add tt_lookup_count and tt_hit_count counters to ThreadData
• Increment counters in apply_ab_tt_lookup (the canonical AB hot-path TT probe)
• Print env-gated summary at SOLVER_DONE when DDS_PRINT_TT_STATS=1

No behavioral changes — counters are cheap increments, output is gated.

Measurement results on hands/largest.txt (21 hardest hands, single-threaded):
Updated findings after per-solve reset fix:

• Per-board hit rate: 57-75% (varies by hand difficulty)
• Per-board overwrite rate: 2-24% (mostly 2-9%)
• Low overwrite rate confirms: replacement churn is NOT the main cause of misses
• Hard hands have lower hit rates due to genuinely unique positions, not TT churn
• Algorithmic improvements (pruning, move ordering) are the next optimization target

How to test:

bazelisk build //library/tests:dtest
DDS_PRINT_TT_STATS=1 ./bazel-bin/library/tests/dtest -f hands/largest.txt -s solve -n 1

[Copilot]

🟡 Changes recommended

It introduces compilation fragility (missing direct standard includes) and a real stats correctness issue (new PageStats fields aren’t reset on TT teardown).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR adds lightweight instrumentation to measure transposition-table (TT) effectiveness on the alpha-beta search hot path, and reports an environment-gated summary at solver completion to support TT sizing/tuning work.

Changes:

• Added per-ThreadData TT probe counters (tt_lookup_count, tt_hit_count) and incremented them in apply_ab_tt_lookup.
• Added env-gated (DDS_PRINT_TT_STATS) stderr summary printing on solver completion paths.
• Extended TransTableL::PageStats with insertion/overwrite counters and incremented them on TT entry creation/overwrite.

File summaries

File	Description
library/src/trans_table/trans_table_l.hpp	Extends PageStats with add/overwrite counters.
library/src/trans_table/trans_table_l.cpp	Initializes new PageStats fields; increments add/overwrite counters in create_or_update.
library/src/system/thread_data.hpp	Adds per-thread TT lookup/hit counters to ThreadData.
library/src/solver_if.cpp	Resets counters per solve and prints env-gated TT hit-rate summary at completion.
library/src/ab_search.cpp	Increments TT lookup/hit counters in the canonical TT probe path.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

There are a few concrete mismatches and consistency issues (notably the env var gating contract and documentation/formatting inconsistencies) that should be addressed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

library/src/trans_table/trans_table_l.hpp:477

• This inline override uses same-line braces; please reformat it to match the Allman function brace style used throughout the codebase.

    auto get_op_stats(int& adds, int& overwrites, int& harvests) const -> void override {
      adds = page_stats_.num_adds_;
      overwrites = page_stats_.num_overwrites_;
      harvests = page_stats_.num_harvests_;
    }

• Files reviewed: 8/8 changed files
• Comments generated: 6
• Review effort level: Lite

[wopdevries]

Direct recommendation: This is a small, low-risk instrumentation PR that I would approve after addressing a couple of portability / style nits. It adds per-thread TT lookup/hit counters, per-solve reset and summary printing gated by DDS_PRINT_TT_STATS, and minimal trans-table operation counters. I scanned the diffs and ran a focused review below.

What I did and how I proceeded

Loaded the PR review workflow and fetched the PR file diff (all changed files).
Read the changes across: library/src/ab_search.cpp, library/src/solver_if.cpp, thread_data.hpp, trans_table headers/impl, and the trans_table unit test mock.
Focused on correctness, thread-safety assumptions, API/ABI surface (virtuals), test updates, and small portability/formatting issues.

PR summary

Purpose: Add cheap instrumentation to measure transposition-table (TT) lookup/hit counts on the Alpha-Beta hot path and print a per-solve summary when DDS_PRINT_TT_STATS=1.
Main changes:
    Add uint64_t tt_lookup_count and tt_hit_count to ThreadData.
    Increment counters in apply_ab_tt_lookup (library/src/ab_search.cpp).
    Reset per-solve counters and call tt->reset_op_stats() at solve start in solver_if.cpp.
    Print summary (lookups, hits, hit_rate, plus TT add/overwrite/harvest stats) when DDS_PRINT_TT_STATS=1 at solver completion in multiple solve functions.
    Add get_op_stats/reset_op_stats virtuals to TransTable interface and implementations (L and S), and update TransTableL to collect adds/overwrites.
    Update trans_table tests' MockTransTable to implement the new virtuals.
Scope: 8 files changed, +112 additions, -1 deletion. Risk: low (non-behavioral for solver correctness; counters and printing only).

Core changes and rationale

Instrumentation point is apply_ab_tt_lookup, which is the canonical AB hot-path TT probe — good place to measure practical hit rates.
Per-thread counters live in ThreadData (per-context, single-threaded claim). The increments are guarded by if (thrp) and only increment small integer counters: very cheap.
The trans-table adds/overwrite/harvest counters are exposed via new virtuals so the solver can include them in printed summaries.
A small per-solve reset ensures stats are per-board/solve rather than accumulating across many boards — the PR mentions fixing an earlier per-solve reset bug, and I see reset_op_stats() called at start.

Other changes / notable snippets

library/src/ab_search.cpp: increments lookup and hit counters inside apply_ab_tt_lookup right after the TT lookup is done.
library/src/solver_if.cpp:
    Includes added: <cstdlib> and <cstdio>.
    Reset of thrp->tt_* counters and a call to tt->reset_op_stats() at the start of solve functions.
    Printing code gated on getenv("DDS_PRINT_TT_STATS") == "1" and using std::fprintf(stderr, ...) to print counts and rates; uses get_op_stats to print adds/overwrites/harvests and compute overwrite rate.
thread_data.hpp: adds uint64_t counters and includes <cstdint>.
trans_table.*:
    trans_table.hpp: adds virtual get_op_stats and reset_op_stats in the interface.
    trans_table_l.hpp/.cpp: implements counters num_adds_ and num_overwrites_, increments them in create_or_update, and provides get/reset implementations.
    trans_table_s.hpp: returns zeros (not instrumented) in get_op_stats/reset_op_stats.
tests: MockTransTable updated to implement the new virtuals (so tests compile).

Merge readiness and risk assessment

Overall status: Low risk. The changes are instrumentation-only and gated at runtime for printing. The PR updates virtual interface and implementations (minor API addition that the repo already adjusts where needed).
Code quality: Clear and small changes; tests updated to compile.
Test coverage: The unit test mock was updated; I don't see new tests asserting counters, but compilation is handled.
Blockers: none critical found in diffs.

Critical/code-anchored review items

library/src/solver_if.cpp: Use a portable printf format for uint64_t counters (avoid hardcoding %llu); prefer PRIu64 from <inttypes.h> or cast to unsigned long long consistently across platforms and document the choice.
library/src/solver_if.cpp: The fprintf format uses %llu but casts to (unsigned long long) — consider including <inttypes.h> and using PRIu64 for clarity/portability, or add a small comment explaining the cast.
library/src/solver_if.cpp: The getenv check uses std::string(env) == "1" which allocates a string; consider comparing env[0] == '1' && env[1] == '\0' to avoid allocation in hot tooling code (micro-optimization).
library/src/trans_table/trans_table_l.cpp: The create_or_update instrumentation increments num_adds_ and conditionally num_overwrites_. Please add a short comment that documents the overwrite condition (n == BlocksPerEntry) so future readers understand why that signifies an overwrite.
library/src/ab_search.cpp: The tt counter increments assume per-thread single ownership — ensure apply_ab_tt_lookup cannot be called concurrently for the same ThreadData from multiple threads; if that is guaranteed elsewhere, add a brief comment to justify non-atomic counters.

Possible improvements / suggestions (non-blocking)

library/src/solver_if.cpp: Prefer using the project's logging API (if one exists) rather than fprintf(stderr, ...) so the output is consistent with other diagnostics and can be filtered/redirected more easily. If there's no logging facility, add a one-line justification for using stderr directly.
library/src/solver_if.cpp & thread_data.hpp: Consider encapsulating per-solve stat reset into a small helper method (e.g., ThreadData::ResetTTStats()) to avoid repeating thrp->tt_lookup_count = 0; thrp->tt_hit_count = 0; in multiple locations.
library/src/trans_table/trans_table_l.hpp/.cpp: Consider making the add/overwrite counters 64-bit if the trans-table is used in very long runs (probably unnecessary for per-solve stats, but safe).

Small style/consistency nits (purely optional)

The new includes (<cstdlib>, <cstdio>) are fine; just verify include ordering is consistent with project style.
When printing percentages, consider guarding divide-by-zero defensively (the code already checks lookup_count > 0 before computing hit_rate — good).

Summary / verdict

This PR is well-scoped, low-risk, and useful for performance investigation. The only actionable changes I recommend before merging are the portability/formatting nit around printing uint64_t and a couple of clarifying comments. None of the findings are blocking for merging.

[wopdevries]

All previous Copilot review comments have been addressed. Could a maintainer request a new Copilot review?

[Copilot]

🟡 Changes recommended

Unresolved moderate findings affect per-solve reporting and supported small-table statistics.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (7)

Previously missed (5) — in code that hasn't changed since the last review.

library/src/solver_if.cpp:293

• The reset is below the cardCount <= 4 early-exit branch, which jumps directly to SOLVER_DONE. On a persistent worker context, solving a short board after a normal board therefore prints the previous board's lookup/hit/op totals as if they belonged to the short board. Move the reset before that early exit so the env-gated report remains per-board.
  library/src/solver_if.cpp:966
• This resets the counters for analyse_later_board, but that function returns without any matching DDS_PRINT_TT_STATS summary. Consequently every post-lead analysis discards its TT lookup/hit totals, so enabling the new instrumentation does not report this instrumented search path. Add the summary on this return path (preferably shared with the other two) or avoid resetting counters here.
  library/src/trans_table/trans_table_s.hpp:309
• TransTableS is a supported TT implementation and its lookup/add paths still perform cache insertions, but this override always reports zero operation counts. The new summary prints these values without identifying them as unsupported, so DDS_PRINT_TT_STATS=1 with a small TT falsely reports no adds (and zeroes the other counters). Instrument this implementation too, or suppress/label these fields for TransTableS.
  library/src/ab_search.cpp:64
• The existing ab_search/tt_lookup_test.cpp covers this helper's hit and miss paths, but no test asserts the new ThreadData counters. A regression in these increments (or in the per-solve reset) would leave functional tests green while making the reported hit rate wrong; add assertions for a miss, a hit, and reset between solves.
  library/src/trans_table/trans_table_l.hpp:480
• The existing Print summary suit statistics comment is now immediately followed by reset_op_stats, so the generated class documentation describes the reset method as the summary printer and leaves print_summary_suit_stats undocumented. Give each new counter method its own brief and move this brief to the summary declaration.

library/src/solver_if.cpp:683

• The report is suppressed when tt_lookup_count is zero, so valid short/last-trick solves emit no DDS_TT_STATS output even with DDS_PRINT_TT_STATS=1. The documented SOLVER_DONE summary should still print zero-valued lookup/hit rates and operation counters; handle the zero denominator instead of skipping the whole report (in both duplicated report blocks).

  if (auto* env = std::getenv("DDS_PRINT_TT_STATS"); env && env[0] == '1' && env[1] == '\0') {
    ThreadData* thrp_ptr = ctx.thread_ptr();
    if (thrp_ptr && thrp_ptr->tt_lookup_count > 0) {

library/src/solver_if.cpp:13

• These adjacent includes are identical; retaining both is redundant and makes include hygiene harder to maintain. Remove the second <cinttypes> include.

#include <cinttypes>

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Lite

[wopdevries]

@tameware
Items #1 (duplicate include) and #4 (zero division) are fixed in the latest commit.

For the 5 previously-missed comments: items on Doxygen (#5) and early-exit reset (#1-solver_if:293) I can fix in this PR. The TransTableS instrumentation (#3), unit tests (#4-ab_search), and analyse_later_board path (#2) could be follow-up PRs to keep this one focused on the basic instrumentation.

Would you prefer I fix all 7 in this PR, or merge what's working and address the rest separately?