perf: Support reusing the best solution instead of cloning in LS by Christopher-Chianelli · Pull Request #2332 · TimefoldAI/timefold-solver

Christopher-Chianelli · 2026-06-01T19:20:56Z

Currently the working solution is cloned when ever a new best solution is found. This is slow and create additional memory pressure on giant datasets, since each entity must be cloned.

This adds a new SolverConfig property, reuseBestSolution, which will reuse the initial best solution clone in local search. When enabled, local search keep track of moves used in each step, which are then used to update the best solution instance.

A read-write lock is used to allow reading the best solution safely outside the solver thread. This read-write lock is automatically used for SolverManager events.

Since the same instance is reused, when this feature is enabled, users should ensure the solution and entities are not saved outside the event handler.

Currently the working solution is cloned when ever a new best solution is found. This is slow and create additional memory pressure on giant datasets, since each entity must be cloned. This adds a new SolverConfig property, `reuseBestSolution`, which will reuse the initial best solution clone in local search. When enabled, local search keep track of moves used in each step, which are then used to update the best solution instance. A read-write lock is used to allow reading the best solution safely outside the solver thread. This read-write lock is automatically used for SolverManager events. Since the same instance is reused, when this feature is enabled, users should ensure the solution and entities are not saved outside the event handler.

sonarqubecloud · 2026-06-01T19:41:57Z

Quality Gate failed

Failed conditions
66.9% Coverage on New Code (required ≥ 70%)

See analysis details on SonarQube Cloud

zepfred

Reusing the best solution is a great idea! I have some questions and suggestions about not changing the existing contract to achieve the goal of this PR.

