From 87325127362ed6992ad73f6e363041014971b9b7 Mon Sep 17 00:00:00 2001 From: Henri Bourcereau Date: Fri, 6 Mar 2026 17:33:07 +0100 Subject: [PATCH 1/3] feat: command to play random games with open_spiel logic --- store/Cargo.toml | 4 + store/src/bin/random_game.rs | 262 +++++++++++++++++++++++++++++++++++ 2 files changed, 266 insertions(+) create mode 100644 store/src/bin/random_game.rs diff --git a/store/Cargo.toml b/store/Cargo.toml index a9234ff..935a2a0 100644 --- a/store/Cargo.toml +++ b/store/Cargo.toml @@ -25,5 +25,9 @@ rand = "0.9" serde = { version = "1.0", features = ["derive"] } transpose = "0.2.2" +[[bin]] +name = "random_game" +path = "src/bin/random_game.rs" + [build-dependencies] cxx-build = "1.0" diff --git a/store/src/bin/random_game.rs b/store/src/bin/random_game.rs new file mode 100644 index 0000000..6da3b9c --- /dev/null +++ b/store/src/bin/random_game.rs @@ -0,0 +1,262 @@ +//! Run one or many games of trictrac between two random players. +//! In single-game mode, prints play-by-play like OpenSpiel's `example.cc`. +//! In multi-game mode, runs silently and reports throughput at the end. +//! +//! Usage: +//! cargo run --bin random_game -- [--seed ] [--games ] [--max-steps ] [--verbose] + +use std::borrow::Cow; +use std::env; +use std::time::Instant; + +use trictrac_store::{ + training_common::sample_valid_action, + Dice, DiceRoller, GameEvent, GameState, Stage, TurnStage, +}; + +// ── CLI args ────────────────────────────────────────────────────────────────── + +struct Args { + seed: Option, + games: usize, + max_steps: usize, + verbose: bool, +} + +fn parse_args() -> Args { + let args: Vec = env::args().collect(); + let mut seed = None; + let mut games = 1; + let mut max_steps = 10_000; + let mut verbose = false; + + let mut i = 1; + while i < args.len() { + match args[i].as_str() { + "--seed" => { + i += 1; + seed = args.get(i).and_then(|s| s.parse().ok()); + } + "--games" => { + i += 1; + if let Some(v) = args.get(i).and_then(|s| s.parse().ok()) { + games = v; + } + } + "--max-steps" => { + i += 1; + if let Some(v) = args.get(i).and_then(|s| s.parse().ok()) { + max_steps = v; + } + } + "--verbose" => verbose = true, + _ => {} + } + i += 1; + } + + Args { + seed, + games, + max_steps, + verbose, + } +} + +// ── Helpers ─────────────────────────────────────────────────────────────────── + +fn player_label(id: u64) -> &'static str { + if id == 1 { "White" } else { "Black" } +} + +/// Apply a `Roll` + `RollResult` in one logical step, returning the dice. +/// This collapses the two-step dice phase into a single "chance node" action, +/// matching how the OpenSpiel layer exposes it. +fn apply_dice_roll(state: &mut GameState, roller: &mut DiceRoller) -> Result { + // RollDice → RollWaiting + state + .consume(&GameEvent::Roll { player_id: state.active_player_id }) + .map_err(|e| format!("Roll event failed: {e}"))?; + + // RollWaiting → Move / HoldOrGoChoice (or Stage::Ended if 13th hole) + let dice = roller.roll(); + state + .consume(&GameEvent::RollResult { player_id: state.active_player_id, dice }) + .map_err(|e| format!("RollResult event failed: {e}"))?; + + Ok(dice) +} + +/// Sample a random action and apply it to `state`, handling the Black-mirror +/// transform exactly as `cxxengine.rs::apply_action` does: +/// +/// 1. For Black, build a mirrored view of the state so that `sample_valid_action` +/// and `to_event` always reason from White's perspective. +/// 2. Mirror the resulting event back to the original coordinate frame before +/// calling `state.consume`. +/// +/// Returns the chosen action (in the view's coordinate frame) for display. +fn apply_player_action(state: &mut GameState) -> Result<(), String> { + let needs_mirror = state.active_player_id == 2; + + // Build a White-perspective view: borrowed for White, owned mirror for Black. + let view: Cow = if needs_mirror { + Cow::Owned(state.mirror()) + } else { + Cow::Borrowed(state) + }; + + let action = sample_valid_action(&view) + .ok_or_else(|| format!("no valid action in stage {:?}", state.turn_stage))?; + + let event = action + .to_event(&view) + .ok_or_else(|| format!("could not convert {action:?} to event"))?; + + // Translate the event from the view's frame back to the game's frame. + let event = if needs_mirror { event.get_mirror(false) } else { event }; + + state + .consume(&event) + .map_err(|e| format!("consume({action:?}): {e}"))?; + + Ok(()) +} + +// ── Single game ──────────────────────────────────────────────────────────────── + +/// Run one full game, optionally printing play-by-play. +/// Returns `(steps, truncated)`. +fn run_game(roller: &mut DiceRoller, max_steps: usize, quiet: bool, verbose: bool) -> (usize, bool) { + let mut state = GameState::new_with_players("White", "Black"); + let mut step = 0usize; + + if !quiet { + println!("{state}"); + } + + while state.stage != Stage::Ended { + step += 1; + if step > max_steps { + return (step - 1, true); + } + + match state.turn_stage { + TurnStage::RollDice => { + let player = state.active_player_id; + match apply_dice_roll(&mut state, roller) { + Ok(dice) => { + if !quiet { + println!( + "[step {step:4}] {} rolls: {} & {}", + player_label(player), + dice.values.0, + dice.values.1 + ); + } + } + Err(e) => { + eprintln!("Error during dice roll: {e}"); + eprintln!("State:\n{state}"); + return (step, true); + } + } + } + stage => { + let player = state.active_player_id; + match apply_player_action(&mut state) { + Ok(()) => { + if !quiet { + println!( + "[step {step:4}] {} ({stage:?})", + player_label(player) + ); + if verbose { + println!("{state}"); + } + } + } + Err(e) => { + eprintln!("Error: {e}"); + eprintln!("State:\n{state}"); + return (step, true); + } + } + } + } + } + + if !quiet { + println!("\n=== Game over after {step} steps ===\n"); + println!("{state}"); + + let white = state.players.get(&1); + let black = state.players.get(&2); + + match (white, black) { + (Some(w), Some(b)) => { + println!("White — holes: {:2}, points: {:2}", w.holes, w.points); + println!("Black — holes: {:2}, points: {:2}", b.holes, b.points); + println!(); + + let white_score = w.holes as i32 * 12 + w.points as i32; + let black_score = b.holes as i32 * 12 + b.points as i32; + + if white_score > black_score { + println!("Winner: White (+{})", white_score - black_score); + } else if black_score > white_score { + println!("Winner: Black (+{})", black_score - white_score); + } else { + println!("Draw"); + } + } + _ => eprintln!("Could not read final player scores."), + } + } + + (step, false) +} + +// ── Main ────────────────────────────────────────────────────────────────────── + +fn main() { + let args = parse_args(); + let mut roller = DiceRoller::new(args.seed); + + if args.games == 1 { + println!("=== Trictrac — random game ==="); + if let Some(s) = args.seed { + println!("seed: {s}"); + } + println!(); + run_game(&mut roller, args.max_steps, false, args.verbose); + } else { + println!("=== Trictrac — {} games ===", args.games); + if let Some(s) = args.seed { + println!("seed: {s}"); + } + println!(); + + let mut total_steps = 0u64; + let mut truncated = 0usize; + + let t0 = Instant::now(); + for _ in 0..args.games { + let (steps, trunc) = run_game(&mut roller, args.max_steps, !args.verbose, args.verbose); + total_steps += steps as u64; + if trunc { + truncated += 1; + } + } + let elapsed = t0.elapsed(); + + let secs = elapsed.as_secs_f64(); + println!("Games : {}", args.games); + println!("Truncated : {truncated}"); + println!("Total steps: {total_steps}"); + println!("Avg steps : {:.1}", total_steps as f64 / args.games as f64); + println!("Elapsed : {:.3} s", secs); + println!("Throughput : {:.1} games/s", args.games as f64 / secs); + println!(" {:.0} steps/s", total_steps as f64 / secs); + } +} From bd4c75228ba9500b78861f7cddd89b744ec7970d Mon Sep 17 00:00:00 2001 From: Henri Bourcereau Date: Fri, 6 Mar 2026 18:09:15 +0100 Subject: [PATCH 2/3] fix: exit with farthest rule (2) --- store/src/game_rules_moves.rs | 52 +++++++++++++++++++++++------------ 1 file changed, 34 insertions(+), 18 deletions(-) diff --git a/store/src/game_rules_moves.rs b/store/src/game_rules_moves.rs index 41221f2..955ab3c 100644 --- a/store/src/game_rules_moves.rs +++ b/store/src/game_rules_moves.rs @@ -361,17 +361,24 @@ impl MoveRules { let _ = board_to_check.move_checker(&Color::White, moves.0); let farthest_on_move2 = Self::get_board_exit_farthest(&board_to_check); - let (is_move1_exedant, is_move2_exedant) = self.move_excedants(moves); - if (is_move1_exedant && moves.0.get_from() != farthest_on_move1) - || (is_move2_exedant && moves.1.get_from() != farthest_on_move2) - { + // dice normal order + let (is_move1_exedant, is_move2_exedant) = self.move_excedants(moves, true); + let is_not_farthest1 = (is_move1_exedant && moves.0.get_from() != farthest_on_move1) + || (is_move2_exedant && moves.1.get_from() != farthest_on_move2); + + // dice reversed order + let (is_move1_exedant, is_move2_exedant) = self.move_excedants(moves, false); + let is_not_farthest2 = (is_move1_exedant && moves.0.get_from() != farthest_on_move1) + || (is_move2_exedant && moves.1.get_from() != farthest_on_move2); + + if is_not_farthest1 && is_not_farthest2 { return Err(MoveError::ExitNotFarthest); } Ok(()) } - fn move_excedants(&self, moves: &(CheckerMove, CheckerMove)) -> (bool, bool) { + fn move_excedants(&self, moves: &(CheckerMove, CheckerMove), dice_order: bool) -> (bool, bool) { let move1to = if moves.0.get_to() == 0 { 25 } else { @@ -386,20 +393,16 @@ impl MoveRules { }; let dist2 = move2to - moves.1.get_from(); - let dist_min = cmp::min(dist1, dist2); - let dist_max = cmp::max(dist1, dist2); - - let dice_min = cmp::min(self.dice.values.0, self.dice.values.1) as usize; - let dice_max = cmp::max(self.dice.values.0, self.dice.values.1) as usize; - - let min_excedant = dist_min != 0 && dist_min < dice_min; - let max_excedant = dist_max != 0 && dist_max < dice_max; - - if dist_min == dist1 { - (min_excedant, max_excedant) + let (dice1, dice2) = if dice_order { + self.dice.values } else { - (max_excedant, min_excedant) - } + (self.dice.values.1, self.dice.values.0) + }; + + ( + dist1 != 0 && dist1 < dice1 as usize, + dist2 != 0 && dist2 < dice2 as usize, + ) } fn get_board_exit_farthest(board: &Board) -> Field { @@ -1587,6 +1590,19 @@ mod tests { CheckerMove::new(22, 0).unwrap(), ); assert!(state.check_exit_rules(&moves).is_ok()); + + state.dice.values = (6, 4); + state.board.set_positions( + &crate::Color::White, + [ + -4, -1, -2, -1, 0, 0, 0, -1, 0, 0, 0, 0, -5, -1, 0, 0, 0, 0, 2, 3, 2, 2, 5, 1, + ], + ); + let moves = ( + CheckerMove::new(20, 24).unwrap(), + CheckerMove::new(23, 0).unwrap(), + ); + assert!(state.check_exit_rules(&moves).is_ok()); } #[test] From 44a5ba87b0988b9cb114873afd70f1ba6bd170db Mon Sep 17 00:00:00 2001 From: Henri Bourcereau Date: Fri, 6 Mar 2026 18:11:03 +0100 Subject: [PATCH 3/3] perf research --- doc/store_research.md | 462 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 462 insertions(+) create mode 100644 doc/store_research.md diff --git a/doc/store_research.md b/doc/store_research.md new file mode 100644 index 0000000..fa1fa5b --- /dev/null +++ b/doc/store_research.md @@ -0,0 +1,462 @@ +# Store Crate: Deep Research & Performance Analysis + +This document covers the Rust `trictrac-store` crate that backs the OpenSpiel C++ game. +It traces the full data-flow from a C++ `get_legal_actions()` call down to individual +board operations, documents design decisions, and identifies performance bottlenecks +relevant to MCTS training throughput. + +--- + +## 1. Module Map + +| File | Responsibility | +|---|---| +| `board.rs` | `Board` (`[i8;24]`), `CheckerMove`, low-level move primitives | +| `dice.rs` | `Dice` (two `u8` values), `DiceRoller` | +| `player.rs` | `Color`, `Player` (score / holes / bredouille flags) | +| `game.rs` | `GameState` (the full game state machine + serialisation) | +| `game_rules_moves.rs` | `MoveRules` — legal move generation and validation | +| `game_rules_points.rs` | `PointsRules` — jan (scoring) computation | +| `training_common.rs` | Action encoding/decoding (`TrictracAction`, 514-element space) | +| `cxxengine.rs` | FFI bridge (cxx.rs) — the sole entry point from C++ | + +--- + +## 2. Data Model + +### 2.1 Board + +``` +positions: [i8; 24] // index i → field i+1 (fields are 1-indexed) +positive → white checkers +negative → black checkers +0 → empty +``` + +Fields 1–6 = player 1's home quarter ("petit jan"), 7–12 = big jan, 13–18 = opponent +big jan, 19–24 = opponent home quarter. +Field 12 = White rest corner, field 13 = Black rest corner. + +Mirror (for Black's perspective): negate every element and reverse the array. + +### 2.2 CheckerMove + +``` +CheckerMove { from: Field, to: Field } +from ∈ [1,24], to ∈ [0,24] (to==0 means bear off) +(0,0) = EMPTY_MOVE (no-op, used when a player cannot move one die) +``` + +### 2.3 Action Space (training_common.rs) + +514 discrete actions: + +| Index | Meaning | +|---|---| +| 0 | `Roll` — trigger a dice roll | +| 1 | `Go` — take the hole and reset (instead of holding) | +| 2–257 | `Move` with `dice_order=true` (die1 first): `2 + checker1*16 + checker2` | +| 258–513 | `Move` with `dice_order=false` (die2 first): `258 + checker1*16 + checker2` | + +`checker1` and `checker2` are 1-indexed ordinal positions of checkers from the +starting field (not the field index). This avoids a per-position bijection and keeps +the space fixed-size regardless of where checkers are. + +### 2.4 GameState + +Key fields that affect performance: + +```rust +pub struct GameState { + pub board: Board, // 24 bytes + pub active_player_id: u64, // 1 or 2 + pub players: HashMap, // 2 entries + pub history: Vec, // grows unboundedly + pub dice: Dice, // 2 bytes + pub dice_points: (u8, u8), + pub dice_moves: (CheckerMove, CheckerMove), + pub dice_jans: PossibleJans, // HashMap> + pub turn_stage: TurnStage, + pub stage: Stage, + ... +} +``` + +`history` is the largest field and grows ~3–4 entries per turn. +A 200-turn game holds ~600 `GameEvent` values. + +--- + +## 3. Call Chain: get_legal_actions (C++ → Rust) + +``` +C++ LegalActions() + └─ engine_->get_legal_actions(player_idx) [cxxengine.rs] + └─ get_valid_action_indices(&game_state or &mirrored) + └─ get_valid_actions(state) [training_common.rs] + └─ MoveRules::get_possible_moves_sequences(true, []) + ├─ get_possible_moves_sequences_by_dices(dice_max, dice_min, ...) + │ ├─ board.get_possible_moves(dice1) [loop over fields] + │ └─ for each first_move: + │ ├─ board.clone() [24-byte copy] + │ ├─ board2.get_possible_moves(dice2) + │ └─ for each second_move: + │ ├─ check_corner_rules() + │ ├─ check_opponent_can_fill_quarter_rule() + │ ├─ check_exit_rules() [may recurse!] + │ └─ check_must_fill_quarter_rule() [recurses!] + └─ get_possible_moves_sequences_by_dices(dice_min, dice_max, ...) + [same structure as above] +``` + +Then for each valid `(CheckerMove, CheckerMove)` pair, `checker_moves_to_trictrac_action()` +maps it back to a `TrictracAction`: + +``` +checker_moves_to_trictrac_action(move1, move2, color, state) + └─ white_checker_moves_to_trictrac_action(...) + ├─ board.get_field_checker(White, from1) [O(24) scan] + ├─ board.clone() + ├─ board.move_checker(White, move1) [board mutation] + └─ board.get_field_checker(White, from2) [O(24) scan] +``` + +For **player 2**, an extra `GameState::mirror()` is called before all of this, +cloning the full state including history. + +--- + +## 4. Move Generation Deep Dive + +### 4.1 get_possible_moves_sequences + +```rust +pub fn get_possible_moves_sequences( + &self, + with_excedents: bool, + ignored_rules: Vec, +) -> Vec<(CheckerMove, CheckerMove)> { + // called TWICE, once per dice order (max-first, then min-first) + let mut seqs = self.get_possible_moves_sequences_by_dices(dice_max, dice_min, ...); + let mut seqs2 = self.get_possible_moves_sequences_by_dices(dice_min, dice_max, ...); + seqs.append(&mut seqs2); + // deduplication via HashSet +} +``` + +The function is **correct but called recursively** through rule validation: + +- `check_must_fill_quarter_rule()` calls `get_quarter_filling_moves_sequences()` +- `get_quarter_filling_moves_sequences()` calls `get_possible_moves_sequences(true, [Exit, MustFillQuarter])` +- This inner call's `check_must_fill_quarter_rule()` does **not** recurse further (because `MustFillQuarter` is in ignored_rules) + +So there are at most **2 levels of recursion**, but the second level is invoked once per +candidate move pair at the outer level. If there are N first-moves × M second-moves, +`get_quarter_filling_moves_sequences()` is called N×M times and each call triggers a +full second-level move generation pass. + +### 4.2 get_possible_moves_sequences_by_dices + +```rust +for first_move in board.get_possible_moves(dice1, ...) { + let mut board2 = self.board.clone(); // ← clone per first move + board2.move_checker(color, first_move); + for second_move in board2.get_possible_moves(dice2, ...) { + // 4 rule checks, each potentially expensive + if all_pass { + moves_seqs.push((first_move, second_move)); + } + } + if !has_second { + // also push (first_move, EMPTY_MOVE) after same 4 checks + } +} +``` + +**Board clones**: one per first-move candidate. With 15 checkers on 24 fields, a +typical position has 5–15 valid first moves → 5–15 board clones per call. + +### 4.3 check_must_fill_quarter_rule + +```rust +fn check_must_fill_quarter_rule(&self, moves) -> Result<(), MoveError> { + let filling_moves_sequences = self.get_quarter_filling_moves_sequences(); + if !filling_moves_sequences.contains(moves) && !filling_moves_sequences.is_empty() { + return Err(MoveError::MustFillQuarter); + } + Ok(()) +} +``` + +`get_quarter_filling_moves_sequences()` runs a full pass of move generation and +applies both checker moves to a board clone for each candidate: + +```rust +pub fn get_quarter_filling_moves_sequences(&self) -> Vec<(CheckerMove, CheckerMove)> { + for moves in self.get_possible_moves_sequences(true, [Exit, MustFillQuarter]) { + let mut board = self.board.clone(); // ← clone per candidate + board.move_checker(color, moves.0).unwrap(); + board.move_checker(color, moves.1).unwrap(); + if board.any_quarter_filled(Color::White) { + moves_seqs.push(moves); + } + } + moves_seqs +} +``` + +If quarter-filling is not relevant to the current position (the common case early in +the game), this entire function still runs a full move generation pass before returning +an empty vec. + +### 4.4 check_exit_rules + +```rust +fn check_exit_rules(&self, moves) -> Result<(), MoveError> { + if !moves.0.is_exit() && !moves.1.is_exit() { return Ok(()); } + if self.has_checkers_outside_last_quarter() { return Err(...); } + let non_excedent_seqs = self.get_possible_moves_sequences(false, [Exit]); // ← full pass + if non_excedent_seqs.contains(moves) { return Ok(()); } + if !non_excedent_seqs.is_empty() { return Err(ExitByEffectPossible); } + // check farthest checker rule ... + Ok(()) +} +``` + +Called per candidate move pair during the inner loop. Triggers another full +`get_possible_moves_sequences(false, [Exit])` pass whenever a move involves bearing off. + +--- + +## 5. Jan (Points) Computation + +### 5.1 get_jans (game_rules_points.rs) + +Called once per dice roll via `game.rs: consume(RollResult)`. Not on the MCTS hot path +(MCTS does not need to compute points, only moves). However it is called during +`get_possible_moves_sequences()` indirectly via `get_scoring_quarter_filling_moves_sequences()`. + +`PossibleJans = HashMap>` — with only 13 possible +enum keys this is overkill. A fixed-size array would be faster. + +`PossibleJansMethods::push()` uses `ways.contains(&cmoves)` — O(n) linear search on +the existing moves list to avoid duplicates. For small lists this is fine, but it can +be called dozens of times per position. + +`PossibleJansMethods::merge()` for `TrueHitBigJan`/`TrueHitSmallJan` does two O(n) +scans per entry (one `contains`, one `retain`). + +--- + +## 6. State Encoding + +### 6.1 to_vec() — neural-network input (36 i8 values) + +``` +[0..23] board positions (i8, negative = black) +[24] active player color (0=white, 1=black) +[25] turn stage (u8 cast to i8) +[26] dice.values.0 +[27] dice.values.1 +[28..31] white player: points, holes, can_bredouille, can_big_bredouille +[32..35] black player: same +``` + +Simple, allocation-heavy (returns `Vec`). For the MCTS hot path, returning a +`[i8; 36]` stack array would avoid the heap allocation entirely. + +### 6.2 GameState::mirror() + +```rust +pub fn mirror(&self) -> GameState { + // Mirrors board (O(24)) + // Swaps and mirrors two player entries in a new HashMap + // Clones and mirrors the entire history Vec (O(history.len())) ← expensive + // Mirrors dice_moves (O(1)) + // Mirrors dice_jans (clone + HashMap iteration) + ... +} +``` + +`mirror()` is called on every `get_legal_actions()` invocation for player 2 (Black). +Cloning the history is **O(history.len())** and history grows through the entire game. + +--- + +## 7. Action Encoding/Decoding + +### 7.1 TrictracAction::to_event() — decode action index → GameEvent + +For `Move` actions, `to_event()`: +1. Reads `checker1` / `checker2` ordinal positions +2. Calls `board.get_checker_field(color, checker1)` — O(24) scan +3. Clones the board +4. Applies the first move on the clone +5. Calls `board.get_checker_field(color, checker2)` — O(24) scan on the clone +6. Adjusts for "prise par puissance" +7. Constructs the `GameEvent::Move` + +This is called once per `apply_action()` call from C++, so it is not as hot as legal +action generation. + +### 7.2 white_checker_moves_to_trictrac_action() — encode (CheckerMove, CheckerMove) → TrictracAction + +Called for **every valid move** during `get_valid_actions()`: +1. Computes `diff_move1` to identify which die was used first +2. Calls `board.get_field_checker(White, from1)` — O(24) scan +3. Clones the board +4. Calls `board.move_checker(White, move1)` — mutation on clone +5. Calls `board.get_field_checker(White, from2)` — O(24) scan on clone + +With 20 valid moves, this is 20 board clones + 40 O(24) scans per +`get_valid_actions()` call. + +--- + +## 8. Identified Performance Bottlenecks + +Ordered by estimated impact on MCTS training throughput: + +### 8.1 [HIGH] Recursive move generation in check_must_fill_quarter_rule + +**Problem**: `get_possible_moves_sequences()` is called O(F₁ × F₂) times where F₁ and F₂ are +the number of first- and second-move candidates. Each inner call runs a complete move +generation pass. + +**When it triggers**: Only when at least one valid move would fill/preserve a quarter +of the board. This is a common situation especially in mid/late game. + +**Fix direction**: Precompute `get_quarter_filling_moves_sequences()` once per +`get_possible_moves_sequences()` call and pass it in, instead of recomputing it for +each candidate pair. + +### 8.2 [HIGH] GameState::mirror() copies history + +**Problem**: `mirror()` clones + iterates `self.history` on every `get_legal_actions()` +call for Black. The history grows proportionally to game length and serves no purpose +for action generation. + +**Fix direction**: Either skip history in `mirror()` (pass an empty `Vec` for that +field) or refactor `cxxengine.rs` to mirror only the board and thread color perspective +through the functions that need it. + +### 8.3 [MEDIUM] Board clones in get_possible_moves_sequences_by_dices + +**Problem**: One `board.clone()` per first-move candidate (5–15 per call). Each clone +is 24 bytes, but the allocator round-trip costs more than the copy. + +**Fix direction**: Apply and undo moves on a single mutable board (move + undo pattern) +rather than cloning. Board mutation is O(1) and undoing is symmetric. + +### 8.4 [MEDIUM] Board clones in get_quarter_filling_moves_sequences + +**Problem**: One `board.clone()` per candidate move sequence, plus two `move_checker()` +calls (which can return Err — currently `.unwrap()`ed). These clones are nested inside +the already-expensive recursive path described in 8.1. + +**Fix direction**: Same move + undo pattern as 8.3. + +### 8.5 [MEDIUM] Board clones and O(24) scans in checker_moves_to_trictrac_action + +**Problem**: Called for every valid move in `get_valid_actions()`. With 20 valid moves, +this is 20 clones + 40 O(24) scans. + +**Fix direction**: Pass the checker ordinal index directly from `get_board_exit_farthest` +or store it alongside the CheckerMove when generating moves. This avoids re-scanning +to convert back. + +### 8.6 [MEDIUM] check_exit_rules triggers a full move generation pass + +**Problem**: Called for every candidate move pair that involves bearing off. Runs +`get_possible_moves_sequences(false, [Exit])` → another full move generation. + +**Fix direction**: Precompute non-excedant moves once if any move in the candidate set +involves bearing off, and pass the result in. + +### 8.7 [LOW] PossibleJans backed by HashMap with linear-search deduplication + +**Problem**: Only 13 `Jan` variants exist. A `HashMap` adds hashing overhead and +pointer indirection. `Vec::contains()` for deduplication is O(n). + +**Fix direction**: Use `[Option>; 13]` with `Jan as usize` index. For +deduplication use a `BTreeSet` or just sort + dedup (the lists are short). + +### 8.8 [LOW] to_vec() returns a heap-allocated Vec + +**Problem**: Called from C++ via `get_tensor()` on every MCTS rollout observation. + +**Fix direction**: Return `[i8; 36]` or write directly into a pre-allocated C++-owned +buffer (possible with cxx.rs via `Pin<&mut CxxVector>`). + +### 8.9 [LOW] get_color_fields() allocates a Vec on every call + +**Problem**: Called repeatedly in move generation (once per first-move enumeration, +once in is_quarter_fillable per field, etc.). + +**Fix direction**: Return a fixed-size `ArrayVec<(Field, i8), 24>` (using the +`arrayvec` crate) or add a small-vec optimisation. + +### 8.10 [LOW] unwrap() calls in hot paths (correctness concern) + +Currently `.unwrap()` in: +- `get_quarter_filling_moves_sequences()` line 529-530: `board.move_checker(...).unwrap()` +- Multiple places in `game_rules_points.rs` + +With `catch_unwind` wrapping at the FFI boundary these panics are now caught, but +they still abort the move and propagate an error to C++ rather than producing a clean +`SpielFatalError` message. These should be `?` with proper `Result` propagation or +`.expect()` with descriptive messages. + +--- + +## 9. Key Correctness Observations + +### 9.1 Game ends during chance action (fixed in alpha_zero.cc) + +`game.rs::consume(RollResult)` sets `stage = Stage::Ended` (line 795) when a player's +jan score grants their 13th hole on the dice roll itself. This makes a **chance action** +terminal. The OpenSpiel `PlayGame` loop in `alpha_zero.cc` did not check `IsTerminal()` +after chance actions — this was the root cause of the SIGSEGV crash. Fixed by adding: +```cpp +state->ApplyAction(action); // chance action +if (state->IsTerminal()) { trajectory.returns = state->Returns(); break; } +``` + +### 9.2 current_player_idx() panics on underflow + +```rust +fn current_player_idx(&self) -> u64 { + self.game_state.active_player_id - 1 // u64 underflow if id == 0 +} +``` + +`active_player_id` is always 1 or 2 during normal play (set by `BeginGame`), so this +is safe in practice. However it should be wrapped in `catch_unwind` or guarded by an +explicit check for robustness: +```rust +self.game_state.active_player_id.saturating_sub(1) +``` + +### 9.3 Mirror is always from White's perspective + +`MoveRules` and `PointsRules` always reason from White's point of view. For Black, the +caller must mirror the board beforehand. The comment at the top of `game_rules_moves.rs` +documents this. `cxxengine.rs` correctly mirrors the `GameState` for player 2 before +calling `get_valid_action_indices()`. + +--- + +## 10. Summary Table + +| Bottleneck | Location | Estimated Severity | Fix Complexity | +|---|---|---|---| +| Recursive `get_possible_moves_sequences` in `check_must_fill_quarter_rule` | `game_rules_moves.rs:272` | High | Medium | +| `GameState::mirror()` clones history | `game.rs:159` | High | Low | +| Board clones per first-move in `get_possible_moves_sequences_by_dices` | `game_rules_moves.rs:555` | Medium | Medium | +| Board clones in `get_quarter_filling_moves_sequences` | `game_rules_moves.rs:528` | Medium | Medium | +| `check_exit_rules` triggers full move generation | `game_rules_moves.rs:335` | Medium | Medium | +| `checker_moves_to_trictrac_action` clones per valid move | `training_common.rs:326` | Medium | Low–Medium | +| `PossibleJans` HashMap + linear dedup | `game_rules_points.rs:67` | Low | Low | +| `to_vec()` heap allocation | `game.rs:213` | Low | Low | +| `get_color_fields()` Vec allocation | `board.rs:405` | Low | Medium | +| `unwrap()` in hot paths | multiple | Correctness | Low |