fix: improve strategy

Author: zepfred

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

@@ -13,6 +13,8 @@
 
 public final class FilteringMoveSelector<Solution_> extends AbstractMoveSelector<Solution_> {
 
+    private static final long BAIL_OUT_MULTIPLIER = 10L;
+
     public static <Solution_> FilteringMoveSelector<Solution_> of(MoveSelector<Solution_> moveSelector,
             SelectionFilter<Solution_, Move<Solution_>> filter) {
         if (moveSelector instanceof FilteringMoveSelector<Solution_> filteringMoveSelector) {
@@ -75,6 +77,27 @@ public Iterator<Move<Solution_>> iterator() {
         return new JustInTimeFilteringMoveIterator(childMoveSelector.iterator(), determineBailOutSize(), phaseScope);
     }
 
+    private long determineBailOutSize() {
+        if (!bailOutEnabled) {
+            return -1L;
+        }
+        try {
+            return childMoveSelector.getSize() * BAIL_OUT_MULTIPLIER;
+        } catch (Exception ex) {
+            // Some move selectors throw an exception when getSize() is called.
+            // In this case, we choose to disregard it and pick a large-enough bail-out size anyway.
+            // The ${bailOutSize+1}th move could in theory show up where previous ${bailOutSize} moves did not,
+            // but we consider this to be an acceptable risk,
+            // outweighed by the benefit of the solver never running into an endless loop.
+            // The exception itself is swallowed, as it doesn't bring any useful information.
+            long bailOutSize = Short.MAX_VALUE * BAIL_OUT_MULTIPLIER;
+            logger.trace(
+                    "        Never-ending move selector ({}) failed to provide size, choosing a bail-out size of ({}) attempts.",
+                    childMoveSelector, bailOutSize);
+            return bailOutSize;
+        }
+    }
+
     private class JustInTimeFilteringMoveIterator extends UpcomingSelectionIterator<Move<Solution_>> {
 
         private final Iterator<Move<Solution_>> childMoveIterator;
@@ -94,6 +117,10 @@ public JustInTimeFilteringMoveIterator(Iterator<Move<Solution_>> childMoveIterat
         protected Move<Solution_> createUpcomingSelection() {
             Move<Solution_> next;
             long attemptsBeforeBailOut = bailOutSize;
+            // To reduce the impact of checking for termination on each move,
+            // we only check for termination
+            // after filtering out a total number of moves equal to size(childMoveSelector).
+            long attemptsBeforeCheckTermination = bailOutSize / BAIL_OUT_MULTIPLIER;
             do {
                 if (!childMoveIterator.hasNext()) {
                     return noUpcomingSelection();
@@ -104,13 +131,18 @@ protected Move<Solution_> createUpcomingSelection() {
                         logger.trace("Bailing out of neverEnding selector ({}) after ({}) attempts to avoid infinite loop.",
                                 FilteringMoveSelector.this, bailOutSize);
                         return noUpcomingSelection();
-                    } else if (termination != null && termination.isPhaseTerminated(phaseScope)) {
-                        logger.trace(
-                                "Bailing out of neverEnding selector ({}) because the termination setting has been triggered.",
-                                FilteringMoveSelector.this);
-                        return noUpcomingSelection();
+                    } else if (termination != null && attemptsBeforeCheckTermination <= 0L) {
+                        // Reset the counter
+                        attemptsBeforeCheckTermination = bailOutSize / BAIL_OUT_MULTIPLIER;
+                        if (termination.isPhaseTerminated(phaseScope)) {
+                            logger.trace(
+                                    "Bailing out of neverEnding selector ({}) because the termination setting has been triggered.",
+                                    FilteringMoveSelector.this);
+                            return noUpcomingSelection();
+                        }
                     }
                     attemptsBeforeBailOut--;
+                    attemptsBeforeCheckTermination--;
                 }
                 next = childMoveIterator.next();
             } while (!accept(scoreDirector, next));
@@ -119,27 +151,6 @@ protected Move<Solution_> createUpcomingSelection() {
 
     }
 
-    private long determineBailOutSize() {
-        if (!bailOutEnabled) {
-            return -1L;
-        }
-        try {
-            return childMoveSelector.getSize() * 10L;
-        } catch (Exception ex) {
-            // Some move selectors throw an exception when getSize() is called.
-            // In this case, we choose to disregard it and pick a large-enough bail-out size anyway.
-            // The ${bailOutSize+1}th move could in theory show up where previous ${bailOutSize} moves did not,
-            // but we consider this to be an acceptable risk,
-            // outweighed by the benefit of the solver never running into an endless loop.
-            // The exception itself is swallowed, as it doesn't bring any useful information.
-            long bailOutSize = Short.MAX_VALUE * 10L;
-            logger.trace(
-                    "        Never-ending move selector ({}) failed to provide size, choosing a bail-out size of ({}) attempts.",
-                    childMoveSelector, bailOutSize);
-            return bailOutSize;
-        }
-    }
-
     private boolean accept(ScoreDirector<Solution_> scoreDirector, Move<Solution_> move) {
         if (filter != null && !filter.accept(scoreDirector, move)) {
             logger.trace("        Move ({}) filtered out by a selection filter ({}).", move, filter);

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

@@ -52,18 +52,17 @@ void bailOutByTermination() {
         var moveSelector = mock(MoveSelector.class);
         var termination = mock(BasicPlumbingTermination.class);
         var iterator = mock(Iterator.class);
-        // We set the maximum value to force it to run many evaluations
-        when(moveSelector.getSize()).thenReturn(Long.MAX_VALUE / 11);
+        when(moveSelector.getSize()).thenReturn(100L);
         when(moveSelector.isNeverEnding()).thenReturn(true);
         when(moveSelector.iterator()).thenReturn(iterator);
         when(iterator.hasNext()).thenReturn(true);
         when(phaseScope.getTermination()).thenReturn(termination);
-        when(termination.isPhaseTerminated(any(AbstractPhaseScope.class))).thenReturn(false, false, true);
+        when(termination.isPhaseTerminated(any(AbstractPhaseScope.class))).thenReturn(false, true);
         var filteredMoveSelector = FilteringMoveSelector.of(moveSelector, (scoreDirector, selection) -> false);
         filteredMoveSelector.phaseStarted(phaseScope);
         assertThat(filteredMoveSelector.iterator().hasNext()).isFalse();
-        // The termination returns true at the third call
-        verify(iterator, times(2)).next();
+        // The termination returns true at the second call, and only (100 * 10) * 30% calls are executed per check
+        verify(iterator, times(200)).next();
     }
 
     public void filter(SelectionCacheType cacheType, int timesCalled) {