Improve dtest -r per-deal timing report

[tameware]

Summary

• Add column headings and right-aligned ms / board columns for dtest -r (space-padded, no tabs).
• Accumulate timings across every solve batch so reports cover all input deals, not only the last MAXNOOFBOARDS chunk; board is the 0-based file deal index.
• Measure in microseconds and print milliseconds with one decimal place; tighten blank lines around the report title.

Test plan

• bazelisk test //library/tests:report_board_timings_test
• bazelisk run //library/tests:dtest -- -f 100 -r and confirm every deal appears with aligned columns
• Spot-check a file larger than 200 deals so multi-batch accumulation is visible

Made with Cursor

[Copilot]

Pull request overview

This PR improves dtest -r/--report per-deal timing reporting by (1) collecting timings across all solve batches (so large input files are fully covered) and (2) printing a more readable, space-aligned table with column headings and millisecond values to one decimal.

Changes:

• Add report_board_timings helper module to format per-board timing output with aligned ms / board columns.
• Accumulate per-batch scheduler timings into a file-wide list so -r covers every deal, not just the final MAXNOOFBOARDS chunk.
• Switch lightweight per-board timing storage to microseconds and print in milliseconds with one decimal place.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
library/tests/testcommon.cpp	Accumulates per-deal timings during solve runs and prints the consolidated report.
library/tests/report_board_timings.hpp	Declares formatting + accumulation helpers for dtest -r output.
library/tests/report_board_timings.cpp	Implements sorting + column-width calculation and printing logic for the report.
library/tests/report_board_timings_test.cpp	Adds unit tests covering formatting, alignment, sorting, and batch accumulation.
library/tests/loop.hpp	Extends loop_solve API to optionally append per-deal timings across batches.
library/tests/loop.cpp	Collects scheduler per-board times after each batch and appends with file offsets.
library/tests/BUILD.bazel	Adds standalone report_board_timings_test target and excludes it from the shared test source glob.
library/tests/args.cpp	Updates -r/--report usage text to reflect per-deal ms output and scope.
library/src/system/scheduler.hpp	Updates comments/parameter naming to reflect microsecond timing.
library/src/system/scheduler.cpp	Renames SetBoardTime parameter and stores microsecond values in the timing field.
library/src/solve_board.cpp	Measures per-board durations in microseconds and records them via Scheduler::SetBoardTime.

---

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

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

Suppressed comments (1)

library/tests/args.cpp:113

• The usage text relies on implicit concatenation of adjacent string literals because the << is missing after the first line. This is easy to miss and inconsistent with the rest of the usage formatting (every other line uses <<), so it’s better to keep the explicit stream insertion operator for readability.

    "-r, --report       Print per-deal timings in ms (one decimal) for every\n"
    "                   hand in the input (solve mode), longest first.\n" <<

[zzcgumn]

Makes sense. Am I right that this is expected to make it easier for a bot to compare two branches?

[tameware]

I was thinking of humans, but I can imagine it being useful for 'bots. We now can measure the difference between the easiest and most difficult deal in a collection.

[tameware]

Addressed the suppressed Copilot note on library/tests/args.cpp: added the missing << between the two -r/--report usage string literals for consistency with the rest of usage().

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

Suppressed comments (1)

library/tests/testcommon.cpp:117

• When -r is enabled, board_times will accumulate one entry per deal across all batches. Reserving capacity upfront avoids repeated reallocations for large input files (especially the multi-batch >MAXNOOFBOARDS case this PR targets).

  std::vector<std::pair<int, int>> board_times;

[tameware]

Addressed the suppressed Copilot note on library/tests/testcommon.cpp: when -r is enabled, board_times now reserves one slot per input deal before multi-batch accumulation.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

Suppressed comments (2)

library/tests/loop.hpp:14

• In loop.hpp, standard library headers should come before project headers. With the new / includes, the current order places <api/dll.h> first, which is inconsistent with the rest of the codebase’s include ordering and can interfere with include-order tooling.

#include <api/dll.h>
#include <utility>
#include <vector>

library/src/system/scheduler.hpp:221

