Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid string operations/allocation in newBitboard #75

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tov
Copy link

@tov tov commented Jun 11, 2021

Instead of building a string of '0's and '1' characters and then converting it, newBitboard uses bit arithmetic to convert a map to a uint64. This avoids unnecessary memory allocation and string processing.

Instead of building a string of "0"s and "1"s and then converting it,
newBitboard now uses bit arithmetic to convert a map to a uint64. This
avoids memory allocation and a bunch of string operations.
@notnil
Copy link
Owner

notnil commented Sep 8, 2021

@tov could you add a test for this change? Also I am not sure this is performance sensitive code. What motivated this change?

@bbars
Copy link
Contributor

bbars commented Jul 12, 2022

@notnil, here's a test (sorry :)

diff --git a/bitboard_test.go b/bitboard_test.go
index 0000000..3c0a55f 100644
--- a/bitboard_test.go
+++ b/bitboard_test.go
@@ -7,6 +7,11 @@ type bitboardTestPair struct {
 	reversed uint64
 }
 
+type bitboardTestNew struct {
+	smap map[Square]bool
+	bits uint64
+}
+
 var (
 	tests = []bitboardTestPair{
 		{
@@ -22,6 +27,50 @@ var (
 			uint64(0),
 		},
 	}
+	newBitboardTests = []bitboardTestNew{
+		{
+			map[Square]bool{},
+			0b_00000000_00000000_00000000_00000000_00000000_00000000_00000000_00000000,
+			// all zeroes
+		},
+		{
+			map[Square]bool{
+				A1: true,
+			},
+			0b_10000000_00000000_00000000_00000000_00000000_00000000_00000000_00000000,
+			// ^
+		},
+		{
+			map[Square]bool{
+				B1: true,
+			},
+			0b_01000000_00000000_00000000_00000000_00000000_00000000_00000000_00000000,
+			//  ^
+		},
+		{
+			map[Square]bool{
+				B3: true,
+			},
+			0b_00000000_00000000_01000000_00000000_00000000_00000000_00000000_00000000,
+			//                    ^
+		},
+		{
+			map[Square]bool{
+				H8: true,
+			},
+			0b_00000000_00000000_00000000_00000000_00000000_00000000_00000000_00000001,
+			//                                                                       ^
+		},
+		{
+			map[Square]bool{
+				A1: true,
+				B3: true,
+				H8: true,
+			},
+			0b_10000000_00000000_01000000_00000000_00000000_00000000_00000000_00000001,
+			// ^                  ^                                                  ^
+		},
+	}
 )
 
 func TestBitboardReverse(t *testing.T) {
@@ -53,3 +102,12 @@ func BenchmarkBitboardReverse(b *testing.B) {
 func intStr(i uint64) string {
 	return bitboard(i).String()
 }
+
+func TestBitboardNew(t *testing.T) {
+	for _, c := range newBitboardTests {
+		bb := newBitboard(c.smap)
+		if uint64(bb) != c.bits {
+			t.Fatalf("new bitboard from %v expected %s but got %s", c.smap, intStr(c.bits), bb)
+		}
+	}
+}

bbars added a commit to bbars/chess that referenced this pull request Jul 12, 2022
Instead of building a string of "0"s and "1"s and then converting it,
newBitboard now uses bit arithmetic to convert a map to a uint64. This
avoids memory allocation and a bunch of string operations.
Add test for this change.

Done by @tov: https://github.com/tov
Original PR: notnil#75
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants