Improve parallelism by solving most difficult deals first#216
Improve parallelism by solving most difficult deals first#216tameware wants to merge 11 commits into
Conversation
tameware
commented
Jun 29, 2026
tameware
commented
The heuristic extraction refactor changed weight_alloc_trump_void1's first branch from `lead_suit == trump` to `suit == trump`. Since that is exhaustive with the following `else if (suit != trump)`, the three ruffing branches (using the `24 - rank + ...` formula) became dead code, and trump ruffs were scored with side-suit discard weights instead. This mis-ordered ruffs, costing alpha-beta cutoffs. The effect is small for solve but compounds heavily in calc's warm-TT iterative deepening: calc explored ~34% more nodes than v2.9. Restoring the original `lead_suit == trump` pitch branch makes the ruffing branches reachable again and cuts calc time ~25% (gap to v2.9: 1.37x -> 1.02x). Ordering-only change; double-dummy results are unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
Per Copilot. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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>
| * @param order Optional dispatch order: a permutation of [0, count) giving the | ||
| * sequence in which board numbers are handed out (e.g. hardest first to | ||
| * shorten the tail). When null/empty, boards are dispatched in index | ||
| * order. Only the dispatch order changes; @p process_board still receives | ||
| * the real board number, so result placement is unaffected. | ||
| * @return First non-success code from @p process_board, or RETURN_NO_FAULT. |
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>
| #include <solve_board.hpp> | ||
| #include <api/solve_board.hpp> | ||
| #include <solver_if.hpp> | ||
| #include <lookup_tables/lookup_tables.hpp> |
There was a problem hiding this comment.
I should probably pull and compile, but I don't see what's being used from this header. fanout comes from scheduler.hpp and all the other called functions appear to be in std:: namespace. What did I miss?
|
|
||
| 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.
| { | ||
| return deal_fanout(dl); | ||
| } | ||
|
|
||
|
|
||
| auto deal_fanout(const Deal& dl) -> int |
There was a problem hiding this comment.
I don't have any skin in this, but why not move Fanout from private to public instead?
What are the advantages of exposing the function this way? (Ideological purity being a perfectly fine answer.)
| posPoint->winner[st].rank = static_cast<unsigned char>(thrp->rel[aggr].abs_rank[1][st].rank); | ||
| posPoint->winner[st].hand = static_cast<unsigned char>(thrp->rel[aggr].abs_rank[1][st].hand); | ||
| posPoint->second_best[st].rank = static_cast<unsigned char>(thrp->rel[aggr].abs_rank[2][st].rank); | ||
| posPoint->second_best[st].hand = static_cast<unsigned char>(thrp->rel[aggr].abs_rank[2][st].hand); | ||
| posPoint->winner[st].rank = thrp->rel[aggr].abs_rank[1][st].rank; | ||
| posPoint->winner[st].hand = thrp->rel[aggr].abs_rank[1][st].hand; | ||
| posPoint->second_best[st].rank = thrp->rel[aggr].abs_rank[2][st].rank; | ||
| posPoint->second_best[st].hand = thrp->rel[aggr].abs_rank[2][st].hand; | ||
|
|
||
| } |
There was a problem hiding this comment.
Might as well fix the indenting problem at the same time.
|
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. |
