[CP-SAT] cleanup C and go API

Author: lperron

ortools/sat/c_api/BUILD.bazel

@@ -25,7 +25,6 @@ cc_library(
         "//ortools/sat:cp_model_solver",
         "//ortools/sat:model",
         "//ortools/sat:sat_parameters_cc_proto",
-        "//ortools/util:time_limit",
         "@com_google_absl//absl/log:check",
     ],
 )

ortools/sat/c_api/cp_solver_c.cc

@@ -13,7 +13,6 @@
 
 #include "ortools/sat/c_api/cp_solver_c.h"
 
-#include <atomic>
 #include <string>
 
 #include "absl/log/check.h"
@@ -22,19 +21,15 @@
 #include "ortools/sat/cp_model_solver.h"
 #include "ortools/sat/model.h"
 #include "ortools/sat/sat_parameters.pb.h"
-#include "ortools/util/time_limit.h"
 
 namespace operations_research::sat {
 
 namespace {
 
-CpSolverResponse solveWithParameters(std::atomic<bool>* const limit_reached,
-                                     const CpModelProto& proto,
+CpSolverResponse solveWithParameters(Model* model, const CpModelProto& proto,
                                      const SatParameters& params) {
-  Model model;
-  model.Add(NewSatParameters(params));
-  model.GetOrCreate<TimeLimit>()->RegisterExternalBooleanAsLimit(limit_reached);
-  return SolveCpModel(proto, &model);
+  model->Add(NewSatParameters(params));
+  return SolveCpModel(proto, model);
 }
 
 }  // namespace
@@ -44,31 +39,28 @@ extern "C" {
 void SolveCpModelWithParameters(const void* creq, int creq_len,
                                 const void* cparams, int cparams_len,
                                 void** cres, int* cres_len) {
-  return SolveCpInterruptible(nullptr, creq, creq_len, cparams, cparams_len,
-                              cres, cres_len);
+  Model model;
+  SolveCpInterruptible(&model, creq, creq_len, cparams, cparams_len, cres,
+                       cres_len);
 }
 
-void* SolveCpNewAtomicBool() { return new std::atomic<bool>(false); }
+void* SolveCpNewEnv() { return new Model(); }
 
-void SolveCpDestroyAtomicBool(void* const atomic_bool) {
-  delete static_cast<std::atomic<bool>*>(atomic_bool);
-}
+void SolveCpDestroyEnv(void* const cenv) { delete static_cast<Model*>(cenv); }
 
-void SolveCpStopSolve(void* const atomic_bool) {
-  *static_cast<std::atomic<bool>*>(atomic_bool) = true;
-}
+void SolveCpStopSolve(void* cenv) { StopSearch(static_cast<Model*>(cenv)); }
 
-void SolveCpInterruptible(void* const limit_reached, const void* creq,
-                          int creq_len, const void* cparams, int cparams_len,
-                          void** cres, int* cres_len) {
+void SolveCpInterruptible(void* const cenv, const void* creq, int creq_len,
+                          const void* cparams, int cparams_len, void** cres,
+                          int* cres_len) {
   CpModelProto req;
   CHECK(req.ParseFromArray(creq, creq_len));
 
   SatParameters params;
   CHECK(params.ParseFromArray(cparams, cparams_len));
 
-  CpSolverResponse res = solveWithParameters(
-      static_cast<std::atomic<bool>*>(limit_reached), req, params);
+  CpSolverResponse res =
+      solveWithParameters(static_cast<Model*>(cenv), req, params);
 
   std::string res_str;
   CHECK(res.SerializeToString(&res_str));

ortools/sat/c_api/cp_solver_c.h

@@ -25,12 +25,12 @@ void SolveCpModelWithParameters(const void* creq, int creq_len,
                                 const void* cparams, int cparams_len,
                                 void** cres, int* cres_len);
 
-void* SolveCpNewAtomicBool();
-void SolveCpDestroyAtomicBool(void* atomic_bool);
-void SolveCpStopSolve(void* atomic_bool);
+void* SolveCpNewEnv();
+void SolveCpDestroyEnv(void* cenv);
+void SolveCpStopSolve(void* cenv);
 // Allows for interruptible solves. Solves can be interrupted by calling
-// `SolveCpStopSolve` with the `limit_reached` atomic Boolean.
-void SolveCpInterruptible(void* limit_reached, const void* creq, int creq_len,
+// `SolveCpStopSolve` with the `cenv` argument.
+void SolveCpInterruptible(void* cenv, const void* creq, int creq_len,
                           const void* cparams, int cparams_len, void** cres,
                           int* cres_len);
 

ortools/sat/cp_model.proto

@@ -827,10 +827,12 @@ message CpSolverResponse {
 
   // Advanced usage.
   //
-  // A lower bound on the inner integer expression of the objective. This is
-  // either a bound on the expression in the returned integer_objective or on
-  // the integer expression of the original objective if the problem already has
-  // an integer objective.
+  // A lower bound on the integer expression of the objective. This is either a
+  // bound on the expression in the returned integer_objective or on the integer
+  // expression of the original objective if the problem already has an integer
+  // objective.
+  //
+  // TODO(user): This should be renamed integer_objective_lower_bound.
   int64 inner_objective_lower_bound = 29;
 
   // Some statistics about the solve.

ortools/sat/go/cpmodel/cp_solver.go

@@ -71,12 +71,12 @@ func SolveCpModelWithParameters(input *cmpb.CpModelProto, params *sppb.SatParame
 }
 
 // SolveCpModelInterruptibleWithParameters solves a CP Model with the given input proto
-// and parameters and returns a CPSolverResponse. The solve can be interrupted by triggering
-// the `interrupt`.
+// and parameters and returns a CPSolverResponse. The solve can be interrupted by calling
+// the `stopSolve`.
 func SolveCpModelInterruptibleWithParameters(input *cmpb.CpModelProto, params *sppb.SatParameters, interrupt <-chan struct{}) (*cmpb.CpSolverResponse, error) {
-	// Create the atomic bool for interrupting solves.
-	limitReached := newAtomicBoolWrapper()
-	defer limitReached.delete()
+	// Create the environment for interrupting solves.
+	env := newEnvWrapper()
+	defer env.delete()
 
 	// Transform `input` into bytes.
 	bReq, err := proto.Marshal(input)
@@ -100,25 +100,24 @@ func SolveCpModelInterruptibleWithParameters(input *cmpb.CpModelProto, params *s
 	go func() {
 		select {
 		case <-interrupt:
-			limitReached.trigger()
+			env.stopSolve()
 		case <-solveDone:
 		}
 	}()
 
-	// We want to make sure we trigger the atomic Bool before we call the solver
-	// if the input `interrupt` is already closed. We can't trust the
+	// We want to make sure we stop the search before we call the solver. We can't trust the
 	// scheduler to execute the previous goroutine immediately, even calling
 	// `runtime.Gosched()` (the unit test failed 3 out of 1000 times when doing
 	// so).
 	select {
 	case <-interrupt:
-		limitReached.trigger()
+		env.stopSolve()
 	default:
 	}
 
 	var cRes unsafe.Pointer
 	var cResLen C.int
-	C.SolveCpInterruptible(limitReached.ptr, cReq, C.int(len(bReq)), cParams, C.int(len(bParams)), &cRes, &cResLen)
+	C.SolveCpInterruptible(env.ptr, cReq, C.int(len(bReq)), cParams, C.int(len(bParams)), &cRes, &cResLen)
 	defer C.free(cRes)
 
 	// Transform `cRes` into the Go response proto.
@@ -131,29 +130,29 @@ func SolveCpModelInterruptibleWithParameters(input *cmpb.CpModelProto, params *s
 	return result, nil
 }
 
-// atomicBoolWrapper keeps a pointer on a C++ AtomicBool instance.
-type atomicBoolWrapper struct {
+// envWrapper keeps a pointer on a C++ Model instance.
+type envWrapper struct {
 	mutex sync.Mutex
 	ptr   unsafe.Pointer // Guarded by mutex.
 }
 
-// newAtomicBoolWrapper returns a new instance of a C++ AtomicBool.
+// newEnvWrapper returns a new instance of a C++ Model.
 //
 // The returned object must be destroyed with delete() for the C++ object not to
 // leak.
 //
-// This object is thread-safe: delete() and trigger() can be called
+// This object is thread-safe: delete() and stopSolve() can be called
 // concurrently.
-func newAtomicBoolWrapper() *atomicBoolWrapper {
-	return &atomicBoolWrapper{
-		ptr: C.SolveCpNewAtomicBool(),
+func newEnvWrapper() *envWrapper {
+	return &envWrapper{
+		ptr: C.SolveCpNewEnv(),
 	}
 }
 
-// trigger triggers the C++ SolveCpStopSolve method with the atomic bool.
+// stopSolve triggers the C++ SolveCpStopSolve method with the environment.
 //
-// If the atomic bool has been deleted this has no effect.
-func (intr *atomicBoolWrapper) trigger() {
+// If the environment has been deleted this has no effect.
+func (intr *envWrapper) stopSolve() {
 	intr.mutex.Lock()
 	defer intr.mutex.Unlock()
 	if uintptr(intr.ptr) != 0 {
@@ -164,12 +163,12 @@ func (intr *atomicBoolWrapper) trigger() {
 // delete deletes the underlying C++ object.
 //
 // Calling it multiple times has not effect.
-func (intr *atomicBoolWrapper) delete() {
+func (intr *envWrapper) delete() {
 	intr.mutex.Lock()
 	defer intr.mutex.Unlock()
 	// We don't test that intr.ptr is not nullptr here since C++ `delete` can be
 	// called with nullptr.
-	C.SolveCpDestroyAtomicBool(intr.ptr)
+	C.SolveCpDestroyEnv(intr.ptr)
 	intr.ptr = unsafe.Pointer(uintptr(0))
 }
 

ortools/sat/presolve_context.cc

@@ -2017,22 +2017,15 @@ bool PresolveContext::CanonicalizeObjective(bool simplify_domain) {
     objective_offset_ /= static_cast<double>(gcd);
     objective_scaling_factor_ *= static_cast<double>(gcd);
 
-    // We update the offset accordingly.
-    absl::int128 offset = absl::int128(objective_integer_before_offset_) *
-                              absl::int128(objective_integer_scaling_factor_) +
-                          absl::int128(objective_integer_after_offset_);
-
-    if (objective_domain_.IsFixed()) {
-      // To avoid overflow in (fixed_value * gcd + before_offset) * factor +
-      // after_offset because the objective is constant (and should fit on an
-      // int64_t), we can rewrite it as fixed_value + offset.
-      objective_integer_scaling_factor_ = 1;
-      offset +=
-          absl::int128(gcd - 1) * absl::int128(objective_domain_.FixedValue());
-    } else {
-      objective_integer_scaling_factor_ *= gcd;
-    }
-
+    // We update the integer offsets accordingly.
+    //
+    // We compute the old "a * objective_scaling_factor_ + b" offset and rewrite
+    // it in term of the new "objective_scaling_factor_".
+    const absl::int128 offset =
+        absl::int128(objective_integer_before_offset_) *
+            absl::int128(objective_integer_scaling_factor_) +
+        absl::int128(objective_integer_after_offset_);
+    objective_integer_scaling_factor_ *= gcd;
     objective_integer_before_offset_ = static_cast<int64_t>(
         offset / absl::int128(objective_integer_scaling_factor_));
     objective_integer_after_offset_ = static_cast<int64_t>(

ortools/sat/swig_helper.cc

@@ -26,7 +26,6 @@
 #include "ortools/sat/cp_model_utils.h"
 #include "ortools/sat/model.h"
 #include "ortools/sat/sat_parameters.pb.h"
-#include "ortools/sat/util.h"
 #include "ortools/util/logging.h"
 #include "ortools/util/sorted_interval_list.h"
 
@@ -156,7 +155,7 @@ operations_research::sat::CpSolverResponse SolveWrapper::Solve(
 }
 
 void SolveWrapper::StopSearch() {
-  model_.GetOrCreate<ModelSharedTimeLimit>()->Stop();
+  ::operations_research::sat::StopSearch(&model_);
 }
 
 std::string CpSatHelper::ModelStats(