fix: process each entity only once and in topological order (#1662)

Previously, the code relied on eventually consistency to avoid
collections (that is, sometimes suppliers will be called with stale
data, but will always eventually be called with correct data).
Now, an initial sort is done to ensure no data is stale, and a map and
comparator are utilized to prevent unnecessary computations.

---------

Co-authored-by: Lukáš Petrovický <[email protected]>

Author: Christopher-Chianelli

core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/AbstractVariableReferenceGraph.java

@@ -18,6 +18,7 @@
 
 public abstract sealed class AbstractVariableReferenceGraph<Solution_, ChangeSet_> implements VariableReferenceGraph
         permits DefaultVariableReferenceGraph, FixedVariableReferenceGraph {
+
     // These structures are immutable.
     protected final List<EntityVariablePair<Solution_>> instanceList;
     protected final Map<VariableMetaModel<?, ?, ?>, Map<Object, EntityVariablePair<Solution_>>> variableReferenceToInstanceMap;

core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/DefaultShadowVariableSessionFactory.java

@@ -14,17 +14,14 @@
 import java.util.Objects;
 import java.util.Set;
 import java.util.function.IntFunction;
-import java.util.function.UnaryOperator;
 
 import ai.timefold.solver.core.impl.domain.solution.descriptor.SolutionDescriptor;
 import ai.timefold.solver.core.impl.domain.variable.descriptor.VariableDescriptor;
-import ai.timefold.solver.core.impl.domain.variable.inverserelation.InverseRelationShadowVariableDescriptor;
 import ai.timefold.solver.core.impl.score.director.InnerScoreDirector;
 import ai.timefold.solver.core.preview.api.domain.metamodel.VariableMetaModel;
 
 import org.jspecify.annotations.NonNull;
 import org.jspecify.annotations.NullMarked;
-import org.jspecify.annotations.Nullable;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -44,7 +41,6 @@ public DefaultShadowVariableSessionFactory(
         this.graphCreator = graphCreator;
     }
 
-    @SuppressWarnings("unchecked")
     public static <Solution_> VariableReferenceGraph buildGraph(
             SolutionDescriptor<Solution_> solutionDescriptor,
             VariableReferenceGraphBuilder<Solution_> variableReferenceGraphBuilder, Object[] entities,
@@ -68,21 +64,21 @@ yield buildSingleDirectionalParentGraph(solutionDescriptor,
         };
     }
 
-    private static <Solution_> VariableReferenceGraph buildSingleDirectionalParentGraph(
+    static <Solution_> VariableReferenceGraph buildSingleDirectionalParentGraph(
             SolutionDescriptor<Solution_> solutionDescriptor,
             ChangedVariableNotifier<Solution_> changedVariableNotifier,
             GraphStructure.GraphStructureAndDirection graphStructureAndDirection,
             Object[] entities) {
         var declarativeShadowVariables = solutionDescriptor.getDeclarativeShadowVariableDescriptors();
         var sortedDeclarativeVariables = topologicallySortedDeclarativeShadowVariables(declarativeShadowVariables);
 
-        var successorFunction =
-                getSuccessorFunction(solutionDescriptor, Objects.requireNonNull(changedVariableNotifier.innerScoreDirector()),
-                        Objects.requireNonNull(graphStructureAndDirection.parentMetaModel()),
+        var topologicalSorter =
+                getTopologicalSorter(solutionDescriptor,
+                        Objects.requireNonNull(changedVariableNotifier.innerScoreDirector()),
                         Objects.requireNonNull(graphStructureAndDirection.direction()));
 
-        return new SingleDirectionalParentVariableReferenceGraph<>(sortedDeclarativeVariables, successorFunction,
-                changedVariableNotifier, entities);
+        return new SingleDirectionalParentVariableReferenceGraph<>(sortedDeclarativeVariables,
+                topologicalSorter, changedVariableNotifier, entities);
     }
 
     private static <Solution_> @NonNull List<DeclarativeShadowVariableDescriptor<Solution_>>
@@ -120,21 +116,21 @@ private static <Solution_> VariableReferenceGraph buildSingleDirectionalParentGr
         return sortedDeclarativeVariables;
     }
 
-    private static <Solution_> @NonNull UnaryOperator<@Nullable Object> getSuccessorFunction(
-            SolutionDescriptor<Solution_> solutionDescriptor, InnerScoreDirector<Solution_, ?> scoreDirector,
-            VariableMetaModel<?, ?, ?> parentMetaModel, ParentVariableType parentVariableType) {
+    private static <Solution_> TopologicalSorter getTopologicalSorter(SolutionDescriptor<Solution_> solutionDescriptor,
+            InnerScoreDirector<Solution_, ?> scoreDirector, ParentVariableType parentVariableType) {
         return switch (parentVariableType) {
-            case PREVIOUS ->
-                scoreDirector.getListVariableStateSupply(solutionDescriptor.getListVariableDescriptor())::getNextElement;
-            case NEXT ->
-                scoreDirector.getListVariableStateSupply(solutionDescriptor.getListVariableDescriptor())::getPreviousElement;
-            case CHAINED_NEXT -> {
-                var entityDescriptor = solutionDescriptor.getEntityDescriptorStrict(parentMetaModel.entity().type());
-                var inverseVariable = (InverseRelationShadowVariableDescriptor<?>) entityDescriptor
-                        .getShadowVariableDescriptor(parentMetaModel.name());
-                var sourceVariable = inverseVariable.getSourceVariableDescriptorList().get(0);
-                var entityType = sourceVariable.getEntityDescriptor().getEntityClass();
-                yield old -> entityType.isInstance(old) ? sourceVariable.getValue(old) : null;
+            case PREVIOUS -> {
+                var listStateSupply = scoreDirector.getListVariableStateSupply(solutionDescriptor.getListVariableDescriptor());
+                yield new TopologicalSorter(listStateSupply::getNextElement,
+                        Comparator.comparingInt(entity -> Objects.requireNonNullElse(listStateSupply.getIndex(entity), 0)),
+                        listStateSupply::getInverseSingleton);
+            }
+            case NEXT -> {
+                var listStateSupply = scoreDirector.getListVariableStateSupply(solutionDescriptor.getListVariableDescriptor());
+                yield new TopologicalSorter(listStateSupply::getPreviousElement,
+                        Comparator.comparingInt(entity -> Objects.requireNonNullElse(listStateSupply.getIndex(entity), 0))
+                                .reversed(),
+                        listStateSupply::getInverseSingleton);
             }
             default -> throw new IllegalStateException(
                     "Impossible state: expected parentVariableType to be previous or next but was %s."
@@ -150,7 +146,7 @@ private static <Solution_> VariableReferenceGraph buildArbitraryGraph(
         var variableIdToUpdater = new HashMap<VariableMetaModel<?, ?, ?>, VariableUpdaterInfo<Solution_>>();
 
         // Create graph node for each entity/declarative shadow variable pair.
-        // Maps a variable id to it source aliases;
+        // Maps a variable id to its source aliases;
         // For instance, "previousVisit.startTime" is a source alias of "startTime"
         // One way to view this concept is "previousVisit.startTime" is a pointer
         // to "startTime" of some visit, and thus alias it.

core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/DefaultVariableReferenceGraph.java

@@ -9,8 +9,7 @@
 
 import org.jspecify.annotations.NonNull;
 
-final class DefaultVariableReferenceGraph<Solution_> extends AbstractVariableReferenceGraph<Solution_, BitSet>
-        implements VariableReferenceGraph {
+final class DefaultVariableReferenceGraph<Solution_> extends AbstractVariableReferenceGraph<Solution_, BitSet> {
     // These structures are mutable.
     private final Consumer<BitSet> affectedEntitiesUpdater;
 

core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/FixedVariableReferenceGraph.java

@@ -8,8 +8,7 @@
 import org.jspecify.annotations.NonNull;
 
 public final class FixedVariableReferenceGraph<Solution_>
-        extends AbstractVariableReferenceGraph<Solution_, PriorityQueue<BaseTopologicalOrderGraph.NodeTopologicalOrder>>
-        implements VariableReferenceGraph {
+        extends AbstractVariableReferenceGraph<Solution_, PriorityQueue<BaseTopologicalOrderGraph.NodeTopologicalOrder>> {
     // These are immutable
     private final ChangedVariableNotifier<Solution_> changedVariableNotifier;
 

core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/GraphStructure.java

@@ -80,10 +80,12 @@ public static <Solution_> GraphStructureAndDirection determineGraphStructure(
                     }
                     // The group variable is unused/always empty
                 }
-                case INDIRECT, INVERSE, VARIABLE -> {
+                case INDIRECT, INVERSE, VARIABLE, CHAINED_NEXT -> {
+                    // CHAINED_NEXT has a complex comparator function;
+                    // so use ARBITRARY despite the fact it can be represented using SINGLE_DIRECTIONAL_PARENT
                     return new GraphStructureAndDirection(ARBITRARY, null, null);
                 }
-                case NEXT, PREVIOUS, CHAINED_NEXT -> {
+                case NEXT, PREVIOUS -> {
                     if (parentMetaModel == null) {
                         parentMetaModel = variableSource.variableSourceReferences().get(0).variableMetaModel();
                         directionalType = parentVariableType;

core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/SingleDirectionalParentVariableReferenceGraph.java

@@ -2,7 +2,9 @@
 
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Comparator;
 import java.util.HashSet;
+import java.util.IdentityHashMap;
 import java.util.List;
 import java.util.Objects;
 import java.util.Set;
@@ -11,9 +13,12 @@
 import ai.timefold.solver.core.preview.api.domain.metamodel.VariableMetaModel;
 
 public final class SingleDirectionalParentVariableReferenceGraph<Solution_> implements VariableReferenceGraph {
+
     private final Set<VariableMetaModel<?, ?, ?>> monitoredSourceVariableSet;
     private final VariableUpdaterInfo<Solution_>[] sortedVariableUpdaterInfos;
     private final UnaryOperator<Object> successorFunction;
+    private final Comparator<Object> topologicalOrderComparator;
+    private final UnaryOperator<Object> keyFunction;
     private final ChangedVariableNotifier<Solution_> changedVariableNotifier;
     private final List<Object> changedEntities;
     private final Class<?> monitoredEntityClass;
@@ -22,7 +27,7 @@ public final class SingleDirectionalParentVariableReferenceGraph<Solution_> impl
     @SuppressWarnings("unchecked")
     public SingleDirectionalParentVariableReferenceGraph(
             List<DeclarativeShadowVariableDescriptor<Solution_>> sortedDeclarativeShadowVariableDescriptors,
-            UnaryOperator<Object> successorFunction,
+            TopologicalSorter topologicalSorter,
             ChangedVariableNotifier<Solution_> changedVariableNotifier,
             Object[] entities) {
         monitoredEntityClass = sortedDeclarativeShadowVariableDescriptors.get(0).getEntityDescriptor().getEntityClass();
@@ -31,9 +36,12 @@ public SingleDirectionalParentVariableReferenceGraph(
         changedEntities = new ArrayList<>();
         isUpdating = false;
 
-        this.successorFunction = successorFunction;
+        this.successorFunction = topologicalSorter.successor();
+        this.topologicalOrderComparator = topologicalSorter.comparator();
+        this.keyFunction = topologicalSorter.key();
         this.changedVariableNotifier = changedVariableNotifier;
-        var shadowEntities = Arrays.stream(entities).filter(monitoredEntityClass::isInstance).toArray();
+        var shadowEntities = Arrays.stream(entities).filter(monitoredEntityClass::isInstance)
+                .sorted(topologicalOrderComparator).toArray();
         var loopedDescriptor =
                 sortedDeclarativeShadowVariableDescriptors.get(0).getEntityDescriptor().getShadowVariableLoopedDescriptor();
 
@@ -55,9 +63,9 @@ public SingleDirectionalParentVariableReferenceGraph(
             }
         }
 
-        for (var shadowEntity : shadowEntities) {
-            updateChanged(shadowEntity);
-        }
+        changedEntities.addAll(List.of(shadowEntities));
+        updateChanged();
+
         if (loopedDescriptor != null) {
             for (var shadowEntity : shadowEntities) {
                 changedVariableNotifier.beforeVariableChanged().accept(loopedDescriptor, shadowEntity);
@@ -70,15 +78,29 @@ public SingleDirectionalParentVariableReferenceGraph(
     @Override
     public void updateChanged() {
         isUpdating = true;
+        changedEntities.sort(topologicalOrderComparator);
+        var processed = new IdentityHashMap<>();
         for (var changedEntity : changedEntities) {
-            updateChanged(changedEntity);
+            var key = keyFunction.apply(changedEntity);
+            var lastProcessed = processed.get(key);
+            if (lastProcessed == null || topologicalOrderComparator.compare(lastProcessed, changedEntity) < 0) {
+                lastProcessed = updateChanged(changedEntity);
+                processed.put(key, lastProcessed);
+            }
         }
         isUpdating = false;
         changedEntities.clear();
     }
 
-    private void updateChanged(Object entity) {
+    /**
+     * Update entities and its successor until one of them does not change.
+     *
+     * @param entity The first entity to process.
+     * @return The last processed entity (i.e. the first entity that did not change).
+     */
+    private Object updateChanged(Object entity) {
         var current = entity;
+        var previous = current;
         while (current != null) {
             var anyChanged = false;
             for (var updater : sortedVariableUpdaterInfos) {
@@ -92,11 +114,13 @@ private void updateChanged(Object entity) {
                 }
             }
             if (anyChanged) {
+                previous = current;
                 current = successorFunction.apply(current);
             } else {
-                current = null;
+                return current;
             }
         }
+        return previous;
     }
 
     @Override

core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/TopologicalSorter.java

@@ -0,0 +1,13 @@
+package ai.timefold.solver.core.impl.domain.variable.declarative;
+
+import java.util.Comparator;
+import java.util.function.UnaryOperator;
+
+import org.jspecify.annotations.NullMarked;
+import org.jspecify.annotations.Nullable;
+
+@NullMarked
+public record TopologicalSorter(UnaryOperator<@Nullable Object> successor,
+        Comparator<Object> comparator,
+        UnaryOperator<Object> key) {
+}

core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/VariableReferenceGraph.java

@@ -3,8 +3,7 @@
 import ai.timefold.solver.core.preview.api.domain.metamodel.VariableMetaModel;
 
 public sealed interface VariableReferenceGraph
-        permits AbstractVariableReferenceGraph, DefaultVariableReferenceGraph, EmptyVariableReferenceGraph,
-        FixedVariableReferenceGraph, SingleDirectionalParentVariableReferenceGraph {
+        permits AbstractVariableReferenceGraph, EmptyVariableReferenceGraph, SingleDirectionalParentVariableReferenceGraph {
 
     void updateChanged();
 

core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/VariableReferenceGraphBuilder.java

@@ -70,12 +70,7 @@ public void addAfterProcessor(GraphChangeType graphChangeType, VariableMetaModel
                 .add(consumer);
     }
 
-    @SuppressWarnings("unchecked")
     public VariableReferenceGraph build(IntFunction<TopologicalOrderGraph> graphCreator) {
-        // TODO empty shows up in VRP example when using it as CVRP, not CVRPTW
-        //  In that case, TimeWindowedCustomer does not exist
-        //  and therefore Customer has no shadow variable.
-        //  Surely there has to be an earlier way to catch this?
         if (instanceList.isEmpty()) {
             return EmptyVariableReferenceGraph.INSTANCE;
         }

core/src/test/java/ai/timefold/solver/core/impl/domain/variable/declarative/GraphStructureTest.java

@@ -32,8 +32,7 @@ void simpleListStructure() {
     void simpleChainedStructure() {
         assertThat(GraphStructure.determineGraphStructure(
                 TestdataChainedSimpleVarSolution.buildSolutionDescriptor()))
-                .hasFieldOrPropertyWithValue("structure", SINGLE_DIRECTIONAL_PARENT)
-                .hasFieldOrPropertyWithValue("direction", ParentVariableType.CHAINED_NEXT);
+                .hasFieldOrPropertyWithValue("structure", ARBITRARY);
     }
 
     @Test