Skip to content

Commit 0da791c

Browse files
zepfredzepfred
authored
chore: filtering move selector honoring the phase termination (#1608)
This PR ensures that the `FilteringMoveSelector` will honor the phase termination setting. There may be use cases where a move filtering filters out all moves, resulting in the iterator analyzing a large number of moves before returning no valid upcoming selection.
1 parent 2a71f1b commit 0da791c

4 files changed

Lines changed: 93 additions & 30 deletions

File tree

‎core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/move/decorator/FilteringMoveSelector.java‎

Lines changed: 54 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,12 @@
99
import ai.timefold.solver.core.impl.heuristic.selector.move.AbstractMoveSelector;
1010
import ai.timefold.solver.core.impl.heuristic.selector.move.MoveSelector;
1111
import ai.timefold.solver.core.impl.phase.scope.AbstractPhaseScope;
12+
import ai.timefold.solver.core.impl.solver.termination.PhaseTermination;
1213

1314
public final class FilteringMoveSelector<Solution_> extends AbstractMoveSelector<Solution_> {
1415

16+
private static final long BAIL_OUT_MULTIPLIER = 10L;
17+
1518
public static <Solution_> FilteringMoveSelector<Solution_> of(MoveSelector<Solution_> moveSelector,
1619
SelectionFilter<Solution_, Move<Solution_>> filter) {
1720
if (moveSelector instanceof FilteringMoveSelector<Solution_> filteringMoveSelector) {
@@ -24,6 +27,7 @@ public static <Solution_> FilteringMoveSelector<Solution_> of(MoveSelector<Solut
2427
private final MoveSelector<Solution_> childMoveSelector;
2528
private final SelectionFilter<Solution_, Move<Solution_>> filter;
2629
private final boolean bailOutEnabled;
30+
private AbstractPhaseScope<Solution_> phaseScope;
2731

2832
private ScoreDirector<Solution_> scoreDirector = null;
2933

@@ -42,13 +46,15 @@ private FilteringMoveSelector(MoveSelector<Solution_> childMoveSelector,
4246
@Override
4347
public void phaseStarted(AbstractPhaseScope<Solution_> phaseScope) {
4448
super.phaseStarted(phaseScope);
45-
scoreDirector = phaseScope.getScoreDirector();
49+
this.scoreDirector = phaseScope.getScoreDirector();
50+
this.phaseScope = phaseScope;
4651
}
4752

4853
@Override
4954
public void phaseEnded(AbstractPhaseScope<Solution_> phaseScope) {
5055
super.phaseEnded(phaseScope);
51-
scoreDirector = null;
56+
this.scoreDirector = null;
57+
this.phaseScope = null;
5258
}
5359

5460
@Override
@@ -68,23 +74,53 @@ public long getSize() {
6874

6975
@Override
7076
public Iterator<Move<Solution_>> iterator() {
71-
return new JustInTimeFilteringMoveIterator(childMoveSelector.iterator(), determineBailOutSize());
77+
return new JustInTimeFilteringMoveIterator(childMoveSelector.iterator(), determineBailOutSize(), phaseScope);
78+
}
79+
80+
private long determineBailOutSize() {
81+
if (!bailOutEnabled) {
82+
return -1L;
83+
}
84+
try {
85+
return childMoveSelector.getSize() * BAIL_OUT_MULTIPLIER;
86+
} catch (Exception ex) {
87+
// Some move selectors throw an exception when getSize() is called.
88+
// In this case, we choose to disregard it and pick a large-enough bail-out size anyway.
89+
// The ${bailOutSize+1}th move could in theory show up where previous ${bailOutSize} moves did not,
90+
// but we consider this to be an acceptable risk,
91+
// outweighed by the benefit of the solver never running into an endless loop.
92+
// The exception itself is swallowed, as it doesn't bring any useful information.
93+
long bailOutSize = Short.MAX_VALUE * BAIL_OUT_MULTIPLIER;
94+
logger.trace(
95+
" Never-ending move selector ({}) failed to provide size, choosing a bail-out size of ({}) attempts.",
96+
childMoveSelector, bailOutSize);
97+
return bailOutSize;
98+
}
7299
}
73100

74101
private class JustInTimeFilteringMoveIterator extends UpcomingSelectionIterator<Move<Solution_>> {
75102

103+
private final long TERMINATION_BAIL_OUT_SIZE = 1000L;
76104
private final Iterator<Move<Solution_>> childMoveIterator;
77105
private final long bailOutSize;
106+
private final AbstractPhaseScope<Solution_> phaseScope;
107+
private final PhaseTermination<Solution_> termination;
78108

79-
public JustInTimeFilteringMoveIterator(Iterator<Move<Solution_>> childMoveIterator, long bailOutSize) {
109+
public JustInTimeFilteringMoveIterator(Iterator<Move<Solution_>> childMoveIterator, long bailOutSize,
110+
AbstractPhaseScope<Solution_> phaseScope) {
80111
this.childMoveIterator = childMoveIterator;
81112
this.bailOutSize = bailOutSize;
113+
this.phaseScope = phaseScope;
114+
this.termination = phaseScope != null ? phaseScope.getTermination() : null;
82115
}
83116

84117
@Override
85118
protected Move<Solution_> createUpcomingSelection() {
86119
Move<Solution_> next;
87120
long attemptsBeforeBailOut = bailOutSize;
121+
// To reduce the impact of checking for termination on each move,
122+
// we only check for termination after filtering out 1000 moves.
123+
long attemptsBeforeCheckTermination = TERMINATION_BAIL_OUT_SIZE;
88124
do {
89125
if (!childMoveIterator.hasNext()) {
90126
return noUpcomingSelection();
@@ -95,8 +131,18 @@ protected Move<Solution_> createUpcomingSelection() {
95131
logger.trace("Bailing out of neverEnding selector ({}) after ({}) attempts to avoid infinite loop.",
96132
FilteringMoveSelector.this, bailOutSize);
97133
return noUpcomingSelection();
134+
} else if (termination != null && attemptsBeforeCheckTermination <= 0L) {
135+
// Reset the counter
136+
attemptsBeforeCheckTermination = TERMINATION_BAIL_OUT_SIZE;
137+
if (termination.isPhaseTerminated(phaseScope)) {
138+
logger.trace(
139+
"Bailing out of neverEnding selector ({}) because the termination setting has been triggered.",
140+
FilteringMoveSelector.this);
141+
return noUpcomingSelection();
142+
}
98143
}
99144
attemptsBeforeBailOut--;
145+
attemptsBeforeCheckTermination--;
100146
}
101147
next = childMoveIterator.next();
102148
} while (!accept(scoreDirector, next));
@@ -105,34 +151,12 @@ protected Move<Solution_> createUpcomingSelection() {
105151

106152
}
107153

108-
private long determineBailOutSize() {
109-
if (!bailOutEnabled) {
110-
return -1L;
111-
}
112-
try {
113-
return childMoveSelector.getSize() * 10L;
114-
} catch (Exception ex) {
115-
// Some move selectors throw an exception when getSize() is called.
116-
// In this case, we choose to disregard it and pick a large-enough bail-out size anyway.
117-
// The ${bailOutSize+1}th move could in theory show up where previous ${bailOutSize} moves did not,
118-
// but we consider this to be an acceptable risk,
119-
// outweighed by the benefit of the solver never running into an endless loop.
120-
// The exception itself is swallowed, as it doesn't bring any useful information.
121-
long bailOutSize = Short.MAX_VALUE * 10L;
122-
logger.trace(
123-
" Never-ending move selector ({}) failed to provide size, choosing a bail-out size of ({}) attempts.",
124-
childMoveSelector, bailOutSize);
125-
return bailOutSize;
126-
}
127-
}
128-
129154
private boolean accept(ScoreDirector<Solution_> scoreDirector, Move<Solution_> move) {
130-
if (filter != null) {
131-
if (!filter.accept(scoreDirector, move)) {
132-
logger.trace(" Move ({}) filtered out by a selection filter ({}).", move, filter);
133-
return false;
134-
}
155+
if (filter != null && !filter.accept(scoreDirector, move)) {
156+
logger.trace(" Move ({}) filtered out by a selection filter ({}).", move, filter);
157+
return false;
135158
}
159+
136160
return true;
137161
}
138162

‎core/src/main/java/ai/timefold/solver/core/impl/phase/AbstractPhase.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,7 @@ public void phaseStarted(AbstractPhaseScope<Solution_> phaseScope) {
9595
solver.phaseStarted(phaseScope);
9696
}
9797
phaseTermination.phaseStarted(phaseScope);
98+
phaseScope.setTermination(phaseTermination);
9899
phaseLifecycleSupport.firePhaseStarted(phaseScope);
99100
}
100101

‎core/src/main/java/ai/timefold/solver/core/impl/phase/scope/AbstractPhaseScope.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import ai.timefold.solver.core.impl.score.director.InnerScore;
1010
import ai.timefold.solver.core.impl.score.director.InnerScoreDirector;
1111
import ai.timefold.solver.core.impl.solver.scope.SolverScope;
12+
import ai.timefold.solver.core.impl.solver.termination.PhaseTermination;
1213
import ai.timefold.solver.core.preview.api.move.Move;
1314

1415
import org.slf4j.Logger;
@@ -36,6 +37,11 @@ public abstract class AbstractPhaseScope<Solution_> {
3637

3738
protected int bestSolutionStepIndex;
3839

40+
/**
41+
* The phase termination configuration
42+
*/
43+
private PhaseTermination<Solution_> termination;
44+
3945
/**
4046
* As defined by #AbstractPhaseScope(SolverScope, int, boolean)
4147
* with the phaseSendingBestSolutionEvents parameter set to true.
@@ -188,6 +194,14 @@ public <Score_ extends Score<Score_>> InnerScoreDirector<Solution_, Score_> getS
188194
return solverScope.getScoreDirector();
189195
}
190196

197+
public void setTermination(PhaseTermination<Solution_> termination) {
198+
this.termination = termination;
199+
}
200+
201+
public PhaseTermination<Solution_> getTermination() {
202+
return termination;
203+
}
204+
191205
public Solution_ getWorkingSolution() {
192206
return solverScope.getWorkingSolution();
193207
}

‎core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/move/decorator/FilteringMoveSelectorTest.java‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,15 @@
22

33
import static ai.timefold.solver.core.testutil.PlannerAssert.assertAllCodesOfMoveSelector;
44
import static ai.timefold.solver.core.testutil.PlannerAssert.verifyPhaseLifecycle;
5+
import static org.assertj.core.api.Assertions.assertThat;
6+
import static org.mockito.ArgumentMatchers.any;
57
import static org.mockito.Mockito.mock;
68
import static org.mockito.Mockito.times;
79
import static org.mockito.Mockito.verify;
810
import static org.mockito.Mockito.when;
911

12+
import java.util.Iterator;
13+
1014
import ai.timefold.solver.core.config.heuristic.selector.common.SelectionCacheType;
1115
import ai.timefold.solver.core.impl.heuristic.move.DummyMove;
1216
import ai.timefold.solver.core.impl.heuristic.selector.SelectorTestUtils;
@@ -15,6 +19,7 @@
1519
import ai.timefold.solver.core.impl.phase.scope.AbstractPhaseScope;
1620
import ai.timefold.solver.core.impl.phase.scope.AbstractStepScope;
1721
import ai.timefold.solver.core.impl.solver.scope.SolverScope;
22+
import ai.timefold.solver.core.impl.solver.termination.BasicPlumbingTermination;
1823
import ai.timefold.solver.core.testdomain.TestdataSolution;
1924

2025
import org.junit.jupiter.api.Test;
@@ -41,6 +46,25 @@ void filterCacheTypeJustInTime() {
4146
filter(SelectionCacheType.JUST_IN_TIME, 5);
4247
}
4348

49+
@Test
50+
void bailOutByTermination() {
51+
var phaseScope = mock(AbstractPhaseScope.class);
52+
var moveSelector = mock(MoveSelector.class);
53+
var termination = mock(BasicPlumbingTermination.class);
54+
var iterator = mock(Iterator.class);
55+
when(moveSelector.getSize()).thenReturn(1000L);
56+
when(moveSelector.isNeverEnding()).thenReturn(true);
57+
when(moveSelector.iterator()).thenReturn(iterator);
58+
when(iterator.hasNext()).thenReturn(true);
59+
when(phaseScope.getTermination()).thenReturn(termination);
60+
when(termination.isPhaseTerminated(any(AbstractPhaseScope.class))).thenReturn(false, true);
61+
var filteredMoveSelector = FilteringMoveSelector.of(moveSelector, (scoreDirector, selection) -> false);
62+
filteredMoveSelector.phaseStarted(phaseScope);
63+
assertThat(filteredMoveSelector.iterator().hasNext()).isFalse();
64+
// The termination returns true at the second call, and 2000 calls are executed in total
65+
verify(iterator, times(2000)).next();
66+
}
67+
4468
public void filter(SelectionCacheType cacheType, int timesCalled) {
4569
MoveSelector childMoveSelector = SelectorTestUtils.mockMoveSelector(
4670
new DummyMove("a1"), new DummyMove("a2"), new DummyMove("a3"), new DummyMove("a4"));

0 commit comments

Comments
 (0)