Perf/polyglot map #26
Conversation
WalkthroughThe changes refactor the methods for processing chess moves. In Changes
Sequence Diagram(s)UCI Notation Decoding FlowsequenceDiagram
participant U as User
participant ND as Notation Decoder
participant SC as Square Calculator
participant PB as Promotion Handler
participant MC as Move Constructor
U->>ND: Provide position & UCI string
ND->>SC: Calculate square indices via arithmetic
SC-->>ND: Return computed indices
ND->>ND: Validate indices with bounds check
ND->>PB: Lookup promotion using precomputed array
PB-->>ND: Return promotion piece
ND->>MC: Build move (add castling/en passant tags)
MC-->>ND: Return move value
ND->>U: Return move object
Polyglot Conversion FlowsequenceDiagram
participant U as User
participant PC as Polyglot Converter
participant CL as Castling Logic
participant BA as Byte Array Builder
U->>PC: Provide Polyglot move input
PC->>CL: Invoke convertPolyglotCastleToUCI (fromFile, toFile, rank)
CL-->>PC: Return four castling bytes
PC->>BA: Build move string using byte array
BA-->>PC: Return move string
PC->>U: Return finalized move
Possibly related PRs
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #26 +/- ##
==========================================
- Coverage 74.48% 70.24% -4.25%
==========================================
Files 27 27
Lines 5245 4476 -769
==========================================
- Hits 3907 3144 -763
+ Misses 1202 1197 -5
+ Partials 136 135 -1 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
polyglot.go (1)
84-93: Refined castling coordinate conversion
This function neatly converts standard castling squares. Consider verifying the rank if additional validation is ever needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
notation.go(2 hunks)polyglot.go(2 hunks)polyglot_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: vulncheck
🔇 Additional comments (13)
notation.go (7)
139-141: Use of direct arithmetic for square derivation
This replacement of map lookups with direct arithmetic is more efficient and reduces overhead.
143-143: Validating square bounds
Good job ensuring the squares fall within valid chessboard bounds.
147-147: Constructing the move object
Initializing the move struct via literal is straightforward and clear.
149-154: Efficient promotion lookup
Using a 256-entry ASCII lookup array is both clear and efficient.
162-162: Return early when position is nil
This early return is correct for handling cases with no position context.
165-165: Castling check comment
This comment provides clarity about the castling logic flow.
182-182: Final return statement
No additional observations here.polyglot.go (5)
97-101: Building move buffer
Populating the byte array for source/target squares is concise and efficient.
107-112: Promotion handling with an extra byte
Including the promotion piece byte is straightforward and aligns with the numeric piece codes.
121-121: Tagging king-side or queen-side castling
The condition checks make sense for finalizing castling tags.
500-500: Pre-allocate map
Creating the map with an initial capacity equals the number of entries is a solid optimization.
104-106: Rewrite castling coordinates
Ensure that all castling moves truly originate from the 'e' file to avoid unexpected transformations.Would you like a script to confirm no unexpected calls to this helper with invalid parameters?
✅ Verification successful
Castling Calls Verified — No Unexpected Parameters
The search confirmed that the only usage of
convertPolyglotCastleToUCIproperly passes parameters ensuring the king’s move always originates from the 'e' file. The function itself checks that thefromFileis 'e' before applying the castling transformation. No invalid or unexpected calls were found.
- The function is only called with
moveBuf[0]as the source file.- The internal check (
if fromFile == 'e') prevents handling invalid cases.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # We'll search all references to convertPolyglotCastleToUCI to see how it's used. rg "convertPolyglotCastleToUCI" -A 5Length of output: 618
polyglot_test.go (1)
746-778: New benchmark for ToMoveMap
Great addition to measure performance of map construction. You might consider parameterizing data sizes to gather more comprehensive benchmarks, but this is a good start.
…g to reduce allocs and improve performances by 200%
…ance and reduced allocations
…ot hash retrieval
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
hashes.go (4)
14-14: End the comment with a period.According to the static analysis hint, style guidelines mandate that comments end with a period.
-// GetPolyglotHashBytes returns a precomputed byte slice for a given index +// GetPolyglotHashBytes returns a precomputed byte slice for a given index.🧰 Tools
🪛 golangci-lint (1.62.2)
14-14: Comment should end in a period
(godot)
19-19: End the comment with a period.According to the static analysis hint, style guidelines mandate that comments end with a period.
-// GetPolyglotHashes returns a reference to the static hash array +// GetPolyglotHashes returns a reference to the static hash array.🧰 Tools
🪛 golangci-lint (1.62.2)
19-19: Comment should end in a period
(godot)
20-21: Add unit tests to cover these lines.The code coverage tool indicates that lines 20–21 are untested. Consider adding a test to verify that
GetPolyglotHashes()returns the proper data.Here’s a minimal test snippet you could add to your test suite:
+func TestGetPolyglotHashes(t *testing.T) { + got := GetPolyglotHashes() + if len(got) != len(polyglotHashes) { + t.Errorf("Expected length %d, got %d", len(polyglotHashes), len(got)) + } +}🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 20-21: hashes.go#L20-L21
Added lines #L20 - L21 were not covered by tests
24-806: Consider externalizing the large polyglot hash data.Storing an 800+ line array of strings in code can bloat the binary and slow down compile times. An alternative approach could store these entries in an external resource (e.g., a file, compressed data, or a smaller specialized format) for a more modular, maintainable, and potentially more memory-efficient design.
zobrist.go (1)
235-250: Add test coverage for FEN validation and extract magic numbers.The FEN validation has been improved, but:
- New validation code paths are not covered by tests.
- Magic numbers should be constants.
Consider extracting magic numbers into constants:
+const ( + fenMinParts = 4 + maxCastlingRights = 4 +)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 245-246: zobrist.go#L245-L246
Added lines #L245 - L246 were not covered by tests
[warning] 249-250: zobrist.go#L249-L250
Added lines #L249 - L250 were not covered by testszobrist_test.go (1)
265-274: Consider adding more benchmark cases.The benchmark covers the starting position, but consider adding cases for:
- Empty board
- Complex positions with en passant
- Different castling rights
Example:
func BenchmarkHashPosition_EmptyBoard(b *testing.B) { hasher := NewZobristHasher() fen := "8/8/8/8/8/8/8/8 w - - 0 1" b.ReportAllocs() for i := 0; i < b.N; i++ { _, _ = hasher.HashPosition(fen) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
hashes.go(1 hunks)zobrist.go(9 hunks)zobrist_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
zobrist.go
72-72: for loop can be changed to use an integer range (Go 1.22+)
(intrange)
85-85: for loop can be changed to use an integer range (Go 1.22+)
(intrange)
53-53: Magic number: 2, in detected
(mnd)
hashes.go
14-14: Comment should end in a period
(godot)
19-19: Comment should end in a period
(godot)
🪛 GitHub Check: codecov/patch
zobrist.go
[warning] 49-50: zobrist.go#L49-L50
Added lines #L49 - L50 were not covered by tests
[warning] 56-56: zobrist.go#L56
Added line #L56 was not covered by tests
[warning] 70-71: zobrist.go#L70-L71
Added lines #L70 - L71 were not covered by tests
[warning] 77-77: zobrist.go#L77
Added line #L77 was not covered by tests
[warning] 82-83: zobrist.go#L82-L83
Added lines #L82 - L83 were not covered by tests
[warning] 86-86: zobrist.go#L86
Added line #L86 was not covered by tests
[warning] 245-246: zobrist.go#L245-L246
Added lines #L245 - L246 were not covered by tests
[warning] 249-250: zobrist.go#L249-L250
Added lines #L249 - L250 were not covered by tests
[warning] 259-259: zobrist.go#L259
Added line #L259 was not covered by tests
[warning] 282-282: zobrist.go#L282
Added line #L282 was not covered by tests
hashes.go
[warning] 20-21: hashes.go#L20-L21
Added lines #L20 - L21 were not covered by tests
🔇 Additional comments (5)
hashes.go (1)
3-3: Import of "encoding/hex" is appropriate and clear.This import accurately supports hex string decoding for the hash data.
zobrist.go (4)
46-60: Add test coverage for error handling in parseHexString.The function has been improved to use
hex.Decodeand validate input length. However, the error handling code paths are not covered by tests.Please add test cases for:
- Odd-length hex strings
- Invalid hex characters
🧰 Tools
🪛 golangci-lint (1.62.2)
53-53: Magic number: 2, in detected
(mnd)
🪛 GitHub Check: codecov/patch
[warning] 49-50: zobrist.go#L49-L50
Added lines #L49 - L50 were not covered by tests
[warning] 56-56: zobrist.go#L56
Added line #L56 was not covered by tests
90-97: LGTM! Efficient in-place XOR operation.The method has been optimized to perform XOR operations in-place, reducing memory allocations.
272-286: LGTM! Simplified hash conversion.The function has been improved to use
strconv.ParseUintdirectly, making it simpler and more efficient.🧰 Tools
🪛 golangci-lint (1.62.2)
283-283: unnecessary trailing newline
(whitespace)
🪛 GitHub Check: codecov/patch
[warning] 282-282: zobrist.go#L282
Added line #L282 was not covered by tests
67-88: Document panic condition and add test coverage.The
xorArraysIntofunction can panic if the output buffer is too small. This should be documented and tested.
- Add a doc comment explaining the panic condition.
- Add test cases for:
- Arrays of different lengths
- Output buffer too small
- Empty arrays
Additionally, consider using Go 1.22's integer range feature for the loops:
-for i := 0; i < length; i++ { +for i := range length {🧰 Tools
🪛 golangci-lint (1.62.2)
72-72: for loop can be changed to use an integer range (Go 1.22+)
(intrange)
85-85: for loop can be changed to use an integer range (Go 1.22+)
(intrange)
🪛 GitHub Check: codecov/patch
[warning] 70-71: zobrist.go#L70-L71
Added lines #L70 - L71 were not covered by tests
[warning] 77-77: zobrist.go#L77
Added line #L77 was not covered by tests
[warning] 82-83: zobrist.go#L82-L83
Added lines #L82 - L83 were not covered by tests
[warning] 86-86: zobrist.go#L86
Added line #L86 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
polyglot.go (1)
120-126: Make castling detection more robust.The current implementation assumes file indices (4 for king, 0/2 for queen-side). Consider using constants for better maintainability and clarity.
+// File indices for castling detection +const ( + kingStartFile = 4 // 'e' file + queenSideFile = 2 // 'c' file + queenRookFile = 0 // 'a' file +) if pm.CastlingMove { - if pm.FromFile == 4 && (pm.ToFile == 0 || pm.ToFile == 2) { + if pm.FromFile == kingStartFile && (pm.ToFile == queenRookFile || pm.ToFile == queenSideFile) { decode.AddTag(QueenSideCastle) } else { decode.AddTag(KingSideCastle) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
engine.go(5 hunks)move.go(1 hunks)notation.go(1 hunks)pgn.go(2 hunks)polyglot.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
pgn.go
[warning] 244-244: pgn.go#L244
Added line #L244 was not covered by tests
polyglot.go
[warning] 93-93: polyglot.go#L93
Added line #L93 was not covered by tests
notation.go
[warning] 169-169: notation.go#L169
Added line #L169 was not covered by tests
[warning] 171-171: notation.go#L171
Added line #L171 was not covered by tests
[warning] 174-174: notation.go#L174
Added line #L174 was not covered by tests
[warning] 179-179: notation.go#L179
Added line #L179 was not covered by tests
[warning] 182-182: notation.go#L182
Added line #L182 was not covered by tests
🔇 Additional comments (7)
polyglot.go (3)
96-113: LGTM! Performance improvements in string construction.The use of byte arrays for move string construction and promotion lookup reduces allocations and improves performance.
499-511: LGTM! Efficient map initialization.Pre-allocating the map capacity with
len(book.entries)reduces rehashing operations and improves performance.
84-94: Add test coverage for the default case.The default case in
convertPolyglotCastleToUCI(line 93) is not covered by tests. This could hide potential issues when handling non-castling moves.Document the king's starting file assumption.
The function assumes 'e' is the king's starting file. This should be documented for maintainability.
-func convertPolyglotCastleToUCI(fromFile, toFile, rank byte) (byte, byte, byte, byte) { +// convertPolyglotCastleToUCI converts polyglot castling moves to UCI format. +// It assumes 'e' as the king's starting file, and 'a'/'h' as rook files. +// Returns fromFile, fromRank, toFile, toRank bytes. +func convertPolyglotCastleToUCI(fromFile, toFile, rank byte) (byte, byte, byte, byte) {🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 93-93: polyglot.go#L93
Added line #L93 was not covered by testsnotation.go (2)
139-145: LGTM! Performance improvement for square computation.The change from map lookup to direct arithmetic computation is more efficient. The bounds check ensures valid squares (A1 to H8).
149-159: LGTM! Efficient promotion piece lookup.Using a precomputed array for promotion piece lookup is more efficient than a switch statement or map lookup.
engine.go (2)
179-193: LGTM! Consistent use of AddTag.The changes correctly use the public
AddTagmethod for adding move tags.
330-370: LGTM! Consistent use of AddTag in castle moves.The changes correctly use the public
AddTagmethod for adding castle move tags.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
notation_test.go (1)
283-287: Fix potential array index out of bounds in benchmark.The current implementation could cause an index out of bounds error:
move := moves[i%len(moves)][i%len(moves[i%len(moves)])]The nested modulo operations could result in accessing an invalid index.
Suggested fix:
-move := moves[i%len(moves)][i%len(moves[i%len(moves)])] +posIndex := i % len(moves) +moveIndex := i % len(moves[posIndex]) +move := moves[posIndex][moveIndex]
🧹 Nitpick comments (1)
hashes.go (1)
8-18: Document panic behavior in initialization.The initialization function panics on invalid hex strings, but this behavior is not documented. This could surprise developers who maintain the code.
Add a comment explaining the panic behavior:
+// polyglotHashesBytes is initialized at package load time. +// It will panic if any of the polyglot hashes contain invalid hex strings. var polyglotHashesBytes = func() [len(polyglotHashes)][8]byte {🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 13-13: hashes.go#L13
Added line #L13 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
hashes.go(1 hunks)move.go(1 hunks)notation.go(1 hunks)notation_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- move.go
- notation.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
hashes.go
20-20: Comment should end in a period
(godot)
25-25: Comment should end in a period
(godot)
🪛 GitHub Check: codecov/patch
hashes.go
[warning] 13-13: hashes.go#L13
Added line #L13 was not covered by tests
[warning] 26-27: hashes.go#L26-L27
Added lines #L26 - L27 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: vulncheck
🔇 Additional comments (2)
notation_test.go (1)
152-241: Test cases look comprehensive and well-structured.The test cases cover a good range of scenarios including:
- Basic moves without promotion
- Moves with promotion
- Captures
- Checks
- En passant moves
- Invalid cases (length, squares, promotion pieces)
hashes.go (1)
25-28: Add test coverage for GetPolyglotHashes.The static analysis indicates that this function is not covered by tests.
Let's verify if there are any existing tests for this function:
🧰 Tools
🪛 golangci-lint (1.62.2)
25-25: Comment should end in a period
(godot)
🪛 GitHub Check: codecov/patch
[warning] 26-27: hashes.go#L26-L27
Added lines #L26 - L27 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
hashes.go (2)
20-20: Fix comment formatting.Add a period at the end of the comment to follow Go's comment formatting conventions.
-// GetPolyglotHashBytes returns a precomputed byte slice for a given index +// GetPolyglotHashBytes returns a precomputed byte slice for a given index.🧰 Tools
🪛 golangci-lint (1.62.2)
20-20: Comment should end in a period
(godot)
28-28: Fix comment formatting.Add a period at the end of the comment to follow Go's comment formatting conventions.
-// GetPolyglotHashes returns a reference to the static hash array +// GetPolyglotHashes returns a reference to the static hash array.🧰 Tools
🪛 golangci-lint (1.62.2)
28-28: Comment should end in a period
(godot)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hashes.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
hashes.go
20-20: Comment should end in a period
(godot)
28-28: Comment should end in a period
(godot)
🪛 GitHub Check: codecov/patch
hashes.go
[warning] 13-13: hashes.go#L13
Added line #L13 was not covered by tests
[warning] 23-24: hashes.go#L23-L24
Added lines #L23 - L24 were not covered by tests
[warning] 29-30: hashes.go#L29-L30
Added lines #L29 - L30 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: vulncheck
🔇 Additional comments (3)
hashes.go (3)
8-18: Add test coverage for hex decoding error handling.The error handling path for invalid hex strings is not covered by tests. Consider adding test cases with invalid hex strings to ensure the error handling works as expected.
Would you like me to help generate test cases for the error handling path?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 13-13: hashes.go#L13
Added line #L13 was not covered by tests
21-26: Add test coverage for bounds checking.The bounds checking logic is not covered by tests. Consider adding test cases with out-of-bounds indices to ensure the function handles invalid inputs correctly.
Would you like me to help generate test cases for the bounds checking?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 23-24: hashes.go#L23-L24
Added lines #L23 - L24 were not covered by tests
29-31: Add test coverage for GetPolyglotHashes.The function is not covered by tests. Consider adding test cases to verify it returns the correct reference to the hash array.
Would you like me to help generate test cases for this function?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 29-30: hashes.go#L29-L30
Added lines #L29 - L30 were not covered by tests
Summary by CodeRabbit
Refactor
Tests