Skip to content

chore: simplify DSV's affected entity update #1619

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
triceo wants to merge 1 commit into from
Closed

chore: simplify DSV's affected entity update #1619

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 2 additions & 64 deletions ...ava/ai/timefold/solver/core/impl/domain/variable/declarative/AffectedEntitiesUpdater.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,37 +4,29 @@
import java.util.List;
import java.util.Objects;
import java.util.PriorityQueue;
import java.util.Set;
import java.util.function.Consumer;
import java.util.function.Function;

import ai.timefold.solver.core.impl.domain.variable.descriptor.VariableDescriptor;
import ai.timefold.solver.core.impl.util.LinkedIdentityHashSet;

final class AffectedEntitiesUpdater<Solution_>
implements Consumer<BitSet> {

// From WorkingReferenceGraph.
private final BaseTopologicalOrderGraph graph;
private final List<EntityVariablePair<Solution_>> instanceList; // Immutable.
private final Function<Object, List<EntityVariablePair<Solution_>>> entityVariablePairFunction;
private final ChangedVariableNotifier<Solution_> changedVariableNotifier;

// Internal state; expensive to create, therefore we reuse.
private final AffectedEntities<Solution_> affectedEntities;
private final LoopedTracker loopedTracker;
private final BitSet visited;
private final PriorityQueue<BaseTopologicalOrderGraph.NodeTopologicalOrder> changeQueue;

AffectedEntitiesUpdater(BaseTopologicalOrderGraph graph, List<EntityVariablePair<Solution_>> instanceList,
Function<Object, List<EntityVariablePair<Solution_>>> entityVariablePairFunction,
ChangedVariableNotifier<Solution_> changedVariableNotifier) {
this.graph = graph;
this.instanceList = instanceList;
this.entityVariablePairFunction = entityVariablePairFunction;
this.changedVariableNotifier = changedVariableNotifier;
var instanceCount = instanceList.size();
this.affectedEntities = new AffectedEntities<>(this::updateLoopedStatusOfAffectedEntity);
this.loopedTracker = new LoopedTracker(instanceCount);
this.visited = new BitSet(instanceCount);
this.changeQueue = new PriorityQueue<>(instanceCount);
Expand Down Expand Up @@ -64,7 +56,6 @@ public void accept(BitSet changed) {
}
}

affectedEntities.processAndClear();
// Prepare for the next time updateChanged() is called.
// No need to clear changeQueue, as that already finishes empty.
loopedTracker.clear();
Expand All @@ -90,30 +81,6 @@ private void initializeChangeQueue(BitSet changed) {
changed.clear();
}

private void updateLoopedStatusOfAffectedEntity(Object affectedEntity) {

@triceo triceo May 29, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we no longer defer the updates (see below), this is no longer necessary.

ShadowVariableLoopedVariableDescriptor<Solution_> shadowVariableLoopedDescriptor = null;
var isEntityLooped = false;
for (var node : entityVariablePairFunction.apply(affectedEntity)) {
// All variables come from the same entity,
// therefore all have the same looped marker.
shadowVariableLoopedDescriptor = node.variableReference().shadowVariableLoopedDescriptor();
if (graph.isLooped(loopedTracker, node.graphNodeId())) {
isEntityLooped = true;
break;
}
}
if (shadowVariableLoopedDescriptor == null) {
// At this point, affectedEntity is guaranteed to have looped marker.
// Otherwise AffectedEntities would not have sent it here.
throw new IllegalStateException("Impossible state: loop marker descriptor does not exist.");
}
var oldValue = shadowVariableLoopedDescriptor.getValue(affectedEntity);
if (!Objects.equals(oldValue, isEntityLooped)) {
changeShadowVariableAndNotify(shadowVariableLoopedDescriptor, affectedEntity, isEntityLooped);
}

}

private boolean updateShadowVariable(EntityVariablePair<Solution_> entityVariable, boolean isLooped) {
var entity = entityVariable.entity();
var shadowVariableReference = entityVariable.variableReference();
Expand All @@ -122,21 +89,19 @@ private boolean updateShadowVariable(EntityVariablePair<Solution_> entityVariabl
if (loopDescriptor != null) {
var oldLooped = (boolean) loopDescriptor.getValue(entity);
if (oldLooped != isLooped) {
// Loop status change; add to affected entities
affectedEntities.add(entityVariable);

@triceo triceo May 29, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was indirection. Put into a set, affect later. Seemingly unnecessary, since the invariant is that the loop status changed from previous, and therefore we need to execute the update even if we had already executed it on this entity before. (The set wouldn't save any work.)

@Christopher-Chianelli Christopher-Chianelli May 29, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect; the reason why entities are added to a set is becauses shadow variables can have different isLooped status, and the looped shadow variable is true if any of them are looped. Consider this situlation:

@ShadowVariable(supplierName="distanceSupplier")
Long distance;

@ShadowVariable(supplierName="arrivalTimeSupplier")
LocalDateTime arrivalTime;

@ShadowVariable(supplierName="serviceStartTimeSupplier")
LocalDateTime serviceStartTime;

@ShadowVariable(supplierName="departureTime")
LocalDateTime departureTime;

@PreviousElementShadowVariable(sourceVariableName="visits")
Visit previous;

List<Visit> visitGroup;

@ShadowSources("previous")
public Long distanceSupplier() {
    if (previous == null) {
        return 0L;
    }
    return previous.distance + 10L;
}

// the normal code for arrivalTime, serviceStartTime, departureTime goes here

In this case, distance can never be looped, but serviceStartTime might be. So if serviceStartTime is looped, and is updated before distance, the distance update would incorrectly set the loop status of the entity to false.

// Loop status change; mark it
changeShadowVariableAndNotify(loopDescriptor, entity, isLooped);
}
}

if (isLooped) {
if (oldValue != null) {
affectedEntities.add(entityVariable);

@triceo triceo May 29, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is effectively dead code. The add method would check if the loop descriptor is not null and it would only have scheduled the update in that case. Since the update in this case already happened above, this code is no longer necessary.

changeShadowVariableAndNotify(shadowVariableReference, entity, null);
}
return true;
} else {
var newValue = shadowVariableReference.calculator().apply(entity);
if (!Objects.equals(oldValue, newValue)) {
affectedEntities.add(entityVariable);

@triceo triceo May 29, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same code, same logic.

changeShadowVariableAndNotify(shadowVariableReference, entity, newValue);
return true;
}
Expand All @@ -157,31 +122,4 @@ private void changeShadowVariableAndNotify(VariableDescriptor<Solution_> variabl
changedVariableNotifier.afterVariableChanged().accept(variableDescriptor, entity);
}

private static final class AffectedEntities<Solution_> {

@triceo triceo May 29, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we no longer defer the updates, we no longer need this.


private final Consumer<Object> consumer;
private final Set<Object> entitiesForLoopedVarUpdateSet;

public AffectedEntities(Consumer<Object> consumer) {
this.consumer = consumer;
this.entitiesForLoopedVarUpdateSet = new LinkedIdentityHashSet<>();
}

public void add(EntityVariablePair<Solution_> shadowVariable) {
var shadowVariableLoopedDescriptor = shadowVariable.variableReference().shadowVariableLoopedDescriptor();
if (shadowVariableLoopedDescriptor == null) {
return;
}
entitiesForLoopedVarUpdateSet.add(shadowVariable.entity());
}

public void processAndClear() {
for (var entity : entitiesForLoopedVarUpdateSet) {
consumer.accept(entity);
}
entitiesForLoopedVarUpdateSet.clear();
}

}

}
8 changes: 1 addition & 7 deletions .../timefold/solver/core/impl/domain/variable/declarative/DefaultVariableReferenceGraph.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ public DefaultVariableReferenceGraph(VariableReferenceGraphBuilder<Solution_> ou
graph.withNodeData(instanceList);
changed = new BitSet(instanceCount);

var entityToVariableReferenceMap = new IdentityHashMap<Object, List<EntityVariablePair<Solution_>>>();
var visited = Collections.newSetFromMap(new IdentityHashMap<>());
for (var instance : instanceList) {
var entity = instance.entity();
Expand All @@ -58,21 +57,16 @@ public DefaultVariableReferenceGraph(VariableReferenceGraphBuilder<Solution_> ou
afterVariableChanged(variableId, entity);
}
}
entityToVariableReferenceMap.computeIfAbsent(entity, ignored -> new ArrayList<>())
.add(instance);
}
for (var fixedEdgeEntry : outerGraph.fixedEdges.entrySet()) {
for (var toEdge : fixedEdgeEntry.getValue()) {
addEdge(fixedEdgeEntry.getKey(), toEdge);
}
}
// Immutable optimized version of the map, now that it won't be updated anymore.
var immutableEntityToVariableReferenceMap = mapOfListsDeepCopyOf(entityToVariableReferenceMap);
// This mutable structure is created once, and reused from there on.
// Otherwise its internal collections were observed being re-created so often
// that the allocation of arrays would become a major bottleneck.
affectedEntitiesUpdater = new AffectedEntitiesUpdater<>(graph, instanceList, immutableEntityToVariableReferenceMap::get,
outerGraph.changedVariableNotifier);
affectedEntitiesUpdater = new AffectedEntitiesUpdater<>(graph, instanceList, outerGraph.changedVariableNotifier);
}

@Override
Expand Down
Loading