Remove complex callback logic.

This commit restores the performance optimization from 7ad5848, which
was accidentally broken in e5485f6 -- the next commit!

It goes back to a simpler way to factor out the `search` logic between
the three use cases:

1. find the solution to a problem when there's exactly one
2. find all solutions to a problem
3. determine if there's more than one solution to a problem

and to abort the search as soon as a solution is found in cases 2 and 3.

Previously, the callback was in charge of two aspects of the logic
(recording solutions and determining whether to abort the search).

Now these responsibilities are split between two arguments, and that's
easier to understand.

Author: aaugustin

go/sudoku/cmd/sudoku/main.go

@@ -163,7 +163,7 @@ func solve(estimate bool, format string, input string, output string, multiple b
 	if err != nil {
 		return err
 	}
-	solutions, difficulty := sudoku.Solve(&grid)
+	solutions, difficulty := sudoku.Solve(&grid, multiple)
 	if len(solutions) == 1 {
 		grid = solutions[0]
 		err = writeGrid(grid, format, output)
@@ -181,7 +181,7 @@ func solve(estimate bool, format string, input string, output string, multiple b
 				}
 			}
 		} else {
-			return fmt.Errorf("multiple solutions found (%d)", len(solutions))
+			return errors.New("multiple solutions found")
 		}
 	}
 	if estimate {

go/sudoku/cmd/sudoku/main_test.go

@@ -120,7 +120,7 @@ func TestSolve(t *testing.T) {
 	checkFileContents(t, []string{"sudoku", "solve", "-i", inputFile, "-o", outputFile}, outputFile, solutionConsole)
 
 	// Runtime errors
-	testDispatch(t, []string{"sudoku", "solve", problem2}, "", 1, "", "^multiple solutions found (6)\n$")
+	testDispatch(t, []string{"sudoku", "solve", problem2}, "", 1, "", "^multiple solutions found\n$")
 	testDispatch(t, []string{"sudoku", "solve", problem3}, "", 1, "", "^no solution found\n$")
 	testDispatch(t, []string{"sudoku", "solve", "-m", problem3}, "", 1, "", "^no solution found\n$")
 	testDispatch(t, []string{"sudoku", "solve", "ABC"}, "", 1, "", "cannot read problem: cell contains invalid value")

go/sudoku/generator.go

@@ -52,26 +52,21 @@ func (g *Grid) minimize() float64 {
 	for _, cell := range rand.Perm(81) {
 		g[cell], value = uint8(0), g[cell]
 		var s solver
-		var solved bool
+		var grids []Grid
 		s.init()
-		if s.load(g) && s.search(func(_ *Grid) bool {
-			// Another solution was already found, abort.
-			if solved {
-				return false
-			}
-			// First solution is found, continue.
-			solved = true
-			return true
-		}) {
+		if !s.load(g) {
+			panic("minimize expects a valid grid")
+		}
+		grids = s.search(grids, false)
+		if len(grids) == 0 {
+			panic("minimize expects a valid grid")
+		} else if len(grids) == 1 {
 			// Only one solution was found.
 			difficulty = s.difficulty()
 		} else {
 			// More than one solution was found, restore cell.
 			g[cell] = value
 		}
-		if !solved {
-			panic("minimize expects a valid grid")
-		}
 	}
 
 	return difficulty

go/sudoku/generator_test.go

@@ -26,7 +26,7 @@ func TestGenerate(t *testing.T) {
 			t.Errorf("seed = %d: expected between 50 and 65 zeroes, got %d", seed, zeroCount)
 		}
 
-		solutions, solveDifficulty := Solve(&grid)
+		solutions, solveDifficulty := Solve(&grid, false)
 		if len(solutions) != 1 {
 			t.Errorf("seed = %d: more than one solution", seed)
 		}

go/sudoku/server.go

@@ -150,7 +150,7 @@ func handleProblem(w http.ResponseWriter, r *http.Request) {
 		http.Error(w, err.Error(), 400)
 		return
 	}
-	solutions, difficulty := Solve(&grid)
+	solutions, difficulty := Solve(&grid, false)
 	if len(solutions) == 0 {
 		http.Error(w, "no solution found", 400)
 		return
@@ -169,7 +169,7 @@ func handleSolution(w http.ResponseWriter, r *http.Request) {
 		http.Error(w, err.Error(), 400)
 		return
 	}
-	solutions, difficulty := Solve(&grid)
+	solutions, difficulty := Solve(&grid, false)
 	if len(solutions) == 0 {
 		http.Error(w, "no solution found", 400)
 		return

go/sudoku/solver.go

@@ -124,16 +124,16 @@ func (s *solver) mark(cell int, value uint8) bool {
 
 // search finds all solutions.
 //
-// Each solution is reported by calling callback.
+// It appends solutions to the grids slice and returns that slice.
 //
-// If callback returns true, search continues and eventually returns true when
-// the search completes.
-//
-// If callback returns false, search aborts and returns false immediately.
-func (s *solver) search(callback func(*Grid) bool) bool {
+// When multiple is false, search aborts as soon as two solutions are found.
+// This is useful to know whether there's zero, one, or several solutions.
+// When multiple is true, it looks for all solutions.
+func (s *solver) search(grids []Grid, multiple bool) []Grid {
 	// If the grid is complete, there is a solution in this branch.
 	if s.progress == 81 {
-		return callback(&s.grid)
+		grids = append(grids, s.grid)
+		return grids
 	}
 
 	// Since s.next is empty, sharing the underlying array with a copy is OK.
@@ -151,15 +151,17 @@ func (s *solver) search(callback func(*Grid) bool) bool {
 		if s.conflicts[cell]&(1<<value) == 0 {
 			copy = *s
 			if copy.mark(cell, value) {
-				if !copy.search(callback) {
+				grids = copy.search(grids, multiple)
+				// Abort search when two solutions are found and we don't look for more.
+				if !multiple && len(grids) > 1 {
 					s.steps = copy.steps
-					return false
+					break
 				}
 			}
 			s.steps = copy.steps
 		}
 	}
-	return true
+	return grids
 }
 
 // candidate find the cell with the most conflicts.
@@ -193,17 +195,19 @@ func (s *solver) difficulty() float64 {
 	return math.Log(math.Max(float64(s.steps)/81, 1)) + 1
 }
 
-// Solve a grid. Return a slice of 0, 1, or several solutions, and an estimate
-// of how difficult the grid is.
-func Solve(g *Grid) ([]Grid, float64) {
+// Solve a grid.
+//
+// Return a slice of 0, 1, or several solutions, and an estimate of how
+// difficult the grid is.
+//
+// When multiple is false and there are multiple solutions, stop searching as
+// soon as two solutions are found.
+func Solve(g *Grid, multiple bool) ([]Grid, float64) {
 	var s solver
 	var grids []Grid
 	s.init()
 	if s.load(g) {
-		s.search(func(g *Grid) bool {
-			grids = append(grids, *g)
-			return true
-		})
+		grids = s.search(grids, multiple)
 	}
 	return grids, s.difficulty()
 }

go/sudoku/solver_test.go

@@ -59,7 +59,7 @@ func TestSolve(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		gotSolutions, gotDifficulty := Solve(&test.input)
+		gotSolutions, gotDifficulty := Solve(&test.input, true)
 		if len(gotSolutions) != len(test.wantSolutions) {
 			t.Errorf("%v: expected %d solution(s), got %d", test.input, len(test.wantSolutions), len(gotSolutions))
 			continue
@@ -110,6 +110,6 @@ func BenchmarkSolve(b *testing.B) {
 	}
 	for n := 0; n < b.N; n++ {
 		grid := grids[n%len(grids)]
-		Solve(&grid)
+		Solve(&grid, false)
 	}
 }

python/_sudoku/generator.c

@@ -102,25 +102,16 @@ void random_grid(uint8_t grid[]) {
     memcpy(grid, s.grid, sizeof(s.grid));
 }
 
-static bool minimize_callback(uint8_t grid[], void *solved) {
-    // Another solution was already found, abort.
-    if (*(bool *)solved) {
-        return false;
-    }
-    // First solution is found, continue.
-    *(bool *)solved = true;
-    return true;
-}
-
 // minimize turns a solution into a problem by removing values from cells.
 double minimize(uint8_t grid[]) {
     size_t order[81];
     size_t i;
     size_t cell;
     uint8_t value;
-    bool solved;
     solver s;
     size_t next[81];
+    bool loaded;
+    int solutions;
     double difficulty = 0;
 
     // Clear cells until this creates multiple solutions.
@@ -129,17 +120,18 @@ double minimize(uint8_t grid[]) {
         cell = order[i];
         value = grid[cell];
         grid[cell] = (uint8_t)0;
-        solved = false;
         solver_init(&s, next);
-        if (solver_load(&s, grid) &&
-            solver_search(&s, &minimize_callback, (void *)&solved)) {
+        loaded = solver_load(&s, grid);
+        assert(loaded);
+        solutions = solver_search(&s, NULL, false);
+        assert(solutions > 0);
+        if (solutions == 1) {
             // Only one solution was found.
             difficulty = solver_difficulty(&s);
         } else {
             // More than one solution was found, restore cell.
             grid[cell] = value;
         }
-        assert(solved);
     }
 
     return difficulty;

python/_sudoku/module.c

@@ -7,28 +7,11 @@
 #include "solver.h"
 #include "utils.h"
 
-static bool solve_callback(uint8_t values[], void *grids) {
-    PyGILState_STATE gstate;
-    gstate = PyGILState_Ensure();
-
-    PyObject *grid = Grid_AsPyObject(values);
-    if (grid == NULL) {
-        PyGILState_Release(gstate);
-        return false;
-    }
-    if (PyList_Append((PyObject *)grids, grid) < 0) {
-        PyGILState_Release(gstate);
-        return false;
-    }
-
-    PyGILState_Release(gstate);
-    return true;
-}
-
 static PyObject *
 _sudoku_solve(PyObject *self, PyObject *args, PyObject *kwds) {
-    static char *kwlist[] = {"grid", NULL};
+    static char *kwlist[] = {"grid", "multiple", NULL};
     PyObject *grid;
+    bool multiple = false;
     uint8_t values[81];
     PyObject *grids;
     solver s;
@@ -37,7 +20,7 @@ _sudoku_solve(PyObject *self, PyObject *args, PyObject *kwds) {
     PyObject *result;
 
     // borrows a reference to grid
-    if (!PyArg_ParseTupleAndKeywords(args, kwds, "O", kwlist, &grid)) {
+    if (!PyArg_ParseTupleAndKeywords(args, kwds, "O|p", kwlist, &grid, &multiple)) {
         return NULL;
     }
 
@@ -53,14 +36,13 @@ _sudoku_solve(PyObject *self, PyObject *args, PyObject *kwds) {
     Py_BEGIN_ALLOW_THREADS
     solver_init(&s, next);
     if (solver_load(&s, values)) {
-        solver_search(&s, solve_callback, grids);
+        if (solver_search(&s, grids, multiple) < 0) {
+            Py_DECREF(grids);
+            return NULL;
+        }
     }
     Py_END_ALLOW_THREADS
-    // solve_callback may cause a Python error.
-    if (PyErr_Occurred()) {
-        Py_DECREF(grids);
-        return NULL;
-    }
+
     difficulty = solver_difficulty(&s);
 
     result = Py_BuildValue("Od", grids, difficulty);
@@ -96,7 +78,7 @@ _sudoku_generate(PyObject *self, PyObject *args) {
 
 static PyMethodDef _sudoku_methods[] = {
     {"solve",  (PyCFunction)_sudoku_solve, METH_VARARGS | METH_KEYWORDS,
-     PyDoc_STR("Solve a grid.\n\nReturn a list of 0, 1, or several solutions.")},
+     PyDoc_STR("Solve a grid.")},
     {"generate", (PyCFunction)_sudoku_generate, METH_VARARGS,
      PyDoc_STR("Create a random problem.")},
     {NULL, NULL, 0, NULL}

python/_sudoku/solver.c

@@ -186,10 +186,35 @@ static size_t solver_candidate(solver *s) {
     return candidate;
 }
 
-bool solver_search(solver *s, bool callback(uint8_t[], void *), void *arg) {
+static bool record_solution(PyObject *grids, uint8_t values[]) {
+    // Re-acquire the GIL, which is released when solver_search runs.
+    PyGILState_STATE gstate;
+    gstate = PyGILState_Ensure();
+
+    PyObject *grid = Grid_AsPyObject(values);
+    if (grid == NULL) {
+        PyGILState_Release(gstate);
+        return false;
+    }
+    if (PyList_Append(grids, grid) < 0) {
+        PyGILState_Release(gstate);
+        return false;
+    }
+
+    PyGILState_Release(gstate);
+    return true;
+}
+
+// Return the number of solutions found, or -1 if an error occurred.
+int solver_search(solver *s, PyObject *grids, bool multiple) {
     // If the grid is complete, there is a solution in this branch.
     if (s->progress == 81) {
-        return callback(s->grid, arg);
+        if (grids != NULL) {
+            if (!record_solution(grids, s->grid)) {
+                return -1;
+            };
+        }
+        return 1;
     }
 
     // Since next is empty, sharing the underlying array with a copy is OK.
@@ -202,19 +227,28 @@ bool solver_search(solver *s, bool callback(uint8_t[], void *), void *arg) {
     solver copy;
     uint8_t value;
     size_t cell = solver_candidate(s);
+    int solutions = 0;
+    int new_solutions;
     for (value = 1; value < 10; value++) {
         if ((s->conflicts[cell] & ((uint16_t)1 << value)) == 0) {
             memcpy(&copy, s, sizeof(solver));
             if (solver_mark(&copy, cell, value)) {
-                if (!solver_search(&copy, callback, arg)) {
+                new_solutions = solver_search(&copy, grids, multiple);
+                if (new_solutions < 0) {
                     s->steps = copy.steps;
-                    return false;
+                    return new_solutions;
+                }
+                solutions += new_solutions;
+                // Abort search when two solutions are found and we don't look for more.
+                if (!multiple && solutions > 1) {
+                    s->steps = copy.steps;
+                    return solutions;
                 }
             }
             s->steps = copy.steps;
         }
     }
-    return true;
+    return solutions;
 }
 
 double solver_difficulty(solver *s) {