Skip to content

perf: Support reusing the best solution instead of cloning in LS#2332

Draft
Christopher-Chianelli wants to merge 1 commit into
TimefoldAI:mainfrom
Christopher-Chianelli:perf/incremental-cloning
Draft

perf: Support reusing the best solution instead of cloning in LS#2332
Christopher-Chianelli wants to merge 1 commit into
TimefoldAI:mainfrom
Christopher-Chianelli:perf/incremental-cloning

Conversation

@Christopher-Chianelli
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown

sonarqubecloud Bot commented Jun 1, 2026

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarQube Cloud

Copy link
Copy Markdown
Contributor

@zepfred zepfred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


public void fireBestSolutionChanged(SolverScope<Solution_> solverScope, EventProducerId eventProducerId,
Solution_ newBestSolution) {
Solution_ newBestSolution, Lock readLock) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

new DefaultBestSolutionChangedEvent<>(solver, eventProducerId, timeMillisSpent, newBestSolution, bestScore);
do {
it.next().bestSolutionChanged(event);
var eventListener = it.next();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"terminationConfig",
"nearbyDistanceMeterClass",
"phaseConfigList",
"reuseBestSolution",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


import org.jspecify.annotations.NonNull;

public interface LockableSolverEventListener<Solution_> extends SolverEventListener<Solution_> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

public void setReuseBestSolution(boolean reuseBestSolution) {
this.reuseBestSolution = reuseBestSolution;
if (reuseBestSolution) {
reusingBestSolutionUpdater = TimefoldSolverEnterpriseService
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

bestSolutionRecaller.setAssertBestScoreIsUnmodified(true);
}
if (reuseBestSolution != null && reuseBestSolution) {
bestSolutionRecaller.setReuseBestSolution(true);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

bestSolutionRecaller.setAssertShadowVariablesAreNotStale(true);
bestSolutionRecaller.setAssertBestScoreIsUnmodified(true);
}
if (reuseBestSolution != null && reuseBestSolution) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (reuseBestSolution != null && reuseBestSolution) {

}
if (reuseBestSolution != null && reuseBestSolution) {
bestSolutionRecaller.setReuseBestSolution(true);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}

}

public <Solution_> BestSolutionRecaller<Solution_> buildBestSolutionRecaller(EnvironmentMode environmentMode) {
public <Solution_> BestSolutionRecaller<Solution_> buildBestSolutionRecaller(EnvironmentMode environmentMode,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

private CompletableFuture<Void> scheduleIntermediateBestSolutionConsumption() {
private CompletableFuture<Void> scheduleIntermediateBestSolutionConsumption(Lock lock) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Collaborator

@triceo triceo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 triceo marked this pull request as draft June 3, 2026 06:27
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