Improve parallelism by solving most difficult deals first#216
Improve parallelism by solving most difficult deals first#216tameware wants to merge 16 commits into
Conversation
tameware
commented
Jun 29, 2026
tameware
commented
The heuristic/quick-tricks refactor introduced static_cast<unsigned char>
wrappers on values that v2.9 used as signed, changing search behavior:
- make_3 / make_3_ctx: winner[]/second_best[] .hand and .rank were cast
to unsigned char, turning the -1 "no card" sentinel into 255. This broke
winner[trump].hand == -1 style checks in QuickTricks, losing cutoffs.
- weight_alloc_trump_void2 / _void3: rel_rank[aggr[suit]][...] indexed
through static_cast<unsigned char>(aggr[suit]), truncating the 13-bit
aggregate holding to 8 bits and reading the wrong rel_rank row.
- QuickTricksPartnerHand{Trump,NT}: bit_map_rank index cast the signed
rank through unsigned char.
With these reverted to v2.9's signed handling, the per-move-generation
ordering trace now matches v2.9 exactly (0 divergences on list1), closing
the residual calc gap to parity. Ordering/pruning-only change; double-dummy
results are unchanged and all library tests pass.
Co-authored-by: Cursor <cursoragent@cursor.com>
The parallel board loop handed boards out in index order via an atomic counter, so a hard board picked near the end left one worker running long while the others sat idle. Hand out the hardest boards first (longest- processing-time-first) so the tail consists of cheap boards. parallel_all_boards_n gains an optional dispatch-order permutation: workers still pull from the same atomic counter, but the slot is mapped through the order before becoming a board number, so only the dispatch sequence changes and result placement is unaffected. The solve path passes no order and is unchanged. calc estimates per-deal difficulty with a cheap, trump-independent structural proxy (deal_fanout, mirroring Scheduler::Fanout) and sorts board indices by descending difficulty before dispatch. calc list1000 -n18: ~11.0s -> ~9.6s wall (~13%), user CPU unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
CalcDDtableN builds one board per strain for a single deal. deal_fanout is trump-independent, so all boards share one fanout and the difficulty sort is a pure no-op there. Gate the sort behind a difficulty_sort flag (default on for batch CalcAllTablesN) and disable it for the single-deal path. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
This PR aims to improve overall throughput for batch calculations by reducing “tail latency” in parallel workloads: it estimates deal difficulty cheaply, sorts boards hardest-first, and adds an optional dispatch-order mechanism to the parallel board runner.
Changes:
- Extend
parallel_all_boards_n()to optionally dispatch boards in a caller-provided order. - Add a cheap per-deal “fanout” estimate and use it to stable-sort batch
calcboards hardest-first before parallel execution. - Simplify/remove several legacy
static_cast<unsigned char>(...)conversions in solver/heuristic code paths.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| library/src/system/parallel_boards.hpp | Adds optional order parameter to control dispatch order (hardest-first, etc.). |
| library/src/system/parallel_boards.cpp | Implements ordered dispatch via slot→board mapping. |
| library/src/calc_tables.cpp | Computes per-deal difficulty estimate and dispatches hardest boards first for batch calc. |
| library/src/heuristic_sorting/heuristic_sorting.cpp | Cleans up heuristic code (including rel-rank indexing casts) and adjusts some void/trump logic. |
| library/src/quick_tricks.cpp | Removes redundant casts when indexing with abs_rank[..].rank. |
| library/src/ab_search.cpp | Removes redundant casts when copying abs_rank winner/second-best into Pos. |
Only honor the optional dispatch order when it is a valid permutation of [0, count: each element in range and unique. A malformed order (duplicates or out-of-range values) now falls back to index order, preventing invalid board indices from reaching process_board. EOF ) Co-authored-by: Cursor <cursoragent@cursor.com>
Use std::ranges::set_intersection and std::views::iota instead of a hand-rolled seen bitmap in is_permutation_of_range. Co-authored-by: Cursor <cursoragent@cursor.com>
Emscripten's libc++ lacks C++20 ranges; use iterator-based sort, iota, and set_intersection instead. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Scheduler: :Fanout is private; move the fanout logic to a shared deal_fanout() helper so calc_tables can use it without accessing the scheduler instance. Co-authored-by: Cursor <cursoragent@cursor.com>
|
|
||
| int res = calc_all_boards_n(&bo, &solved, maxThreads); | ||
| // Single deal: all boards share one deal, so hardest-first sorting is a no-op. | ||
| int res = calc_all_boards_n(&bo, &solved, maxThreads, /*difficulty_sort=*/false); |
There was a problem hiding this comment.
trivial
Inline variable-name comment isn't necessary.
There was a problem hiding this comment.
Why not? It seems useful to me. Were I writing the code I might have replaced it with a named constant, say DIFFICULTY_SORT_FALSE.
|
When reading doc/2014-11 Performance and Benchmarking.docx I learned that the Scheduler should be doing this already. I asked Cursor about it. It replied: The 2014 doc is right about what the Scheduler was designed to do, but at the time of What the commit changedBefore const int slot = next.fetch_add(1, std::memory_order_relaxed);
if (slot >= count || first_error.load(std::memory_order_relaxed) != RETURN_NO_FAULT)
{
break;
}
const int bno = board_of(slot);
Why that improved wall time (~13%) without changing CPUThis is classic longest-processing-time-first (LPT) scheduling for a thread pool. The 2014 benchmarking doc describes exactly this failure mode: solve times are highly skewed (roughly Weibull), so with work-stealing in index order one thread can draw a very hard board near the end and sit alone while the others idle. Wall time is dominated by that tail, not by total CPU. The commit’s benchmark confirms that story:
So no less work was done — boards were just started in a better order so threads finish closer together. “But the Scheduler already sorts hardest first…”Yes — in the v2.8 design, documented in the 2014 paper. The Scheduler:
That old path looked like: st = scheduler.GetNumber(thr_id);
index = st.number;But on the main line at
There was a side branch ( So the doc describes the intended v2.8 behavior; How
|
|
I see now that a71b01a was mine! I think I abandoned it because I mistakenly thought it degraded performance. That was before I'd vibe-coded benchmark.sh. I cherrypicked a71b01a on top of the develop head and ran benchmark.sh, averaging over three runs. It shows timing close between the two approaches: |
zzcgumn
left a comment
zzcgumn
left a comment
There was a problem hiding this comment.
I think we go with this pull request for sorting batches to start with the most difficult hand to solve.
|
OK! I'll make the changes suggested by tzimnoch, including removing the less-efficient STL use. |
Keep the structural difficulty estimate with the data it evaluates, allowing scheduler and batch calculation code to share it without exposing scheduler internals. Co-authored-by: Cursor <cursoragent@cursor.com>
Use the seen bitmap again to validate dispatch orders with one allocation and linear work while preserving malformed-order fallback behavior. Co-authored-by: Cursor <cursoragent@cursor.com>
Clarify that parallel workers retain read access to the optional order vector for the duration of the call. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Latest benchmark with this PR: Calc for 1,000 unique deals (so 20,000 solves) is 9% faster than the develop head, 9% slower than v 2.9. |
Remove the C++ method from the public C API struct and expose the shared fanout calculation only through an internal system helper. Co-authored-by: Cursor <cursoragent@cursor.com>
Make the test replacement for operator new conform across platforms where malloc(0) may return null. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep calc_tables decoupled from lookup-table implementation details now owned by the internal fanout helper. Co-authored-by: Cursor <cursoragent@cursor.com>
| const bool use_order = | ||
| (order != nullptr && | ||
| static_cast<int>(order->size()) == count && | ||
| is_permutation_of_range(*order, count)); |
| int fanout = 0; | ||
| int fanoutSuit, numVoids, c; | ||
|
|
||
| for (int h = 0; h < DDS_HANDS; h++) | ||
| { | ||
| fanoutSuit = 0; | ||
| numVoids = 0; | ||
| for (int s = 0; s < DDS_SUITS; s++) | ||
| { | ||
| c = static_cast<int>(dl.remainCards[h][s] >> 2); | ||
| fanoutSuit += group_data[c].last_group_ + 1; | ||
| if (c == 0) | ||
| numVoids++; | ||
| } | ||
| fanoutSuit += numVoids * fanoutSuit; | ||
| fanout += fanoutSuit; | ||
| } |
