Skip to content

Commit 8c67908

Browse files
Christopher-ChianelliChristopher-Chianelli
committed
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.
1 parent 81374a6 commit 8c67908

9 files changed

Lines changed: 364 additions & 33 deletions

File tree

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

Lines changed: 19 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -14,17 +14,14 @@
1414
import java.util.Objects;
1515
import java.util.Set;
1616
import java.util.function.IntFunction;
17-
import java.util.function.UnaryOperator;
1817

1918
import ai.timefold.solver.core.impl.domain.solution.descriptor.SolutionDescriptor;
2019
import ai.timefold.solver.core.impl.domain.variable.descriptor.VariableDescriptor;
21-
import ai.timefold.solver.core.impl.domain.variable.inverserelation.InverseRelationShadowVariableDescriptor;
2220
import ai.timefold.solver.core.impl.score.director.InnerScoreDirector;
2321
import ai.timefold.solver.core.preview.api.domain.metamodel.VariableMetaModel;
2422

2523
import org.jspecify.annotations.NonNull;
2624
import org.jspecify.annotations.NullMarked;
27-
import org.jspecify.annotations.Nullable;
2825
import org.slf4j.Logger;
2926
import org.slf4j.LoggerFactory;
3027

@@ -68,21 +65,22 @@ yield buildSingleDirectionalParentGraph(solutionDescriptor,
6865
};
6966
}
7067

71-
private static <Solution_> VariableReferenceGraph buildSingleDirectionalParentGraph(
68+
static <Solution_> VariableReferenceGraph buildSingleDirectionalParentGraph(
7269
SolutionDescriptor<Solution_> solutionDescriptor,
7370
ChangedVariableNotifier<Solution_> changedVariableNotifier,
7471
GraphStructure.GraphStructureAndDirection graphStructureAndDirection,
7572
Object[] entities) {
7673
var declarativeShadowVariables = solutionDescriptor.getDeclarativeShadowVariableDescriptors();
7774
var sortedDeclarativeVariables = topologicallySortedDeclarativeShadowVariables(declarativeShadowVariables);
7875

79-
var successorFunction =
80-
getSuccessorFunction(solutionDescriptor, Objects.requireNonNull(changedVariableNotifier.innerScoreDirector()),
76+
var topologicalSorter =
77+
getTopologicalSorter(solutionDescriptor,
78+
Objects.requireNonNull(changedVariableNotifier.innerScoreDirector()),
8179
Objects.requireNonNull(graphStructureAndDirection.parentMetaModel()),
8280
Objects.requireNonNull(graphStructureAndDirection.direction()));
8381

84-
return new SingleDirectionalParentVariableReferenceGraph<>(sortedDeclarativeVariables, successorFunction,
85-
changedVariableNotifier, entities);
82+
return new SingleDirectionalParentVariableReferenceGraph<>(sortedDeclarativeVariables,
83+
topologicalSorter, changedVariableNotifier, entities);
8684
}
8785

8886
private static <Solution_> @NonNull List<DeclarativeShadowVariableDescriptor<Solution_>>
@@ -120,21 +118,22 @@ private static <Solution_> VariableReferenceGraph buildSingleDirectionalParentGr
120118
return sortedDeclarativeVariables;
121119
}
122120

123-
private static <Solution_> @NonNull UnaryOperator<@Nullable Object> getSuccessorFunction(
121+
private static <Solution_> TopologicalSorter getTopologicalSorter(
124122
SolutionDescriptor<Solution_> solutionDescriptor, InnerScoreDirector<Solution_, ?> scoreDirector,
125123
VariableMetaModel<?, ?, ?> parentMetaModel, ParentVariableType parentVariableType) {
126124
return switch (parentVariableType) {
127-
case PREVIOUS ->
128-
scoreDirector.getListVariableStateSupply(solutionDescriptor.getListVariableDescriptor())::getNextElement;
129-
case NEXT ->
130-
scoreDirector.getListVariableStateSupply(solutionDescriptor.getListVariableDescriptor())::getPreviousElement;
131-
case CHAINED_NEXT -> {
132-
var entityDescriptor = solutionDescriptor.getEntityDescriptorStrict(parentMetaModel.entity().type());
133-
var inverseVariable = (InverseRelationShadowVariableDescriptor<?>) entityDescriptor
134-
.getShadowVariableDescriptor(parentMetaModel.name());
135-
var sourceVariable = inverseVariable.getSourceVariableDescriptorList().get(0);
136-
var entityType = sourceVariable.getEntityDescriptor().getEntityClass();
137-
yield old -> entityType.isInstance(old) ? sourceVariable.getValue(old) : null;
125+
case PREVIOUS -> {
126+
var listStateSupply = scoreDirector.getListVariableStateSupply(solutionDescriptor.getListVariableDescriptor());
127+
yield new TopologicalSorter(listStateSupply::getNextElement,
128+
Comparator.comparingInt(entity -> Objects.requireNonNullElse(listStateSupply.getIndex(entity), 0)),
129+
listStateSupply::getInverseSingleton);
130+
}
131+
case NEXT -> {
132+
var listStateSupply = scoreDirector.getListVariableStateSupply(solutionDescriptor.getListVariableDescriptor());
133+
yield new TopologicalSorter(listStateSupply::getPreviousElement,
134+
Comparator.comparingInt(entity -> Objects.requireNonNullElse(listStateSupply.getIndex(entity), 0))
135+
.reversed(),
136+
listStateSupply::getInverseSingleton);
138137
}
139138
default -> throw new IllegalStateException(
140139
"Impossible state: expected parentVariableType to be previous or next but was %s."

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,10 +80,12 @@ public static <Solution_> GraphStructureAndDirection determineGraphStructure(
8080
}
8181
// The group variable is unused/always empty
8282
}
83-
case INDIRECT, INVERSE, VARIABLE -> {
83+
case INDIRECT, INVERSE, VARIABLE, CHAINED_NEXT -> {
84+
// CHAINED_NEXT has a complex comparator function;
85+
// so use ARBITRARY despite the fact it can be represented using SINGLE_DIRECTIONAL_PARENT
8486
return new GraphStructureAndDirection(ARBITRARY, null, null);
8587
}
86-
case NEXT, PREVIOUS, CHAINED_NEXT -> {
88+
case NEXT, PREVIOUS -> {
8789
if (parentMetaModel == null) {
8890
parentMetaModel = variableSource.variableSourceReferences().get(0).variableMetaModel();
8991
directionalType = parentVariableType;

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

Lines changed: 33 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,11 @@
22

33
import java.util.ArrayList;
44
import java.util.Arrays;
5+
import java.util.Comparator;
56
import java.util.HashSet;
7+
import java.util.IdentityHashMap;
68
import java.util.List;
9+
import java.util.Map;
710
import java.util.Objects;
811
import java.util.Set;
912
import java.util.function.UnaryOperator;
@@ -14,6 +17,8 @@ public final class SingleDirectionalParentVariableReferenceGraph<Solution_> impl
1417
private final Set<VariableMetaModel<?, ?, ?>> monitoredSourceVariableSet;
1518
private final VariableUpdaterInfo<Solution_>[] sortedVariableUpdaterInfos;
1619
private final UnaryOperator<Object> successorFunction;
20+
private final Comparator<Object> topologicalOrderComparator;
21+
private final UnaryOperator<Object> keyFunction;
1722
private final ChangedVariableNotifier<Solution_> changedVariableNotifier;
1823
private final List<Object> changedEntities;
1924
private final Class<?> monitoredEntityClass;
@@ -22,7 +27,7 @@ public final class SingleDirectionalParentVariableReferenceGraph<Solution_> impl
2227
@SuppressWarnings("unchecked")
2328
public SingleDirectionalParentVariableReferenceGraph(
2429
List<DeclarativeShadowVariableDescriptor<Solution_>> sortedDeclarativeShadowVariableDescriptors,
25-
UnaryOperator<Object> successorFunction,
30+
TopologicalSorter topologicalSorter,
2631
ChangedVariableNotifier<Solution_> changedVariableNotifier,
2732
Object[] entities) {
2833
monitoredEntityClass = sortedDeclarativeShadowVariableDescriptors.get(0).getEntityDescriptor().getEntityClass();
@@ -31,9 +36,12 @@ public SingleDirectionalParentVariableReferenceGraph(
3136
changedEntities = new ArrayList<>();
3237
isUpdating = false;
3338

34-
this.successorFunction = successorFunction;
39+
this.successorFunction = topologicalSorter.successor();
40+
this.topologicalOrderComparator = topologicalSorter.comparator();
41+
this.keyFunction = topologicalSorter.key();
3542
this.changedVariableNotifier = changedVariableNotifier;
36-
var shadowEntities = Arrays.stream(entities).filter(monitoredEntityClass::isInstance).toArray();
43+
var shadowEntities = Arrays.stream(entities).filter(monitoredEntityClass::isInstance)
44+
.sorted(topologicalOrderComparator).toArray();
3745
var loopedDescriptor =
3846
sortedDeclarativeShadowVariableDescriptors.get(0).getEntityDescriptor().getShadowVariableLoopedDescriptor();
3947

@@ -55,9 +63,9 @@ public SingleDirectionalParentVariableReferenceGraph(
5563
}
5664
}
5765

58-
for (var shadowEntity : shadowEntities) {
59-
updateChanged(shadowEntity);
60-
}
66+
changedEntities.addAll(List.of(shadowEntities));
67+
updateChanged();
68+
6169
if (loopedDescriptor != null) {
6270
for (var shadowEntity : shadowEntities) {
6371
changedVariableNotifier.beforeVariableChanged().accept(loopedDescriptor, shadowEntity);
@@ -70,15 +78,29 @@ public SingleDirectionalParentVariableReferenceGraph(
7078
@Override
7179
public void updateChanged() {
7280
isUpdating = true;
81+
changedEntities.sort(topologicalOrderComparator);
82+
Map<Object, Object> processed = new IdentityHashMap<>();
7383
for (var changedEntity : changedEntities) {
74-
updateChanged(changedEntity);
84+
var key = keyFunction.apply(changedEntity);
85+
var lastProcessed = processed.get(key);
86+
if (lastProcessed == null || topologicalOrderComparator.compare(lastProcessed, changedEntity) < 0) {
87+
lastProcessed = updateChanged(changedEntity);
88+
processed.put(key, lastProcessed);
89+
}
7590
}
7691
isUpdating = false;
7792
changedEntities.clear();
7893
}
7994

80-
private void updateChanged(Object entity) {
95+
/**
96+
* Update entities and its successor until one of them does not change.
97+
*
98+
* @param entity The first entity to process.
99+
* @return The last processed entity (i.e. the first entity that did not change).
100+
*/
101+
private Object updateChanged(Object entity) {
81102
var current = entity;
103+
var previous = current;
82104
while (current != null) {
83105
var anyChanged = false;
84106
for (var updater : sortedVariableUpdaterInfos) {
@@ -92,11 +114,13 @@ private void updateChanged(Object entity) {
92114
}
93115
}
94116
if (anyChanged) {
117+
previous = current;
95118
current = successorFunction.apply(current);
96119
} else {
97-
current = null;
120+
return current;
98121
}
99122
}
123+
return previous;
100124
}
101125

102126
@Override

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
package ai.timefold.solver.core.impl.domain.variable.declarative;
2+
3+
import java.util.Comparator;
4+
import java.util.function.UnaryOperator;
5+
6+
import org.jspecify.annotations.NullMarked;
7+
import org.jspecify.annotations.Nullable;
8+
9+
@NullMarked
10+
public record TopologicalSorter(UnaryOperator<@Nullable Object> successor,
11+
Comparator<Object> comparator,
12+
UnaryOperator<Object> key) {
13+
}

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,7 @@ void simpleListStructure() {
3232
void simpleChainedStructure() {
3333
assertThat(GraphStructure.determineGraphStructure(
3434
TestdataChainedSimpleVarSolution.buildSolutionDescriptor()))
35-
.hasFieldOrPropertyWithValue("structure", SINGLE_DIRECTIONAL_PARENT)
36-
.hasFieldOrPropertyWithValue("direction", ParentVariableType.CHAINED_NEXT);
35+
.hasFieldOrPropertyWithValue("structure", ARBITRARY);
3736
}
3837

3938
@Test

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

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
package ai.timefold.solver.core.impl.domain.variable.declarative;
2+
3+
import static org.assertj.core.api.Assertions.assertThat;
4+
import static org.assertj.core.api.Assertions.assertThatCode;
5+
6+
import java.util.List;
7+
8+
import ai.timefold.solver.core.impl.domain.variable.ListVariableStateSupply;
9+
import ai.timefold.solver.core.impl.score.director.InnerScoreDirector;
10+
import ai.timefold.solver.core.testdomain.declarative.counting.TestdataCountingEntity;
11+
import ai.timefold.solver.core.testdomain.declarative.counting.TestdataCountingSolution;
12+
import ai.timefold.solver.core.testdomain.declarative.counting.TestdataCountingValue;
13+
14+
import org.junit.jupiter.api.Test;
15+
import org.mockito.Mockito;
16+
17+
class SingleDirectionalParentVariableReferenceGraphTest {
18+
@Test
19+
public void supplierMethodsAreOnlyCalledOnce() {
20+
var solutionDescriptor = TestdataCountingSolution.buildSolutionDescriptor();
21+
var graphStructureAndDirection = GraphStructure.determineGraphStructure(solutionDescriptor);
22+
23+
var entity1 = new TestdataCountingEntity("e1");
24+
var entity2 = new TestdataCountingEntity("e2");
25+
26+
var value1 = new TestdataCountingValue("v1");
27+
var value2 = new TestdataCountingValue("v2");
28+
var value3 = new TestdataCountingValue("v3");
29+
var value4 = new TestdataCountingValue("v4");
30+
var value5 = new TestdataCountingValue("v5");
31+
32+
var scoreDirector = Mockito.mock(InnerScoreDirector.class);
33+
var listStateSupply = Mockito.mock(ListVariableStateSupply.class);
34+
Mockito.when(scoreDirector.getListVariableStateSupply(Mockito.any()))
35+
.thenReturn(listStateSupply);
36+
37+
value1.setEntity(entity1);
38+
value1.setPrevious(null);
39+
Mockito.when(listStateSupply.getIndex(value1)).thenReturn(0);
40+
Mockito.when(listStateSupply.getNextElement(value1)).thenReturn(null);
41+
Mockito.when(listStateSupply.getInverseSingleton(value1)).thenReturn(entity1);
42+
43+
value2.setEntity(entity2);
44+
value2.setPrevious(null);
45+
Mockito.when(listStateSupply.getIndex(value2)).thenReturn(0);
46+
Mockito.when(listStateSupply.getNextElement(value2)).thenReturn(value3);
47+
Mockito.when(listStateSupply.getInverseSingleton(value2)).thenReturn(entity2);
48+
49+
value3.setEntity(entity2);
50+
value3.setPrevious(value2);
51+
Mockito.when(listStateSupply.getIndex(value3)).thenReturn(1);
52+
Mockito.when(listStateSupply.getNextElement(value3)).thenReturn(value4);
53+
Mockito.when(listStateSupply.getInverseSingleton(value3)).thenReturn(entity2);
54+
55+
value4.setEntity(entity2);
56+
value4.setPrevious(value3);
57+
Mockito.when(listStateSupply.getIndex(value4)).thenReturn(2);
58+
Mockito.when(listStateSupply.getNextElement(value4)).thenReturn(null);
59+
Mockito.when(listStateSupply.getInverseSingleton(value4)).thenReturn(entity2);
60+
61+
value5.setEntity(null);
62+
value5.setPrevious(null);
63+
Mockito.when(listStateSupply.getIndex(value5)).thenReturn(-1);
64+
Mockito.when(listStateSupply.getNextElement(value5)).thenReturn(null);
65+
Mockito.when(listStateSupply.getInverseSingleton(value5)).thenReturn(null);
66+
67+
var values = List.of(value1, value2, value3, value4, value5);
68+
69+
@SuppressWarnings("unchecked")
70+
var graph = DefaultShadowVariableSessionFactory.buildSingleDirectionalParentGraph(solutionDescriptor,
71+
ChangedVariableNotifier.of(scoreDirector),
72+
graphStructureAndDirection,
73+
new Object[] { entity1, entity2, value5, value4, value3, value2, value1 });
74+
75+
assertThat(value1.getCount()).isZero();
76+
assertThat(value2.getCount()).isZero();
77+
assertThat(value3.getCount()).isOne();
78+
assertThat(value4.getCount()).isEqualTo(2);
79+
assertThat(value5.getCount()).isNull();
80+
81+
values.forEach(TestdataCountingValue::reset);
82+
Mockito.reset(listStateSupply);
83+
84+
value2.setPrevious(value3);
85+
value3.setPrevious(value5);
86+
value5.setEntity(entity2);
87+
value4.setPrevious(value2);
88+
89+
Mockito.when(listStateSupply.getIndex(value1)).thenReturn(0);
90+
Mockito.when(listStateSupply.getNextElement(value1)).thenReturn(null);
91+
Mockito.when(listStateSupply.getInverseSingleton(value1)).thenReturn(entity1);
92+
93+
Mockito.when(listStateSupply.getIndex(value5)).thenReturn(0);
94+
Mockito.when(listStateSupply.getNextElement(value5)).thenReturn(value3);
95+
Mockito.when(listStateSupply.getInverseSingleton(value5)).thenReturn(entity2);
96+
97+
Mockito.when(listStateSupply.getIndex(value3)).thenReturn(1);
98+
Mockito.when(listStateSupply.getNextElement(value3)).thenReturn(value2);
99+
Mockito.when(listStateSupply.getInverseSingleton(value3)).thenReturn(entity2);
100+
101+
Mockito.when(listStateSupply.getIndex(value2)).thenReturn(2);
102+
Mockito.when(listStateSupply.getNextElement(value2)).thenReturn(value4);
103+
Mockito.when(listStateSupply.getInverseSingleton(value2)).thenReturn(entity2);
104+
105+
Mockito.when(listStateSupply.getIndex(value4)).thenReturn(3);
106+
Mockito.when(listStateSupply.getNextElement(value4)).thenReturn(null);
107+
Mockito.when(listStateSupply.getInverseSingleton(value4)).thenReturn(entity2);
108+
109+
var previousVariableMetamodel =
110+
solutionDescriptor.getMetaModel().entity(TestdataCountingValue.class).variable("previous");
111+
var entityVariableMetamodel = solutionDescriptor.getMetaModel().entity(TestdataCountingValue.class).variable("entity");
112+
113+
graph.afterVariableChanged(previousVariableMetamodel, value2);
114+
graph.afterVariableChanged(previousVariableMetamodel, value3);
115+
graph.afterVariableChanged(entityVariableMetamodel, value5);
116+
graph.afterVariableChanged(previousVariableMetamodel, value4);
117+
118+
assertThatCode(graph::updateChanged).doesNotThrowAnyException();
119+
120+
assertThat(value1.getCount()).isZero();
121+
assertThat(value2.getCount()).isEqualTo(2);
122+
assertThat(value3.getCount()).isOne();
123+
assertThat(value4.getCount()).isEqualTo(3);
124+
assertThat(value5.getCount()).isZero();
125+
}
126+
}

‎core/src/test/java/ai/timefold/solver/core/testdomain/declarative/counting/TestdataCountingEntity.java‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
package ai.timefold.solver.core.testdomain.declarative.counting;
2+
3+
import java.util.ArrayList;
4+
import java.util.List;
5+
6+
import ai.timefold.solver.core.api.domain.entity.PlanningEntity;
7+
import ai.timefold.solver.core.api.domain.variable.PlanningListVariable;
8+
import ai.timefold.solver.core.testdomain.TestdataObject;
9+
10+
@PlanningEntity
11+
public class TestdataCountingEntity extends TestdataObject {
12+
@PlanningListVariable
13+
List<TestdataCountingValue> values;
14+
15+
public TestdataCountingEntity() {
16+
values = new ArrayList<>();
17+
}
18+
19+
public TestdataCountingEntity(String code) {
20+
super(code);
21+
values = new ArrayList<>();
22+
}
23+
24+
public List<TestdataCountingValue> getValues() {
25+
return values;
26+
}
27+
28+
public void setValues(List<TestdataCountingValue> values) {
29+
this.values = values;
30+
}
31+
}

0 commit comments

Comments
 (0)