From 8c67908955f1a9a2b714d156e2f547a13b8e367a Mon Sep 17 00:00:00 2001 From: Christopher Chianelli Date: Thu, 26 Jun 2025 14:11:13 -0400 Subject: [PATCH 1/2] fix: process each entity only once and in topological order 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. --- .../DefaultShadowVariableSessionFactory.java | 39 +++--- .../variable/declarative/GraphStructure.java | 6 +- ...rectionalParentVariableReferenceGraph.java | 42 ++++-- .../declarative/TopologicalSorter.java | 13 ++ .../declarative/GraphStructureTest.java | 3 +- ...ionalParentVariableReferenceGraphTest.java | 126 ++++++++++++++++++ .../counting/TestdataCountingEntity.java | 31 +++++ .../counting/TestdataCountingSolution.java | 64 +++++++++ .../counting/TestdataCountingValue.java | 73 ++++++++++ 9 files changed, 364 insertions(+), 33 deletions(-) create mode 100644 core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/TopologicalSorter.java create mode 100644 core/src/test/java/ai/timefold/solver/core/impl/domain/variable/declarative/SingleDirectionalParentVariableReferenceGraphTest.java create mode 100644 core/src/test/java/ai/timefold/solver/core/testdomain/declarative/counting/TestdataCountingEntity.java create mode 100644 core/src/test/java/ai/timefold/solver/core/testdomain/declarative/counting/TestdataCountingSolution.java create mode 100644 core/src/test/java/ai/timefold/solver/core/testdomain/declarative/counting/TestdataCountingValue.java diff --git a/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/DefaultShadowVariableSessionFactory.java b/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/DefaultShadowVariableSessionFactory.java index fba6889e5c5..0a5e1fabc01 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/DefaultShadowVariableSessionFactory.java +++ b/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; @@ -68,7 +65,7 @@ yield buildSingleDirectionalParentGraph(solutionDescriptor, }; } - private static VariableReferenceGraph buildSingleDirectionalParentGraph( + static VariableReferenceGraph buildSingleDirectionalParentGraph( SolutionDescriptor solutionDescriptor, ChangedVariableNotifier changedVariableNotifier, GraphStructure.GraphStructureAndDirection graphStructureAndDirection, @@ -76,13 +73,14 @@ private static VariableReferenceGraph buildSingleDirectionalParentGr var declarativeShadowVariables = solutionDescriptor.getDeclarativeShadowVariableDescriptors(); var sortedDeclarativeVariables = topologicallySortedDeclarativeShadowVariables(declarativeShadowVariables); - var successorFunction = - getSuccessorFunction(solutionDescriptor, Objects.requireNonNull(changedVariableNotifier.innerScoreDirector()), + var topologicalSorter = + getTopologicalSorter(solutionDescriptor, + Objects.requireNonNull(changedVariableNotifier.innerScoreDirector()), Objects.requireNonNull(graphStructureAndDirection.parentMetaModel()), Objects.requireNonNull(graphStructureAndDirection.direction())); - return new SingleDirectionalParentVariableReferenceGraph<>(sortedDeclarativeVariables, successorFunction, - changedVariableNotifier, entities); + return new SingleDirectionalParentVariableReferenceGraph<>(sortedDeclarativeVariables, + topologicalSorter, changedVariableNotifier, entities); } private static @NonNull List> @@ -120,21 +118,22 @@ private static VariableReferenceGraph buildSingleDirectionalParentGr return sortedDeclarativeVariables; } - private static @NonNull UnaryOperator<@Nullable Object> getSuccessorFunction( + private static TopologicalSorter getTopologicalSorter( SolutionDescriptor solutionDescriptor, InnerScoreDirector scoreDirector, VariableMetaModel parentMetaModel, 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." diff --git a/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/GraphStructure.java b/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/GraphStructure.java index 6e3d0eae6d8..b54ed93b50d 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/GraphStructure.java +++ b/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/GraphStructure.java @@ -80,10 +80,12 @@ public static 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; diff --git a/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/SingleDirectionalParentVariableReferenceGraph.java b/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/SingleDirectionalParentVariableReferenceGraph.java index fe5c137496f..92ccdab96e6 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/SingleDirectionalParentVariableReferenceGraph.java +++ b/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/SingleDirectionalParentVariableReferenceGraph.java @@ -2,8 +2,11 @@ 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.Map; import java.util.Objects; import java.util.Set; import java.util.function.UnaryOperator; @@ -14,6 +17,8 @@ public final class SingleDirectionalParentVariableReferenceGraph impl private final Set> monitoredSourceVariableSet; private final VariableUpdaterInfo[] sortedVariableUpdaterInfos; private final UnaryOperator successorFunction; + private final Comparator topologicalOrderComparator; + private final UnaryOperator keyFunction; private final ChangedVariableNotifier changedVariableNotifier; private final List changedEntities; private final Class monitoredEntityClass; @@ -22,7 +27,7 @@ public final class SingleDirectionalParentVariableReferenceGraph impl @SuppressWarnings("unchecked") public SingleDirectionalParentVariableReferenceGraph( List> sortedDeclarativeShadowVariableDescriptors, - UnaryOperator successorFunction, + TopologicalSorter topologicalSorter, ChangedVariableNotifier 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); + Map 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 diff --git a/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/TopologicalSorter.java b/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/TopologicalSorter.java new file mode 100644 index 00000000000..0a67e4ad692 --- /dev/null +++ b/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 comparator, + UnaryOperator key) { +} diff --git a/core/src/test/java/ai/timefold/solver/core/impl/domain/variable/declarative/GraphStructureTest.java b/core/src/test/java/ai/timefold/solver/core/impl/domain/variable/declarative/GraphStructureTest.java index 5435aa870a7..d58faa39ba1 100644 --- a/core/src/test/java/ai/timefold/solver/core/impl/domain/variable/declarative/GraphStructureTest.java +++ b/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 diff --git a/core/src/test/java/ai/timefold/solver/core/impl/domain/variable/declarative/SingleDirectionalParentVariableReferenceGraphTest.java b/core/src/test/java/ai/timefold/solver/core/impl/domain/variable/declarative/SingleDirectionalParentVariableReferenceGraphTest.java new file mode 100644 index 00000000000..eb3a5f6facc --- /dev/null +++ b/core/src/test/java/ai/timefold/solver/core/impl/domain/variable/declarative/SingleDirectionalParentVariableReferenceGraphTest.java @@ -0,0 +1,126 @@ +package ai.timefold.solver.core.impl.domain.variable.declarative; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; + +import java.util.List; + +import ai.timefold.solver.core.impl.domain.variable.ListVariableStateSupply; +import ai.timefold.solver.core.impl.score.director.InnerScoreDirector; +import ai.timefold.solver.core.testdomain.declarative.counting.TestdataCountingEntity; +import ai.timefold.solver.core.testdomain.declarative.counting.TestdataCountingSolution; +import ai.timefold.solver.core.testdomain.declarative.counting.TestdataCountingValue; + +import org.junit.jupiter.api.Test; +import org.mockito.Mockito; + +class SingleDirectionalParentVariableReferenceGraphTest { + @Test + public void supplierMethodsAreOnlyCalledOnce() { + var solutionDescriptor = TestdataCountingSolution.buildSolutionDescriptor(); + var graphStructureAndDirection = GraphStructure.determineGraphStructure(solutionDescriptor); + + var entity1 = new TestdataCountingEntity("e1"); + var entity2 = new TestdataCountingEntity("e2"); + + var value1 = new TestdataCountingValue("v1"); + var value2 = new TestdataCountingValue("v2"); + var value3 = new TestdataCountingValue("v3"); + var value4 = new TestdataCountingValue("v4"); + var value5 = new TestdataCountingValue("v5"); + + var scoreDirector = Mockito.mock(InnerScoreDirector.class); + var listStateSupply = Mockito.mock(ListVariableStateSupply.class); + Mockito.when(scoreDirector.getListVariableStateSupply(Mockito.any())) + .thenReturn(listStateSupply); + + value1.setEntity(entity1); + value1.setPrevious(null); + Mockito.when(listStateSupply.getIndex(value1)).thenReturn(0); + Mockito.when(listStateSupply.getNextElement(value1)).thenReturn(null); + Mockito.when(listStateSupply.getInverseSingleton(value1)).thenReturn(entity1); + + value2.setEntity(entity2); + value2.setPrevious(null); + Mockito.when(listStateSupply.getIndex(value2)).thenReturn(0); + Mockito.when(listStateSupply.getNextElement(value2)).thenReturn(value3); + Mockito.when(listStateSupply.getInverseSingleton(value2)).thenReturn(entity2); + + value3.setEntity(entity2); + value3.setPrevious(value2); + Mockito.when(listStateSupply.getIndex(value3)).thenReturn(1); + Mockito.when(listStateSupply.getNextElement(value3)).thenReturn(value4); + Mockito.when(listStateSupply.getInverseSingleton(value3)).thenReturn(entity2); + + value4.setEntity(entity2); + value4.setPrevious(value3); + Mockito.when(listStateSupply.getIndex(value4)).thenReturn(2); + Mockito.when(listStateSupply.getNextElement(value4)).thenReturn(null); + Mockito.when(listStateSupply.getInverseSingleton(value4)).thenReturn(entity2); + + value5.setEntity(null); + value5.setPrevious(null); + Mockito.when(listStateSupply.getIndex(value5)).thenReturn(-1); + Mockito.when(listStateSupply.getNextElement(value5)).thenReturn(null); + Mockito.when(listStateSupply.getInverseSingleton(value5)).thenReturn(null); + + var values = List.of(value1, value2, value3, value4, value5); + + @SuppressWarnings("unchecked") + var graph = DefaultShadowVariableSessionFactory.buildSingleDirectionalParentGraph(solutionDescriptor, + ChangedVariableNotifier.of(scoreDirector), + graphStructureAndDirection, + new Object[] { entity1, entity2, value5, value4, value3, value2, value1 }); + + assertThat(value1.getCount()).isZero(); + assertThat(value2.getCount()).isZero(); + assertThat(value3.getCount()).isOne(); + assertThat(value4.getCount()).isEqualTo(2); + assertThat(value5.getCount()).isNull(); + + values.forEach(TestdataCountingValue::reset); + Mockito.reset(listStateSupply); + + value2.setPrevious(value3); + value3.setPrevious(value5); + value5.setEntity(entity2); + value4.setPrevious(value2); + + Mockito.when(listStateSupply.getIndex(value1)).thenReturn(0); + Mockito.when(listStateSupply.getNextElement(value1)).thenReturn(null); + Mockito.when(listStateSupply.getInverseSingleton(value1)).thenReturn(entity1); + + Mockito.when(listStateSupply.getIndex(value5)).thenReturn(0); + Mockito.when(listStateSupply.getNextElement(value5)).thenReturn(value3); + Mockito.when(listStateSupply.getInverseSingleton(value5)).thenReturn(entity2); + + Mockito.when(listStateSupply.getIndex(value3)).thenReturn(1); + Mockito.when(listStateSupply.getNextElement(value3)).thenReturn(value2); + Mockito.when(listStateSupply.getInverseSingleton(value3)).thenReturn(entity2); + + Mockito.when(listStateSupply.getIndex(value2)).thenReturn(2); + Mockito.when(listStateSupply.getNextElement(value2)).thenReturn(value4); + Mockito.when(listStateSupply.getInverseSingleton(value2)).thenReturn(entity2); + + Mockito.when(listStateSupply.getIndex(value4)).thenReturn(3); + Mockito.when(listStateSupply.getNextElement(value4)).thenReturn(null); + Mockito.when(listStateSupply.getInverseSingleton(value4)).thenReturn(entity2); + + var previousVariableMetamodel = + solutionDescriptor.getMetaModel().entity(TestdataCountingValue.class).variable("previous"); + var entityVariableMetamodel = solutionDescriptor.getMetaModel().entity(TestdataCountingValue.class).variable("entity"); + + graph.afterVariableChanged(previousVariableMetamodel, value2); + graph.afterVariableChanged(previousVariableMetamodel, value3); + graph.afterVariableChanged(entityVariableMetamodel, value5); + graph.afterVariableChanged(previousVariableMetamodel, value4); + + assertThatCode(graph::updateChanged).doesNotThrowAnyException(); + + assertThat(value1.getCount()).isZero(); + assertThat(value2.getCount()).isEqualTo(2); + assertThat(value3.getCount()).isOne(); + assertThat(value4.getCount()).isEqualTo(3); + assertThat(value5.getCount()).isZero(); + } +} diff --git a/core/src/test/java/ai/timefold/solver/core/testdomain/declarative/counting/TestdataCountingEntity.java b/core/src/test/java/ai/timefold/solver/core/testdomain/declarative/counting/TestdataCountingEntity.java new file mode 100644 index 00000000000..505dfdae3b2 --- /dev/null +++ b/core/src/test/java/ai/timefold/solver/core/testdomain/declarative/counting/TestdataCountingEntity.java @@ -0,0 +1,31 @@ +package ai.timefold.solver.core.testdomain.declarative.counting; + +import java.util.ArrayList; +import java.util.List; + +import ai.timefold.solver.core.api.domain.entity.PlanningEntity; +import ai.timefold.solver.core.api.domain.variable.PlanningListVariable; +import ai.timefold.solver.core.testdomain.TestdataObject; + +@PlanningEntity +public class TestdataCountingEntity extends TestdataObject { + @PlanningListVariable + List values; + + public TestdataCountingEntity() { + values = new ArrayList<>(); + } + + public TestdataCountingEntity(String code) { + super(code); + values = new ArrayList<>(); + } + + public List getValues() { + return values; + } + + public void setValues(List values) { + this.values = values; + } +} diff --git a/core/src/test/java/ai/timefold/solver/core/testdomain/declarative/counting/TestdataCountingSolution.java b/core/src/test/java/ai/timefold/solver/core/testdomain/declarative/counting/TestdataCountingSolution.java new file mode 100644 index 00000000000..70096eb4a37 --- /dev/null +++ b/core/src/test/java/ai/timefold/solver/core/testdomain/declarative/counting/TestdataCountingSolution.java @@ -0,0 +1,64 @@ +package ai.timefold.solver.core.testdomain.declarative.counting; + +import java.util.List; +import java.util.Set; + +import ai.timefold.solver.core.api.domain.solution.PlanningEntityCollectionProperty; +import ai.timefold.solver.core.api.domain.solution.PlanningScore; +import ai.timefold.solver.core.api.domain.solution.PlanningSolution; +import ai.timefold.solver.core.api.domain.valuerange.ValueRangeProvider; +import ai.timefold.solver.core.api.score.buildin.simple.SimpleScore; +import ai.timefold.solver.core.config.solver.PreviewFeature; +import ai.timefold.solver.core.impl.domain.solution.descriptor.SolutionDescriptor; +import ai.timefold.solver.core.testdomain.TestdataObject; + +@PlanningSolution +public class TestdataCountingSolution extends TestdataObject { + public static SolutionDescriptor buildSolutionDescriptor() { + return SolutionDescriptor.buildSolutionDescriptor(Set.of(PreviewFeature.DECLARATIVE_SHADOW_VARIABLES), + TestdataCountingSolution.class, TestdataCountingEntity.class, TestdataCountingValue.class); + } + + @PlanningEntityCollectionProperty + List entities; + + @PlanningEntityCollectionProperty + @ValueRangeProvider + List values; + + @PlanningScore + SimpleScore score; + + public TestdataCountingSolution() { + } + + public TestdataCountingSolution(String code, List entities, List values) { + super(code); + this.entities = entities; + this.values = values; + } + + public List getEntities() { + return entities; + } + + public void setEntities(List entities) { + this.entities = entities; + } + + public List getValues() { + return values; + } + + public void setValues(List values) { + this.values = values; + } + + public SimpleScore getScore() { + return score; + } + + public void setScore(SimpleScore score) { + this.score = score; + } +} diff --git a/core/src/test/java/ai/timefold/solver/core/testdomain/declarative/counting/TestdataCountingValue.java b/core/src/test/java/ai/timefold/solver/core/testdomain/declarative/counting/TestdataCountingValue.java new file mode 100644 index 00000000000..f2a40e7babe --- /dev/null +++ b/core/src/test/java/ai/timefold/solver/core/testdomain/declarative/counting/TestdataCountingValue.java @@ -0,0 +1,73 @@ +package ai.timefold.solver.core.testdomain.declarative.counting; + +import ai.timefold.solver.core.api.domain.entity.PlanningEntity; +import ai.timefold.solver.core.api.domain.variable.InverseRelationShadowVariable; +import ai.timefold.solver.core.api.domain.variable.PreviousElementShadowVariable; +import ai.timefold.solver.core.api.domain.variable.ShadowVariable; +import ai.timefold.solver.core.preview.api.domain.variable.declarative.ShadowSources; +import ai.timefold.solver.core.testdomain.TestdataObject; + +@PlanningEntity +public class TestdataCountingValue extends TestdataObject { + @PreviousElementShadowVariable(sourceVariableName = "values") + TestdataCountingValue previous; + + @InverseRelationShadowVariable(sourceVariableName = "values") + TestdataCountingEntity entity; + + @ShadowVariable(supplierName = "countSupplier") + Integer count; + + int calledCount = 0; + + public TestdataCountingValue() { + } + + public TestdataCountingValue(String code) { + super(code); + } + + public TestdataCountingValue getPrevious() { + return previous; + } + + public void setPrevious(TestdataCountingValue previous) { + this.previous = previous; + } + + public TestdataCountingEntity getEntity() { + return entity; + } + + public void setEntity(TestdataCountingEntity entity) { + this.entity = entity; + } + + public Integer getCount() { + return count; + } + + public void setCount(Integer count) { + this.count = count; + } + + @ShadowSources({ "previous.count", "entity" }) + public Integer countSupplier() { + if (calledCount != 0) { + throw new IllegalStateException("Supplier for entity %s was already called." + .formatted(entity)); + } + calledCount++; + if (entity == null) { + return null; + } + if (previous == null) { + return 0; + } + return previous.count + 1; + } + + public void reset() { + calledCount = 0; + } +} From 5c48308d9f22c363a21542e60798de4dc3f1b90d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Petrovick=C3=BD?= Date: Fri, 27 Jun 2025 06:59:57 +0200 Subject: [PATCH 2/2] Small tweaks and Sonar --- .../declarative/AbstractVariableReferenceGraph.java | 1 + .../declarative/DefaultShadowVariableSessionFactory.java | 9 +++------ .../declarative/DefaultVariableReferenceGraph.java | 3 +-- .../declarative/FixedVariableReferenceGraph.java | 3 +-- .../SingleDirectionalParentVariableReferenceGraph.java | 4 ++-- .../variable/declarative/VariableReferenceGraph.java | 3 +-- .../declarative/VariableReferenceGraphBuilder.java | 5 ----- ...ingleDirectionalParentVariableReferenceGraphTest.java | 4 +++- 8 files changed, 12 insertions(+), 20 deletions(-) diff --git a/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/AbstractVariableReferenceGraph.java b/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/AbstractVariableReferenceGraph.java index e604c2b38ef..fb4183ce2c5 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/AbstractVariableReferenceGraph.java +++ b/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/AbstractVariableReferenceGraph.java @@ -18,6 +18,7 @@ public abstract sealed class AbstractVariableReferenceGraph implements VariableReferenceGraph permits DefaultVariableReferenceGraph, FixedVariableReferenceGraph { + // These structures are immutable. protected final List> instanceList; protected final Map, Map>> variableReferenceToInstanceMap; diff --git a/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/DefaultShadowVariableSessionFactory.java b/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/DefaultShadowVariableSessionFactory.java index 0a5e1fabc01..ba28768a010 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/DefaultShadowVariableSessionFactory.java +++ b/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/DefaultShadowVariableSessionFactory.java @@ -41,7 +41,6 @@ public DefaultShadowVariableSessionFactory( this.graphCreator = graphCreator; } - @SuppressWarnings("unchecked") public static VariableReferenceGraph buildGraph( SolutionDescriptor solutionDescriptor, VariableReferenceGraphBuilder variableReferenceGraphBuilder, Object[] entities, @@ -76,7 +75,6 @@ static VariableReferenceGraph buildSingleDirectionalParentGraph( var topologicalSorter = getTopologicalSorter(solutionDescriptor, Objects.requireNonNull(changedVariableNotifier.innerScoreDirector()), - Objects.requireNonNull(graphStructureAndDirection.parentMetaModel()), Objects.requireNonNull(graphStructureAndDirection.direction())); return new SingleDirectionalParentVariableReferenceGraph<>(sortedDeclarativeVariables, @@ -118,9 +116,8 @@ static VariableReferenceGraph buildSingleDirectionalParentGraph( return sortedDeclarativeVariables; } - private static TopologicalSorter getTopologicalSorter( - SolutionDescriptor solutionDescriptor, InnerScoreDirector scoreDirector, - VariableMetaModel parentMetaModel, ParentVariableType parentVariableType) { + private static TopologicalSorter getTopologicalSorter(SolutionDescriptor solutionDescriptor, + InnerScoreDirector scoreDirector, ParentVariableType parentVariableType) { return switch (parentVariableType) { case PREVIOUS -> { var listStateSupply = scoreDirector.getListVariableStateSupply(solutionDescriptor.getListVariableDescriptor()); @@ -149,7 +146,7 @@ private static VariableReferenceGraph buildArbitraryGraph( var variableIdToUpdater = new HashMap, VariableUpdaterInfo>(); // 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. diff --git a/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/DefaultVariableReferenceGraph.java b/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/DefaultVariableReferenceGraph.java index a8bcb1084e0..4570b5f880f 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/DefaultVariableReferenceGraph.java +++ b/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 extends AbstractVariableReferenceGraph - implements VariableReferenceGraph { +final class DefaultVariableReferenceGraph extends AbstractVariableReferenceGraph { // These structures are mutable. private final Consumer affectedEntitiesUpdater; diff --git a/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/FixedVariableReferenceGraph.java b/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/FixedVariableReferenceGraph.java index bfc06e180dd..74e113fdf45 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/FixedVariableReferenceGraph.java +++ b/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 - extends AbstractVariableReferenceGraph> - implements VariableReferenceGraph { + extends AbstractVariableReferenceGraph> { // These are immutable private final ChangedVariableNotifier changedVariableNotifier; diff --git a/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/SingleDirectionalParentVariableReferenceGraph.java b/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/SingleDirectionalParentVariableReferenceGraph.java index 92ccdab96e6..f49290a6e62 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/SingleDirectionalParentVariableReferenceGraph.java +++ b/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/SingleDirectionalParentVariableReferenceGraph.java @@ -6,7 +6,6 @@ import java.util.HashSet; import java.util.IdentityHashMap; import java.util.List; -import java.util.Map; import java.util.Objects; import java.util.Set; import java.util.function.UnaryOperator; @@ -14,6 +13,7 @@ import ai.timefold.solver.core.preview.api.domain.metamodel.VariableMetaModel; public final class SingleDirectionalParentVariableReferenceGraph implements VariableReferenceGraph { + private final Set> monitoredSourceVariableSet; private final VariableUpdaterInfo[] sortedVariableUpdaterInfos; private final UnaryOperator successorFunction; @@ -79,7 +79,7 @@ public SingleDirectionalParentVariableReferenceGraph( public void updateChanged() { isUpdating = true; changedEntities.sort(topologicalOrderComparator); - Map processed = new IdentityHashMap<>(); + var processed = new IdentityHashMap<>(); for (var changedEntity : changedEntities) { var key = keyFunction.apply(changedEntity); var lastProcessed = processed.get(key); diff --git a/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/VariableReferenceGraph.java b/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/VariableReferenceGraph.java index 5350c159554..8be7c95c8c9 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/VariableReferenceGraph.java +++ b/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(); diff --git a/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/VariableReferenceGraphBuilder.java b/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/VariableReferenceGraphBuilder.java index 53dce5d2a53..adb6a441a68 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/VariableReferenceGraphBuilder.java +++ b/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 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; } diff --git a/core/src/test/java/ai/timefold/solver/core/impl/domain/variable/declarative/SingleDirectionalParentVariableReferenceGraphTest.java b/core/src/test/java/ai/timefold/solver/core/impl/domain/variable/declarative/SingleDirectionalParentVariableReferenceGraphTest.java index eb3a5f6facc..06970ac431f 100644 --- a/core/src/test/java/ai/timefold/solver/core/impl/domain/variable/declarative/SingleDirectionalParentVariableReferenceGraphTest.java +++ b/core/src/test/java/ai/timefold/solver/core/impl/domain/variable/declarative/SingleDirectionalParentVariableReferenceGraphTest.java @@ -15,8 +15,9 @@ import org.mockito.Mockito; class SingleDirectionalParentVariableReferenceGraphTest { + @Test - public void supplierMethodsAreOnlyCalledOnce() { + void supplierMethodsAreOnlyCalledOnce() { var solutionDescriptor = TestdataCountingSolution.buildSolutionDescriptor(); var graphStructureAndDirection = GraphStructure.determineGraphStructure(solutionDescriptor); @@ -123,4 +124,5 @@ public void supplierMethodsAreOnlyCalledOnce() { assertThat(value4.getCount()).isEqualTo(3); assertThat(value5.getCount()).isZero(); } + }