• Scheduler timing values are now documented/treated as microseconds, but the API/storage types are still int (e.g., SetBoardTime(int time_us) and std::pair<int,int> in GetBoardTimes). Microsecond counts can exceed INT_MAX (~35 minutes) and overflow/narrow (implementation-defined) when produced by duration_cast<microseconds>().count() and later cast to int (see solve_board.cpp), which can corrupt the report for very slow/hung boards.

   * Fills outVec with pairs (boardIndex, userTimeUs) for each board in
   * the current run. Times are wall-clock microseconds. This is intended
   * for post-run reporting.
   */
  void GetBoardTimes(std::vector<std::pair<int,int>>& outVec) const;

  // Lightweight API to set a board's time in microseconds for reporting when
  // full DDS_SCHEDULER timing is not enabled. Thread-safe for single-writer per-board.
  void SetBoardTime(int boardIndex, int time_us);

[tameware]

Addressed the latest suppressed Copilot notes:

1. loop.hpp include order — fixed: <utility> / <vector> now precede <api/dll.h>.
2. int microsecond overflow — dismissed: INT_MAX µs is ~35 minutes for a single board, which is outside realistic dtest -r use, and HandType::time / the surrounding APIs are already int end-to-end. Widening that storage would be a broader API change than this PR’s reporting polish.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

Suppressed comments (1)

library/tests/loop.cpp:20

• The new standard-library includes (, ) are placed after project headers, which makes the include blocks harder to scan and differs from the rest of this file’s standard-then-project header grouping. Move these standard headers up with the other <...> includes and keep a blank line between standard and project includes.

#include "loop.hpp"
#include "TestTimer.hpp"
#include "compare.hpp"
#include "print.hpp"
#include <vector>
#include <utility>

[tameware]

Addressed the suppressed Copilot note on library/tests/loop.cpp: moved <utility> / <vector> up with the other standard includes (before the project headers).

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

Suppressed comments (6)

library/tests/report_board_timings.hpp:29

• Indentation for wrapped parameters should match the 4-space continuation style used elsewhere in library/tests headers (e.g., args.hpp, loop.hpp).

void append_batch_board_times(
  std::vector<std::pair<int, int>>& accumulated,
  const std::vector<std::pair<int, int>>& batch_times,
  int file_offset);

library/tests/report_board_timings.cpp:87

• Same indentation issue for wrapped parameters here; match the 4-space continuation indentation used elsewhere.

void append_batch_board_times(
  std::vector<std::pair<int, int>>& accumulated,
  const std::vector<std::pair<int, int>>& batch_times,
  int file_offset)

library/src/solve_board.cpp:93

• Casting microsecond durations to int can overflow for long-running boards (around 35 minutes+), which can wrap negative and produce incorrect timings. Clamp/saturate before the cast so reporting stays correct even in extreme cases.

      auto dur = std::chrono::duration_cast<std::chrono::microseconds>(
        std::chrono::steady_clock::now() - t0).count();
      if (dur < 0) dur = 0;
      scheduler.SetBoardTime(bno, static_cast<int>(dur));

library/tests/report_board_timings.hpp:19

• Wrapped parameters are indented by 2 spaces here, but other headers in library/tests (e.g., args.hpp, loop.hpp) indent continuation lines by 4 spaces. Aligning this keeps formatting consistent across the test codebase.

This issue also appears on line 26 of the same file.

void print_per_board_timings(
  std::ostream& out,
  std::vector<std::pair<int, int>> times);

library/tests/report_board_timings.cpp:57

• Function parameter continuation indentation is 2 spaces here; most C++ files in this repo use 4-space indentation for wrapped parameters. Adjusting to 4 keeps formatting consistent.

This issue also appears on line 84 of the same file.

void print_per_board_timings(
  std::ostream& out,
  std::vector<std::pair<int, int>> times)
{

library/src/solve_board.cpp:286

• Same potential overflow here: converting a microsecond count to int without bounds checking can wrap and yield negative/incorrect per-board timings. Clamp/saturate before casting to int.

    auto dur = std::chrono::duration_cast<std::chrono::microseconds>(
      std::chrono::steady_clock::now() - t0).count();
    if (dur < 0) dur = 0;
    scheduler.SetBoardTime(bno, static_cast<int>(dur));

[tameware]

Addressed the latest suppressed Copilot notes:

1. Wrapped-parameter indentation in report_board_timings.hpp / .cpp — now 4-space continuation, matching args.hpp / loop.hpp.
2. Microsecond → int overflow — SetBoardTime now takes long long and saturates via saturate_board_time_us (covered by scheduler_board_time_test); both solve_board.cpp call sites pass the raw duration through.

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.