zepfred · 2026-06-02T13:30:47Z


    public void fireBestSolutionChanged(SolverScope<Solution_> solverScope, EventProducerId eventProducerId,
-            Solution_ newBestSolution) {
+            Solution_ newBestSolution, Lock readLock) {


Why are we including the lock as a parameter? Considering the principle of separation of concerns, wouldn't it be better for the lock to be part of the LockableSolverEventListener instead?

This fires events for all listeners. SolverManager fire off its events in a seperate thread, which makes a lock neccessary for it but not other event listeners. This is not a part of the API; it just propagate events.

zepfred · 2026-06-02T13:31:34Z

                    new DefaultBestSolutionChangedEvent<>(solver, eventProducerId, timeMillisSpent, newBestSolution, bestScore);
            do {
-                it.next().bestSolutionChanged(event);
+                var eventListener = it.next();


If the listener contract remains unchanged, there will be no need to update the block.

LockableSolverEventListener is a SolverEventListener; SolverEventListener is not a LockableSolverEventListener. The block changed so a lock can be passed to LockableSolverEventListener without adapting all SolverEventListener.

zepfred · 2026-06-02T13:35:46Z

        "terminationConfig",
        "nearbyDistanceMeterClass",
        "phaseConfigList",
+        "reuseBestSolution",


I'm not sure if we need a new property. Do we always want to use the new related logic? Are there scenarios where enabling this feature by default may not be recommended?

+1. Already discussed on Slack with Chris; no external configuration, we do the right thing.

The primary reason for the switch is that this reuses the existing solution; users might rely on getting new best solution instances if they save them outside the event handler (ex: store them in a MutableReference).

zepfred · 2026-06-02T13:42:20Z

+
+import org.jspecify.annotations.NonNull;
+
+public interface LockableSolverEventListener<Solution_> extends SolverEventListener<Solution_> {


I wonder if we really need a new contract. Can we provide a new event listener implementation that incorporates the required logic with locking without changing the contract?

zepfred · 2026-06-02T13:44:16Z

+    public void setReuseBestSolution(boolean reuseBestSolution) {
+        this.reuseBestSolution = reuseBestSolution;
+        if (reuseBestSolution) {
+            reusingBestSolutionUpdater = TimefoldSolverEnterpriseService


Does this mean that if a community user sets reuseBestSolution to true, the solving process will fail?

zepfred · 2026-06-02T13:53:21Z

            bestSolutionRecaller.setAssertBestScoreIsUnmodified(true);
        }
+        if (reuseBestSolution != null && reuseBestSolution) {
+            bestSolutionRecaller.setReuseBestSolution(true);


Suggested change

bestSolutionRecaller.setReuseBestSolution(true);

bestSolutionRecaller.setReuseBestSolution(reuseBestSolution != null && reuseBestSolution);

zepfred · 2026-06-02T13:53:34Z

            bestSolutionRecaller.setAssertShadowVariablesAreNotStale(true);
            bestSolutionRecaller.setAssertBestScoreIsUnmodified(true);
        }
+        if (reuseBestSolution != null && reuseBestSolution) {


Suggested change

if (reuseBestSolution != null && reuseBestSolution) {

zepfred · 2026-06-02T13:53:45Z

        }
+        if (reuseBestSolution != null && reuseBestSolution) {
+            bestSolutionRecaller.setReuseBestSolution(true);
+        }


Suggested change

}

zepfred · 2026-06-02T13:55:10Z

    }

-    public <Solution_> BestSolutionRecaller<Solution_> buildBestSolutionRecaller(EnvironmentMode environmentMode) {
+    public <Solution_> BestSolutionRecaller<Solution_> buildBestSolutionRecaller(EnvironmentMode environmentMode,


Please refer to my comment on BestSolutionRecaller. It appears we could pass the lock instance as a parameter here.

zepfred · 2026-06-02T14:00:28Z

    }

-    private CompletableFuture<Void> scheduleIntermediateBestSolutionConsumption() {
+    private CompletableFuture<Void> scheduleIntermediateBestSolutionConsumption(Lock lock) {


I believe here is another place where we should consider keeping the existing contract unchanged. Let's say we can create LockableConsumerSupport that receives or creates its internal lock instance. We could extend ConsumerSupport and override scheduleIntermediateBestSolutionConsumption without changing the existing contract. Does it make sense?

triceo

Folks, we will not be merging this now.

I am not convinced that the problem we're solving warrants this level of complexity. Specifically:

Growing list of changes that need to be applied, effectively a controlled memory leak.
Heuristics for the solution sizes where this should kick in, and heuristics for the list size where this should be disabled.
External configuration for something so low-level, because we need to be able to deal with a situation that the user modifies the best solution while we still need to apply moves on it.
Lockable event listeners?!

This shows me that the approach selected here is not the right one. Considering that we currently do not feel this pain, and that we have other priorities to tackle, I think we should not be spending more time right now to get this right.

triceo · 2026-07-29T05:51:23Z

I was thinking about this more, and I really think that this should not be merged. My previous objections still stand, but I want to add another one - even if the user is aware of the fact that the best solutions will be updated, there is still no sane way to work with best solution events.

If two events can share the same solution, that means the user possibly has two events which influence each other; this behavior is impossible to deal with. If the new event already exists, the old event has the new event's data; this tells me that if we want to keep this mechanism instead of cloning, we need to refactor how best solutions are consumed.

I consider this mechanism fundamentally and unfixably incompatible with best solution events.

Christopher-Chianelli · 2026-07-29T06:19:33Z

I was thinking about this more, and I really think that this should not be merged. My previous objections still stand, but I want to add another one - even if the user is aware of the fact that the best solutions will be updated, there is still no sane way to work with best solution events.

If two events can share the same solution, that means the user possibly has two events which influence each other; this behavior is impossible to deal with. If the new event already exists, the old event has the new event's data; this tells me that if we want to keep this mechanism instead of cloning, we need to refactor how best solutions are consumed.

I consider this mechanism fundamentally and unfixably incompatible with best solution events.

Hence why event listeners are lockable; via SolverFactory, nothing changes since event listeners are synchronous (and if they will spawn a new thread, they can implement LockableEventListener instead). Via SolverManager, the event listener threads locks the best solution so the Solver cannot update it (and thus cannot fire additional events) until the event listener finishes.

triceo · 2026-07-29T06:36:54Z

Hence why event listeners are lockable; via SolverFactory, nothing changes since event listeners are synchronous (and if they will spawn a new thread, they can implement LockableEventListener instead). Via SolverManager, the event listener threads locks the best solution so the Solver cannot update it (and thus cannot fire additional events) until the event listener finishes.

But all of this is wrong! With SolverFactory, the event listeners are synchronously triggered, but who knows how they're processed by the user; my objection still stands.

The approach is more obviously wrong on the SolverManager side. Once the new best solution is available, arguably the old one becomes garbage; at the very least, it becomes much less useful. Blocking the new, valuable solutions so that the stale one can finish being processed is counterproductive.

Christopher-Chianelli requested review from triceo and zepfred as code owners June 1, 2026 19:20

Christopher-Chianelli temporarily deployed to internal June 1, 2026 19:21 — with GitHub Actions Inactive

zepfred requested changes Jun 2, 2026

View reviewed changes

triceo requested changes Jun 2, 2026

View reviewed changes

triceo marked this pull request as draft June 3, 2026 06:27


		import org.jspecify.annotations.NonNull;

		public interface LockableSolverEventListener<Solution_> extends SolverEventListener<Solution_> {

	bestSolutionRecaller.setReuseBestSolution(true);
	bestSolutionRecaller.setReuseBestSolution(reuseBestSolution != null && reuseBestSolution);

Uh oh!

Conversation

Christopher-Chianelli commented Jun 1, 2026

Uh oh!

sonarqubecloud Bot commented Jun 1, 2026

Quality Gate failed

Uh oh!

zepfred left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

triceo left a comment

Choose a reason for hiding this comment

Uh oh!

triceo commented Jul 29, 2026

Uh oh!

Christopher-Chianelli commented Jul 29, 2026

Uh oh!

triceo commented Jul 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants