V3#123
Draft
CorentinGS wants to merge 135 commits into
Draft
Conversation
Final alloc profile: big.pgn 543,589 allocs/run (baseline 690k, -21%); big_big.pgn 1,921,237 allocs/run (baseline 2.86M, -33%); bytes 815M->665M (-18%). Ceilings tightened (Big 580k->575k, BigBig 2.1M->2.05M). sync.Pool for *Position rejected after spike: ~3% win comparable to pool overhead. Pre-allocate undos cap=128 rejected after spike: 33KB/game upfront, newMoveTree became #1 flat source. writeMoveEncoding uses Cursor.Peek (no-copy) inside renderTo save/restore boundary. ADR-018 (lazy-position-api) documents the design and rejected alternatives. ADR-0001 amended to point at ADR-018 (applyMove now owns the incremental hash so cursor-derived positions carry a correct hash for repetition detection). ADR-011 extended to cover PositionCursor.Peek (zero-copy aliasing pointer, opt-in). Post-review cleanup: PositionCursor.Forward(idx) routes through setCurrent so the makeMove/undo append invariant lives in one place; writeMoveEncoding drops dead params (_ *MoveNode, _ = subVariation).
Two changes close the remaining gap vs the pre-cursor baseline (vnxmzqwy):
1. cursorUndo (~56B) replaces perft's full-state positionUndo (~264B) in the tree undo stack. The undo stores the move's moveEffect plus scalar state; board.unapply reverses the move with bitboard ops instead of a 192B board memcpy. Perft keeps the full-state undo (stack-allocated, ADR-0001 hot path). applyMove returns its moveEffect so makeMoveCursor does not compute it twice. Correctness pinned by TestCursorUndoRoundTrip: FEN + hash equality after unmake at every node across the perft FEN suite (castling, en passant, promotions).
2. setCurrent slow path retreats to the lowest common ancestor (lcaNode) instead of resetCursor + replay-from-root. Variation entry/exit in PGN parsing becomes O(distance) with no rootPos.copy allocation. game_split seeds current=root in the tree literal so the LCA walk has a valid start. isAncestor and replayTo are subsumed and deleted.
benchstat vs vnxmzqwy (n=10, BenchmarkPGN_FullGameDecode_{Big,BigBig}):
sec/op Big -1.75% (p=0.002) BigBig -15.52% (p=0.000)
B/op Big -32.66% (p=0.000) BigBig -27.37% (p=0.000)
allocs/op Big -19.50% (p=0.000) BigBig -28.12% (p=0.000)
Intermediate steps: slim undo alone recovered B/op -46%/-64% and sec/op -2.4%/-17.5% vs the naive full-state cursor; LCA then cut Big decode time a further 26%. ADR-018 updated with the final numbers and both decisions.
--- Part 1: Code refactor --- Replace the explicit saved-var + setCurrent pattern with defer at 6 sites that save and restore the tree cursor around work: - game.go: MoveList, Positions, numOfRepetitions - movetree.go: AddVariation - pgn_renderer.go: renderTo - move.go: MoveNode.Position() Defer captures t.current at registration time and restores on every return path, including panics. In AddVariation this also fixes a latent bug: the Goto failure path was not restoring the cursor, leaving the caller's active position corrupted. Benchstat vs pre-defer (slim+LCA, n=10): B/op and allocs/op identical; sec/op noise-level (Big -1.47% p=0.011, BigBig ~unchanged p=0.353). --- Part 2: Documentation sync after two-axis code review --- Address all findings from the review of v3..HEAD: MIGRATION.md:66 - fix stale inline comment on node.Position() PRD.md - add "Follow-up commits" section acknowledging nwyyxmsk issue 02 - annotate undos/Clone spec with ADR-018 deviations issue 05 - annotate MoveList slice decision + []Comment never existed issue 06 - annotate writeMoveEncoding/buildOneGameFromPath deviations move.go MoveNode.Position() - doc comment explaining cursor side effect move_applier.go applyMove - ADR-016 reference for COW+in-place paths
polybook and outcome
(refinements after two-axis code review: add TestMoveTreeGotoSiblingSubtree; rewrite the vacuous panic test to pin observational purity on the Position() call; use Peek().copy() in game.Positions to remove direct pos field access; drop the duplicate doc line on MoveNode.Position)
refactor(pgn): hoist bool ponytail comment into writeMoves doc; build pgnRender once
Adds resolveCanonicalMove in legality.go so the safe move seam (Game.Move, MoveTree.AddVariation, PushMoveText) stores the canonical generated Move with position-derived tags. Existing Move occurrences with stale tags and no continuations are repaired; those with continuations are rejected to protect subtree consistency. Includes canonical_move_seam_test.go and a small comment correction in pgn_alloc_budget_test.go.
Adds NewMove(s1, s2 Square, promo ...PieceType) for building a Move from raw coordinates. Tags are left empty; godoc explains that Capture and Check tags are position-derived and must be added by the canonical move seam. Closes .scratch/chess-api-improvements/01-NewMove-constructor.md.
Position already tracked inCheck internally; extend it to store the checking squares so Checkers() is O(1) after a position update. Adds checkState to compute both the in-check flag and the checker squares from the existing attack-set walk. Cursor and perft undo records preserve the checker list so navigation remains consistent. Closes .scratch/chess-api-improvements/02-Position-IsCheck-Checkers.md.
Exposes a public single-probe legality check on Position. The initial implementation linear-scans ValidMovesUnsafe, matching the existing validatePositionMove behavior. Null moves and moves by the side not to move return false. Closes .scratch/chess-api-improvements/03-Position-IsLegal.md.
Adds a convenience method that derives the decisive or draw outcome implied by the terminal position, or NoOutcome if play continues. It wraps the existing Status() and auto-draw classification, restricted to board-state terminal conditions: checkmate, stalemate, insufficient material, and the seventy-five move rule. Game-level outcomes (resignation, draw offer, and the claimable fifty-move rule) remain on Game. Closes .scratch/chess-api-improvements/04-Position-Outcome.md.
Generalizes the existing Board.hasSufficientMaterial into a public per-color query. Color c has insufficient material when it has no queen, rook, or pawn and its remaining pieces cannot force checkmate: lone king, single minor piece, or bishops confined to one square color. The existing whole-position auto-draw path now reuses the per-color form. Closes .scratch/chess-api-improvements/05-Position-HasInsufficientMaterial.md.
Introduces a public Bitboard type backed by uint64 alongside the existing unexported bitboard. It provides NewBitboardFromSquares, a range-over-func iterator (Squares), set operations, and text marshalling. The internal bitboard type is left untouched; this is the expand step of the migration. Closes .scratch/chess-api-improvements/06-Public-Bitboard-type.md.
Adds public O(1) queries on Board that return the new Bitboard type: White, Black, Occupied, Empty, Color, and Pieces. These wrap the precomputed convenience bitboards that Board already maintains internally. Closes .scratch/chess-api-improvements/07-Board-set-queries.md.
Adds KnightAttacks, KingAttacks, PawnAttacks, BishopAttacks, RookAttacks, and QueenAttacks as pure functions over the existing precomputed and magic attack tables. Sliding-piece attacks take a caller-supplied occupancy Bitboard. Closes .scratch/chess-api-improvements/08-Public-attacks-module.md.
Keeps Position.EnPassantSquare() as the raw post-double-push target and adds Position.LegalEnPassantSquare(), which returns the square only when an enemy pawn can actually capture en passant. Godoc on both methods clarifies raw vs legal and notes that the legal form feeds the Zobrist hash. Closes .scratch/chess-api-improvements/09-En-passant-mode-distinction.md.
Adds Game.MoveText and Game.UnsafeMoveText as the preferred names and marks PushMove, PushMoveText, and UnsafePushMoveText as deprecated. Migrates in-tree callers (tests, README examples, PGN path) to the new names. The deprecated aliases remain functional for this major version. Closes .scratch/chess-api-improvements/10-Collapse-game-move-methods.md.
Replaces the string-based Outcome with an int-backed enum (NoOutcome, WhiteWon, BlackWon, Draw). String() preserves the PGN result tokens, and ParseOutcome converts result tokens back to typed values. All in-tree callers are migrated; UnknownOutcome collapses into NoOutcome. Closes .scratch/chess-api-improvements/11-Strongly-type-Outcome.md.
Adds a public Setup struct that describes a position without validating it, and NewPosition(Setup) that validates king counts, back-rank pawns, castling-rights consistency, en-passant consistency, and move counters before constructing a playable Position. FEN parsing now builds a Setup first (decodeFENSetup) and routes the result through NewPosition. An internal decodeFENUnsafe path preserves the ability for tests and hash-only callers to construct positions from arrow or inconsistent FENs. Closes .scratch/chess-api-improvements/13-Setup-validator-split.md.
Adds ParseSquare, ParseColor, ParsePieceType, and ParsePieceTypeFromByte with Go-idiomatic (T, error) signatures. The old zero-on-error helpers are kept as deprecated wrappers so this is the expand step; callers inside the package and tests are migrated to the new forms. Closes .scratch/chess-api-improvements/14-T-error-parse-helpers.md.
- Add decodeFENForHash hash-only fast path for HashFromFEN, avoiding the attack-set scan that decodeFENUnsafe now performs for Position.IsCheck / Position.Checkers correctness. - Add an occupancy-based hot-path filter to checkState so positions with no attacker on a queen/knight ray can skip the full slider/piece lookup. - Reuse the cached pos.checkers bitboard in newLegality instead of recomputing the full attack set when the side to move is in check. These changes recover most of the HashFromFEN regression caused by ticket 02 and turn PositionUpdate into a net win versus the pre-ticket baseline.
Pre-Chess960 cleanup derived from a full code review (linter + 9 parallel skill reviewers + v2 regression + shakmaty gap analysis). Hash API settlement: - MIGRATION.md/CHANGELOG.md now say Position.Hash() [16]byte is removed, not "kept as deprecated" - Document removal of GetPolyglotHashBytes, ZobristHasher, NewChessHasher, NewZobristHasher, ZobristHashToUint64 with migration paths Error sentinels: - Add ErrInvalidFEN and ErrInvalidPGN; ErrNoGameFound now wraps ErrInvalidPGN - Wire through decodeFEN/decodeFENSetup/PGNDecoder.Decode using Go 1.20+ multi-%w so errors.Is and errors.As both traverse the chain FEN parser relaxation (matches shakmaty behavior): - Accept 1-6 fields (board required); default missing fields to 8/8/8/8/8/8/8/8 w - - 0 1 - Accept underscores, tabs, and multiple spaces as separators - Accept 0 fullmove number, treat as 1 Setup helpers: - EmptySetup(), InitialSetup(), (Setup) SwapTurn(), (Setup) Mirror() Hygiene: - Remove 4 duplicate package comments (game.go, board.go, position.go, pgn.go) - Add Game.FEN() nil guard mirroring Game.Position() - doc.go: math/rand -> math/rand/v2, rand.Intn -> rand.IntN - Fix ErrIllegalMove wrap order (%s: %w) - Lowercase ST1005 error strings; add chess:/uci: prefixes to bare errors - Rename calcConvienceBBs -> calcConvenienceBBs - Fix docstring typos (TextUnarshaler, ParseNAG docstring, "staring") - Fix opening/eco.go err shadow Breaking renames (user-approved): - opening.Opening -> opening.Entry - (*PolyglotBook).GetRandomMove -> RandomMove - (*PolyglotBook).GetChessMoves -> ChessMoves Deferred-work tickets created at .scratch/post-review/ (move-filtering predicates, performance wins, goleak subpackage coverage, runnable examples). Lint: 33 -> 28 issues. All tests green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.