Optimize move generation with early cross-set filtering

[olaugh]

Summary

This change optimizes the move generation algorithm by reordering condition checks to perform cheaper cross-set lookups before more expensive rack count lookups, reducing unnecessary memory accesses during the recursive tile exploration phase.

Key Changes

• Reordered condition evaluation: Check in_cross_set() first before accessing gen->rack.counts[tile], since the cross-set bit test is cheaper than array lookup
• Added rack_bits bitmap: Introduced gen.rack_bits field to cache a bitmap representation of tiles in the rack for faster tile-in-rack checks
• Initialize and restore rack_bits: Added initialization of rack_bits after copying the rack and restoration after shadow generation to keep it in sync with rack state

Implementation Details

The optimization leverages the observation that most tiles in the KWG dictionary won't be in the cross-set for a given position, so we can skip the rack lookup entirely for those tiles. By checking the cross-set first (a simple bit test operation), we avoid the more expensive gen->rack.counts[tile] array access for tiles that can't be played anyway.

The rack_bits bitmap is built using build_rack_cross_set() and maintained throughout the generation process to ensure consistent state, particularly after shadow generation which may modify the rack during exploration.

This is a performance optimization with no functional changes to the move generation logic.

[Copilot]

Pull request overview

This pull request optimizes the move generation algorithm in the recursive GADDAG traversal by reordering condition checks. The main change swaps the order of cross-set and rack lookup checks to potentially reduce unnecessary memory accesses when generating moves.

Changes:

• Reordered condition evaluation in recursive_gen() to check in_cross_set() before gen->rack.counts[tile]
• Added initialization and restoration of gen.rack_bits in generate_moves() (though this field already existed and was already being used elsewhere in the codebase)

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The comment states this builds a rack_bits bitmap for "fast tile-in-rack checks", but the optimized code in recursive_gen (lines 1674-1677) doesn't actually use gen.rack_bits. It only reorders the conditions to check in_cross_set before gen->rack.counts[tile]. The rack_bits field was already present in the codebase and is used elsewhere (e.g., in shadow algorithm), so this initialization is redundant with the original behavior. Consider either: (1) updating the comment to clarify this just maintains existing behavior, or (2) actually using rack_bits in recursive_gen by adding an early check like "if ((cross_set & gen->rack_bits) == 0 && !has_blank) return;" before the loop to skip entirely when no rack tiles match the cross-set.

[Copilot]

The comment claims shadow may modify rack_bits during exploration, but rack_bits is a derived value computed from rack.counts. The shadow algorithm modifies gen->rack.counts (e.g., lines 605, 613), not rack_bits directly. Since the rack is being restored via memcpy from the original rack on line 2061, and rack_bits is simply rebuilt from that rack, this comment is misleading. The restoration of rack_bits here is necessary because the rack was restored, not because "shadow may modify it during exploration" - rack_bits is never directly modified, only recomputed.

Suggested change

	/* Restore rack after shadow (gen_shadow modifies it) */
	memcpy(&gen.rack, rack, sizeof(Rack));
	/* Restore rack_bits after shadow (shadow may modify it during exploration) */
	/* Restore rack after shadow (gen_shadow modifies gen.rack.counts) */
	memcpy(&gen.rack, rack, sizeof(Rack));
	/* Rebuild rack_bits because it is derived from gen.rack, which was restored */

[Copilot]

The comment describes in_cross_set as a "cheap bit test", but it actually involves an array lookup (BIT_MASK[UNBLANKED(ml)]), bitwise operations, and a comparison - similar in cost to the rack.counts array lookup it's being compared against. The optimization is still valid because it avoids the rack lookup for tiles not in the cross-set, but the comment overstates the cost difference. Consider revising to: "Check cross-set first to avoid rack lookup for tiles not in cross_set" to be more accurate about the optimization rationale.

Suggested change

	/* Check cross-set first (cheap bit test), then rack if in cross-set.
	/* Check cross-set first, then rack if in cross-set.

[github-actions]

Speed Test Benchmark Results

Performance Comparison

Metric	Test (refs/pull/34/merge)	Baseline (main)	Change
NWL23 Hybrid	311.4 frames	285.8 frames	+8.9500% slower
CSW24 Hybrid	344.6 frames	315.3 frames	+9.2900% slower

Detailed Results

Test Results (refs/pull/34/merge)

{
  "num_games": 50,
  "lexicons": [
    {
      "name": "NWL23",
      "shadow_mean": 332.3,
      "noshadow_mean": 362.0,
      "hybrid_mean": 311.4,
      "move_count": 1148
    },
    {
      "name": "CSW24",
      "shadow_mean": 362.6,
      "noshadow_mean": 421.9,
      "hybrid_mean": 344.6,
      "move_count": 1091
    }
  ]
}

Baseline Results (main)

{
  "num_games": 50,
  "lexicons": [
    {
      "name": "NWL23",
      "shadow_mean": 308.2,
      "noshadow_mean": 332.6,
      "hybrid_mean": 285.8,
      "move_count": 1148
    },
    {
      "name": "CSW24",
      "shadow_mean": 335.4,
      "noshadow_mean": 386.4,
      "hybrid_mean": 315.3,
      "move_count": 1091
    }
  ]
}

---

View full workflow run

[olaugh]

this is slower than before, giving up