Fable heuristic inline by tameware · Pull Request #263 · dds-bridge/dds

tameware · 2026-07-23T15:21:20Z

Fable found this performance improvement by profiling v2.9 and the current develop head. It helps us get closer to v2.9 performance.

$ ./benchmark.sh --binary $DTEST29 --branch develop --branch . --max_deals 100 --repeats 3 
Building dtest from 'develop'...
Building dtest from 'fable-heuristic-inline'...
Restoring 'fable-heuristic-inline'...
DDS dtest benchmark
===================
baseline:    dtest  (/Users/adamw/src/bridge-hackathon/dds/libdds/.build/test/dtest)
binary 2:    develop  (/var/folders/12/xtx6dlwd0mdcxkspvmsszsrc0000gn/T//dds-dtest-bin.XkIbqf)
binary 3:    fable-heuristic-inline  (/var/folders/12/xtx6dlwd0mdcxkspvmsszsrc0000gn/T//dds-dtest-bin.r1S0m3)
details:     off (summary only)
run order:   interleaved dtest, develop, fable-heuristic-inline
hands dir:   /Users/adamw/src/dds/hands
max_deals:   100
files:       list100.txt list10.txt list1.txt
git branch:  fable-heuristic-inline
repeats:     3


Summary (avg user ms)
==============================================================================
solver file                 dtest      develop fable-heuris
------ ------------- ------------ ------------ ------------
solve  list100.txt           1.71         1.84         1.78
solve  list10.txt            4.00         3.97         3.90
solve  list1.txt            11.17        11.00        11.00
calc   list100.txt           7.20         7.55         7.45
calc   list10.txt           13.21        13.83        13.47
calc   list1.txt            37.15        38.33        37.67
------ ------------- ------------ ------------ ------------
TOTAL  elapsed (s)           3.68         3.94         3.79

Completed 54 runs (54 expected).

Profiles showed call_heuristic as the second-hottest function: every per-suit call rebuilt the full HeuristicContext and re-derived hand_rel, trump state and voidness that MoveGen0/MoveGen123 already knew. The context is now built once per move-generation call and dispatched on the caller's findex. Solve drops from 16.97 to 15.09 ms/hand single-threaded on list1000 (2.9: 16.43); calc from 81.94 to 73.2 ms/hand on list100. Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot

Pull request overview

This PR optimizes heuristic sorting dispatch by hoisting HeuristicContext construction and passing a precomputed dispatch index (findex) into a new call_heuristic(ctx, findex) overload, reducing repeated per-call recomputation in move generation’s hot paths.

Changes:

Adds a call_heuristic(const HeuristicContext&, int findex) overload with a direct dispatch switch.
Hoists HeuristicContext construction in Moves::MoveGen0 / Moves::MoveGen123 via Moves::make_heuristic_context() and passes precomputed findex.
Adds a new GoogleTest to verify findex-based dispatch is equivalent to legacy dispatch for all reachable findex values.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
library/src/moves/moves.hpp	Replaces per-call heuristic invocation with `make_heuristic_context()` API for hoisted context construction.
library/src/moves/moves.cpp	Hoists heuristic context construction and uses new `findex` overload for hot-path dispatch.
library/src/heuristic_sorting/heuristic_sorting.hpp	Makes `suit` mutable and declares new `call_heuristic(ctx, findex)` overload.
library/src/heuristic_sorting/heuristic_sorting.cpp	Implements `findex`-based dispatcher and refactors legacy overload to delegate to it.
library/tests/heuristic_sorting/dispatch_findex_test.cpp	Adds equivalence tests for legacy vs `findex` dispatch across reachable indices.
library/tests/heuristic_sorting/BUILD.bazel	Registers the new `dispatch_findex_test` target.

Clarify that bit 0/1 means a trump winner is available, not merely a trump game, and pin the edge case with a dispatch equivalence test. Co-authored-by: Cursor <cursoragent@cursor.com>

Avoid dereferencing the nullable Moves::trackp member so misuse is a compile-time error instead of undefined behavior. Co-authored-by: Cursor <cursoragent@cursor.com>

An out-of-range findex previously left stale move weights untouched, which could make MergeSort ordering nondeterministic. Co-authored-by: Cursor <cursoragent@cursor.com>

into fable-heuristic-inline

Copilot

Pull request overview

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

The helper reads lead/curr/trump/suit/move-count members that the constructor leaves uninitialized; set them so the snapshot test avoids UB. Co-authored-by: Cursor <cursoragent@cursor.com>

Avoid ~1MB of per-fixture stack for a zero-initialized table that tests only need by pointer. Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

…uristic. Match the legacy dispatcher’s suit-range check before indexing winner[trump] in MoveGen, and drop const_cast by making weighting overloads non-const. Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

library/src/moves/moves.cpp:267

MoveGen123 hoists make_heuristic_context() but does not initialize lastNumMoves/suit beforehand. This makes make_heuristic_context() read indeterminate member values (undefined behavior). Initialize these members before creating the context.

  for (int s = 0; s < DDS_SUITS; s++)
    trackp->lowest_win[handRel][s] = 0;
  numMoves = 0;

  int findex;

make_heuristic_context snapshots suit/lastNumMoves/leadSuit; define them in the constructor and reset them in MoveGen0/123 so the hoist does not read indeterminate values. Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Align the behaviour bullet with the mutable HeuristicContext& API and the legacy vs findex dispatch forms. Co-authored-by: Cursor <cursoragent@cursor.com>

The previous 0x3fff-everywhere fixture wrote past MovePlyType::move[14] and failed under ASan. Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

library/src/moves/moves.cpp:732

make_heuristic_context() unconditionally snapshots tr.move[1], tr.move[2], and tr.high[...]. For leading-hand generation (MoveGen0) and for second hand in a trick (hand_rel==1), those track slots may not have been populated yet (Init() doesn’t initialize them), so this risks reading indeterminate values (UB) just to populate the snapshot fields.

A safer approach is to snapshot only the trick-card fields that are defined for the current hand_rel (and leave the others as 0, matching the default member initializers).

  // Snapshot removed_ranks and minimal trick state into the context to avoid
  // direct dependence on the mutable Moves::trackp buffer inside heuristic
  // code.
  for (int s = 0; s < DDS_SUITS; ++s)
    context.removed_ranks[s] = tr.removed_ranks[s];
  context.move1_rank = tr.move[1].rank;
  context.high1 = tr.high[1];
  context.move1_suit = tr.move[1].suit;
  context.move2_rank = tr.move[2].rank;
  context.move2_suit = tr.move[2].suit;
  context.high2 = tr.high[2];
  context.lead0_rank = tr.move[0].rank;

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Gate lead/move1/move2 copies on hand_rel so MoveGen0 and early following hands do not read unpopulated TrackType slots. Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot

Pull request overview

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

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Locals in context_for_hand_rel died on return while HeuristicContext kept references to them, leaving dangling pointers in the snapshot. Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot

Pull request overview

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

zzcgumn · 2026-07-24T07:52:27Z

+/// @note Prefer the `call_heuristic(context, findex)` overload when the dispatch
+///       index is already known (hot path). Use this overload when callers do
+///       not have `findex` available.
+void call_heuristic(HeuristicContext& context);


As mentioned above, we want to refactor any call sites that don't pre-calculate findex. Maybe change this method to return findex to avoid making two calls, one to get a findex and another to sort.

zzcgumn · 2026-07-24T08:00:04Z

This looks promising. Memory serves me correctly 2.9 had an array of pointers to functions which was very hard to understand and test. It looks like findex avoids repeating the calculation in a way that allows for unit testing. I am also mindful that findex is probably short for function index, maybe we can find a more descriptive name?

There probably is a good reason to not include findex in the HeuristicContext, but I think it is worth documenting why this design decision was made.

Move generation already passes a precomputed findex everywhere; drop the unused context-only API and the equivalence tests that depended on it. Co-authored-by: Cursor <cursoragent@cursor.com>

Documentation for the method already lives in moves.hpp. Co-authored-by: Cursor <cursoragent@cursor.com>

tameware and others added 2 commits July 23, 2026 12:51

Merge branch 'dds-bridge:develop' into fable-heuristic-inline

93dd14c

tameware requested a review from Copilot July 23, 2026 15:21

tameware self-assigned this Jul 23, 2026

Copilot started reviewing on behalf of tameware July 23, 2026 15:21 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread library/src/heuristic_sorting/heuristic_sorting.hpp Outdated

Comment thread library/src/moves/moves.cpp Outdated

Comment thread library/src/heuristic_sorting/heuristic_sorting.cpp

tameware and others added 4 commits July 23, 2026 17:39

Fix findex encoding docs to match trump-winner semantics.

a6e6d34

Clarify that bit 0/1 means a trump winner is available, not merely a trump game, and pin the edge case with a dispatch equivalence test. Co-authored-by: Cursor <cursoragent@cursor.com>

Require an explicit TrackType in make_heuristic_context.

cfc12ba

Avoid dereferencing the nullable Moves::trackp member so misuse is a compile-time error instead of undefined behavior. Co-authored-by: Cursor <cursoragent@cursor.com>

Fall back to NT0 weights for invalid findex dispatch.

29730d0

An out-of-range findex previously left stale move weights untouched, which could make MergeSort ordering nondeterministic. Co-authored-by: Cursor <cursoragent@cursor.com>

Merge branch 'fable-heuristic-inline' of https://github.com/tameware/dds

a9524fb

into fable-heuristic-inline

tameware requested a review from Copilot July 23, 2026 15:48

Copilot started reviewing on behalf of tameware July 23, 2026 15:48 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread library/tests/moves/moves_test.cpp Outdated

Comment thread library/tests/heuristic_sorting/dispatch_findex_test.cpp

tameware and others added 2 commits July 23, 2026 19:14

Initialize Moves fields before make_heuristic_context in tests.

f08a1e1

The helper reads lead/curr/trump/suit/move-count members that the constructor leaves uninitialized; set them so the snapshot test avoids UB. Co-authored-by: Cursor <cursoragent@cursor.com>

Store RelRanksType table statically in dispatch fixture tests.

e7ad330

Avoid ~1MB of per-fixture stack for a zero-initialized table that tests only need by pointer. Co-authored-by: Cursor <cursoragent@cursor.com>

tameware requested a review from Copilot July 23, 2026 17:16

Copilot started reviewing on behalf of tameware July 23, 2026 17:17 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Guard out-of-range trump and take mutable HeuristicContext in call_he…

933d495

…uristic. Match the legacy dispatcher’s suit-range check before indexing winner[trump] in MoveGen, and drop const_cast by making weighting overloads non-const. Co-authored-by: Cursor <cursoragent@cursor.com>

tameware requested a review from Copilot July 23, 2026 18:10

Copilot started reviewing on behalf of tameware July 23, 2026 18:10 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread library/src/moves/moves.cpp

Initialize Moves scalars before hoisting heuristic context.

631cb55

make_heuristic_context snapshots suit/lastNumMoves/leadSuit; define them in the constructor and reset them in MoveGen0/123 so the hoist does not read indeterminate values. Co-authored-by: Cursor <cursoragent@cursor.com>

tameware requested a review from Copilot July 23, 2026 18:27

Copilot started reviewing on behalf of tameware July 23, 2026 18:28 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread specs/heuristic-sorting.md

tameware and others added 2 commits July 23, 2026 20:47

Document both call_heuristic overloads in the heuristic-sorting spec.

3d3b62e

Align the behaviour bullet with the mutable HeuristicContext& API and the legacy vs findex dispatch forms. Co-authored-by: Cursor <cursoragent@cursor.com>

Use a compact deal in MoveGen tests to avoid move-list overflow.

358c1ca

The previous 0x3fff-everywhere fixture wrote past MovePlyType::move[14] and failed under ASan. Co-authored-by: Cursor <cursoragent@cursor.com>

tameware requested a review from Copilot July 23, 2026 18:57

Copilot started reviewing on behalf of tameware July 23, 2026 18:58 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread library/src/heuristic_sorting/heuristic_sorting.hpp Outdated

tameware and others added 2 commits July 23, 2026 20:05

Potential fix for pull request finding

e657e50

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Snapshot only defined trick-card fields in make_heuristic_context.

d4e87f6

Gate lead/move1/move2 copies on hand_rel so MoveGen0 and early following hands do not read unpopulated TrackType slots. Co-authored-by: Cursor <cursoragent@cursor.com>

tameware requested a review from Copilot July 23, 2026 20:03

Copilot started reviewing on behalf of tameware July 23, 2026 20:04 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

tameware marked this pull request as ready for review July 23, 2026 20:23

tameware requested review from Copilot and zzcgumn July 23, 2026 20:23

Copilot started reviewing on behalf of tameware July 23, 2026 20:24 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread library/tests/moves/moves_test.cpp

Bind HeuristicContext test refs to static storage.

a6329c5

Locals in context_for_hand_rel died on return while HeuristicContext kept references to them, leaving dangling pointers in the snapshot. Co-authored-by: Cursor <cursoragent@cursor.com>

tameware requested a review from Copilot July 24, 2026 02:26

Copilot started reviewing on behalf of tameware July 24, 2026 02:27 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

zzcgumn reviewed Jul 24, 2026

View reviewed changes

Comment thread library/src/heuristic_sorting/heuristic_sorting.cpp Outdated

zzcgumn reviewed Jul 24, 2026

View reviewed changes

Comment thread library/src/moves/moves.cpp Outdated

tameware and others added 2 commits July 24, 2026 10:38

Remove legacy call_heuristic overload that re-derives findex.

d87f771

Move generation already passes a precomputed findex everywhere; drop the unused context-only API and the equivalence tests that depended on it. Co-authored-by: Cursor <cursoragent@cursor.com>

Drop duplicate make_heuristic_context doxygen from the .cpp.

bd361a8

Documentation for the method already lives in moves.hpp. Co-authored-by: Cursor <cursoragent@cursor.com>

Uh oh!

Conversation

tameware commented Jul 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

zzcgumn Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zzcgumn commented Jul 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants