V3#123
Draft
CorentinGS wants to merge 112 commits into
Draft
Conversation
Extract decodeSANText as the single SAN parse+resolve entry point, parameterised by canonical flag. MoveTextCodec.Decode now owns the canonical decision via policy; redundant validatePositionMove removed for SAN and long algebraic (resolveSANMove already guarantees legality). Replace parseMove's 180-line token-by-token SAN reconstruction with grammar-aware token collection that delegates to the codec. Delete dead helpers: isPromotionDestination, parsePieceType, parseSquare, and the now-unnecessary algebraicNotation.Decode wrapper. Net -120 lines. One locality for SAN grammar, leniency, legality, and diagnostics.
Board.update and Position.updateHash both read a shared moveEffect struct so the board mutation and the Zobrist hash delta cannot drift on en-passant squares, castle rook squares, or capture targets. Special-move effects (en-passant direction, castle rook rank) are derived from the moving piece's color rather than Position.turn, so unsafe moves through Game.UnsafeMove / Game.UnsafePushMoveText cannot apply effects for the wrong side; Board.update captures before shifting the mover's color aggregate so the same square can be cleared and re-added under the mover's color without leaving whiteSqs/blackSqs out of sync with the per-piece bitboards and mailbox. TestComputeMoveEffect pins the descriptor across quiet moves, captures, en-passant for both colors, all four castles, both promotions, and the degenerate empty-origin case. TestApplyMoveDifferential walks the perft positions in lockstep and asserts incremental hash equals full recompute at every node. TestUnsafeTransitionConsistency guards the three unsafe paths the consolidation could regress: out-of-turn black castling, out-of-turn en-passant tagging, and the unsafe king-on-friendly-square move. Renumber docs/adr/0001-zobrist-hash-consolidation.md to 0017 so each ADR has a unique sequence number.
Avoid decoding the constant starting FEN for every parsed Game. BenchmarkPGN_FullGameDecode_BigBig on linux/amd64, Ryzen 9 5950X: time -20.50%, bytes -0.78%, allocations -0.41%, throughput +25.78%.
Resolve ordinary pawn pushes from destination-directed candidates instead of falling back to full legal-move enumeration. BenchmarkPGN_FullGameDecode_BigBig on linux/amd64, Ryzen 9 5950X: time -35.57%, bytes -0.35%, allocations -1.60%, throughput +55.24%.
Return one-character move token values as slices of the lexer input instead of allocating strings. BenchmarkPGN_FullGameDecode_BigBig on linux/amd64, Ryzen 9 5950X: time -1.37%, bytes -1.56%, allocations -32.70%, throughput +1.40%.
Return tag values as lexer input slices unless escape decoding requires a builder. Benchmarks on linux/amd64, Ryzen 9 5950X: BenchmarkPGNRecord_Tags_Big: time -7.07%, bytes -3.69%, allocations -79.00%. BenchmarkPGN_FullGameDecode_BigBig: time -5.40%, bytes -0.75%, allocations -7.83%.
Build FEN strings directly into byte buffers, add allocation-free move continuation iteration, and simplify PGN parser checks and error formatting. BenchmarkPGN_FullGameDecode_BigBig on linux/amd64, Ryzen 9 5950X: no significant time or throughput change (p=0.684); allocations unchanged.
Architecture deepening PR 1-4 (PR 5 deferred to a later beta): - remove Position.ChangeTurn(), Game.Comments()/comments field, and dead sortedCommandKeys helper; amend ADR-011 (annotations live on MoveNode) - Game.copy() no longer aliases the move tree; Clone, UnmarshalText, Split and the test PGN option set it explicitly, removing a latent shared-tree footgun with no observable behaviour change - move parsePGNText + lexerTokenSource from framer.go to pgn.go next to the Parser they feed; framer.go header now honestly framing + tokenize bridge - relabel notation.go and notation_resolver.go (renamed from san_resolver.go) as MoveTextCodec internals (ADR-013, ADR-016); no behaviour change go test ./... green; golangci-lint adds no new issues (21 pre-existing).
MoveTree now owns rootPos, pos (live cursor view), and undos stack. addMove/GoForward/GoBack/setCurrent all advance pos via the in-place makeMove/unmakeMove pair from perft.go instead of re-deriving positions via Update. setCurrent has three fast paths (direct child, strict ancestor, replay-from-root) to keep alloc cost minimal during linear parsing and variation entry/exit. Per-node MoveNode.position is still populated from t.pos.copy() so external readers and addVariationUnchecked keep working; ticket 04 removes that field. Alloc budget bumped ~1% to absorb the new undos slice growth cost (big: 685k→700k, big_big: 2.85M→2.9M). Ticket 04 will ratchet these back below the pre-cursor ceilings. Observed alloc/run: big.pgn 690k, big_big.pgn 2.86M.
PositionCursor is a thin handle onto the tree active cursor (ADR-016 single-active-cursor invariant). Methods: - Peek(): zero-copy read of live position (callers must not mutate) - Position(): defensive copy (legacy Game.Position semantics) - ForwardMain/Forward(idx)/Back/Goto/Reset: navigation via tree makeMove Multiple cursors on the same tree share state, matching the existing tree-owned cursor model. Tests cover navigation round-trips, defensive copy semantics, shared-state invariant, and nil safety.
- MoveNode drops position *Position; gains tree *MoveTree back-pointer via setTree - Position() routes through cursor; saves/restores active cursor to avoid disturbing callers that compose it with other cursor-dependent reads - numOfRepetitions and PGN renderer save/restore cursor around main-line walks - AddVariation saves/restores cursor so the active position is unchanged - MoveHistory: defensive copies captured before ForwardMain mutates t.pos - game_split: setTree patches back-pointer; setCurrent replays cursor - Alloc budget: Big 690k->550k, Big_big 2.86M->1.98M
- MoveHistory/MoveHistoryEntry types removed
- New types: MoveList ([]MoveListEntry) and MoveListEntry {Move, Comments}
- Game.MoveList() replaces Game.MoveHistory(); no *Position cached per entry
- Callers needing positions replay via Game.MoveTree() + PositionCursor
- README example updated to walk the cursor alongside the move list
- All MoveHistory tests migrated to MoveList; position-invariant tests
dropped (positions no longer part of the type)
- Alloc budget unchanged from ticket 04 (MoveList path was already lazy)
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)
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.