fix: address various issues identified during benchmarking

Signed-off-by: Lukáš Petrovický <[email protected]>

benchmark/src/main/java/ai/timefold/solver/benchmark/impl/statistic/PureSubSingleStatistic.java

@@ -1,5 +1,6 @@
 package ai.timefold.solver.benchmark.impl.statistic;
 
+import java.util.ArrayList;
 import java.util.List;
 
 import jakarta.xml.bind.annotation.XmlSeeAlso;
@@ -30,7 +31,7 @@ public abstract class PureSubSingleStatistic<Solution_, StatisticPoint_ extends
     protected SingleStatisticType singleStatisticType;
 
     @XmlTransient
-    protected List<Chart_> chartList;
+    protected List<Chart_> chartList = new ArrayList<>();
 
     protected PureSubSingleStatistic() {
         // For JAXB.

benchmark/src/main/java/ai/timefold/solver/benchmark/impl/statistic/bestsolutionmutation/BestSolutionMutationProblemStatistic.java

@@ -43,11 +43,12 @@ protected List<LineChart<Long, Long>> generateCharts(BenchmarkReport benchmarkRe
                     long timeMillisSpent = point.getTimeMillisSpent();
                     long mutationCount = point.getMutationCount();
                     builder.add(solverLabel, timeMillisSpent, mutationCount);
+                    builder.add(solverLabel, timeMillisSpent + 1, 0L); // Drop back to zero.
                 }
             }
         }
         return singletonList(builder.build("bestSolutionMutationProblemStatisticChart",
                 problemBenchmarkResult.getName() + " best solution mutation statistic", "Time spent",
-                "Best solution mutation count.", true, true, false));
+                "Best solution mutation count", true, true, false));
     }
 }

benchmark/src/main/resources/ai/timefold/solver/benchmark/impl/report/chart-bar.js.ftl

@@ -73,8 +73,6 @@ var ${chartId} = new Chart(document.getElementById('${chartId}'), {
                         }
                     </#if>
                 },
-                suggestedMin: ${chart.yMin()?cn},
-                suggestedMax: ${chart.yMax()?cn},
                 type: '<#if yAxisLogarithmic>logarithmic<#else>linear</#if>',
                 display: true
             }

benchmark/src/main/resources/ai/timefold/solver/benchmark/impl/report/chart-line.js.ftl

@@ -105,8 +105,6 @@ var ${chartId} = new Chart(document.getElementById('${chartId}'), {
                         }
                     </#if>
                 },
-                suggestedMin: ${chart.yMin()?cn},
-                suggestedMax: ${chart.yMax()?cn},
                 type: '<#if yAxisLogarithmic>logarithmic<#else>linear</#if>',
                 display: true
             }

core/constraint-streams/src/main/java/ai/timefold/solver/constraint/streams/common/InnerConstraintFactory.java

@@ -23,7 +23,6 @@
 import ai.timefold.solver.core.api.score.stream.bi.BiConstraintStream;
 import ai.timefold.solver.core.api.score.stream.bi.BiJoiner;
 import ai.timefold.solver.core.api.score.stream.uni.UniConstraintStream;
-import ai.timefold.solver.core.config.util.ConfigUtils;
 import ai.timefold.solver.core.impl.domain.common.accessor.MemberAccessor;
 import ai.timefold.solver.core.impl.domain.entity.descriptor.EntityDescriptor;
 import ai.timefold.solver.core.impl.domain.solution.descriptor.SolutionDescriptor;
@@ -55,9 +54,7 @@ public <A> BiConstraintStream<A, A> forEachUniquePair(Class<A> sourceClass, BiJo
 
     private <A> DefaultBiJoiner<A, A> buildLessThanId(Class<A> sourceClass) {
         SolutionDescriptor<Solution_> solutionDescriptor = getSolutionDescriptor();
-        MemberAccessor planningIdMemberAccessor =
-                ConfigUtils.findPlanningIdMemberAccessor(sourceClass, solutionDescriptor.getMemberAccessorFactory(),
-                        solutionDescriptor.getDomainAccessType());
+        MemberAccessor planningIdMemberAccessor = solutionDescriptor.getPlanningIdAccessor(sourceClass);
         if (planningIdMemberAccessor == null) {
             throw new IllegalArgumentException("The fromClass (" + sourceClass + ") has no member with a @"
                     + PlanningId.class.getSimpleName() + " annotation,"

core/core-impl/src/main/java/ai/timefold/solver/core/impl/domain/solution/descriptor/DummyMemberAccessor.java

@@ -0,0 +1,71 @@
+package ai.timefold.solver.core.impl.domain.solution.descriptor;
+
+import java.lang.annotation.Annotation;
+import java.lang.reflect.Type;
+import java.util.function.Function;
+
+import ai.timefold.solver.core.impl.domain.common.accessor.MemberAccessor;
+
+final class DummyMemberAccessor implements MemberAccessor {
+
+    static final MemberAccessor INSTANCE = new DummyMemberAccessor();
+
+    private DummyMemberAccessor() {
+
+    }
+
+    @Override
+    public Class<?> getDeclaringClass() {
+        return null;
+    }
+
+    @Override
+    public String getName() {
+        return null;
+    }
+
+    @Override
+    public Class<?> getType() {
+        return null;
+    }
+
+    @Override
+    public Type getGenericType() {
+        return null;
+    }
+
+    @Override
+    public Object executeGetter(Object bean) {
+        return null;
+    }
+
+    @Override
+    public <Fact_, Result_> Function<Fact_, Result_> getGetterFunction() {
+        return null;
+    }
+
+    @Override
+    public boolean supportSetter() {
+        return false;
+    }
+
+    @Override
+    public void executeSetter(Object bean, Object value) {
+
+    }
+
+    @Override
+    public String getSpeedNote() {
+        return null;
+    }
+
+    @Override
+    public <T extends Annotation> T getAnnotation(Class<T> annotationClass) {
+        return null;
+    }
+
+    @Override
+    public <T extends Annotation> T[] getDeclaredAnnotationsByType(Class<T> annotationClass) {
+        return null;
+    }
+}

core/core-impl/src/main/java/ai/timefold/solver/core/impl/domain/solution/descriptor/SolutionDescriptor.java

@@ -153,6 +153,7 @@ private static List<Class<?>> sortEntityClassList(List<Class<?>> entityClassList
     private final Map<Class<?>, EntityDescriptor<Solution_>> entityDescriptorMap = new LinkedHashMap<>();
     private final List<Class<?>> reversedEntityClassList = new ArrayList<>();
     private final ConcurrentMap<Class<?>, EntityDescriptor<Solution_>> lowestEntityDescriptorMap = new ConcurrentHashMap<>();
+    private final ConcurrentMap<Class<?>, MemberAccessor> planningIdMemberAccessorMap = new ConcurrentHashMap<>();
 
     private SolutionCloner<Solution_> solutionCloner;
     private boolean assertModelForCloning = false;
@@ -855,6 +856,28 @@ public int getEntityCount(Solution_ solution) {
         return entityCount.intValue();
     }
 
+    /**
+     * Return accessor for a given member of a given class, if present,
+     * and cache it for future use.
+     *
+     * @param factClass never null
+     * @return null if no such member exists
+     */
+    public MemberAccessor getPlanningIdAccessor(Class<?> factClass) {
+        MemberAccessor memberAccessor = planningIdMemberAccessorMap.get(factClass);
+        if (memberAccessor == null) {
+            memberAccessor =
+                    ConfigUtils.findPlanningIdMemberAccessor(factClass, getMemberAccessorFactory(), getDomainAccessType());
+            MemberAccessor nonNullMemberAccessor = Objects.requireNonNullElse(memberAccessor, DummyMemberAccessor.INSTANCE);
+            planningIdMemberAccessorMap.put(factClass, nonNullMemberAccessor);
+            return memberAccessor;
+        } else if (memberAccessor == DummyMemberAccessor.INSTANCE) {
+            return null;
+        } else {
+            return memberAccessor;
+        }
+    }
+
     public void visitAllEntities(Solution_ solution, Consumer<Object> visitor) {
         visitAllEntities(solution, visitor, collection -> collection.forEach(visitor));
     }

core/core-impl/src/main/java/ai/timefold/solver/core/impl/domain/solution/mutation/MutationCounter.java

@@ -4,9 +4,11 @@
 import java.util.List;
 
 import ai.timefold.solver.core.api.domain.solution.PlanningSolution;
+import ai.timefold.solver.core.impl.domain.common.accessor.MemberAccessor;
 import ai.timefold.solver.core.impl.domain.entity.descriptor.EntityDescriptor;
 import ai.timefold.solver.core.impl.domain.solution.descriptor.SolutionDescriptor;
 import ai.timefold.solver.core.impl.domain.variable.descriptor.GenuineVariableDescriptor;
+import ai.timefold.solver.core.impl.domain.variable.descriptor.ListVariableDescriptor;
 
 /**
  * @param <Solution_> the solution type, the class with the {@link PlanningSolution} annotation
@@ -35,11 +37,34 @@ public int countMutations(Solution_ a, Solution_ b) {
                 Object bEntity = bIt.next();
                 for (GenuineVariableDescriptor<Solution_> variableDescriptor : entityDescriptor
                         .getGenuineVariableDescriptorList()) {
-                    // TODO broken if the value is an entity, because then it's never the same
-                    // But we don't want to depend on value/entity equals() => use surrogate entity IDs to compare
-                    // https://issues.redhat.com/browse/PLANNER-170
-                    if (variableDescriptor.getValue(aEntity) != variableDescriptor.getValue(bEntity)) {
-                        mutationCount++;
+                    if (variableDescriptor.isListVariable()) {
+                        ListVariableDescriptor<Solution_> listVariableDescriptor =
+                                (ListVariableDescriptor<Solution_>) variableDescriptor;
+                        List<Object> aValues = listVariableDescriptor.getListVariable(aEntity);
+                        List<Object> bValues = listVariableDescriptor.getListVariable(bEntity);
+                        int aSize = aValues.size();
+                        int bSize = bValues.size();
+                        if (aSize != bSize) {
+                            // First add mutations for the elements that are missing in one list.
+                            mutationCount += Math.abs(aSize - bSize);
+                        }
+                        // Then iterate over the list and count every item that is different.
+                        for (int i = 0; i < aSize; i++) {
+                            if (i == bSize) { // The other list has no more items.
+                                break;
+                            }
+                            Object aValue = aValues.get(i);
+                            Object bValue = bValues.get(i);
+                            if (areDifferent(aValue, bValue)) {
+                                mutationCount++;
+                            }
+                        }
+                    } else {
+                        Object aValue = variableDescriptor.getValue(aEntity);
+                        Object bValue = variableDescriptor.getValue(bEntity);
+                        if (areDifferent(aValue, bValue)) {
+                            mutationCount++;
+                        }
                     }
                 }
             }
@@ -51,6 +76,44 @@ public int countMutations(Solution_ a, Solution_ b) {
         return mutationCount;
     }
 
+    private boolean areDifferent(Object aValue, Object bValue) {
+        EntityDescriptor<Solution_> aValueEntityDescriptor =
+                solutionDescriptor.findEntityDescriptor(aValue.getClass());
+        EntityDescriptor<Solution_> bValueEntityDescriptor =
+                solutionDescriptor.findEntityDescriptor(bValue.getClass());
+        if (aValueEntityDescriptor == null && bValueEntityDescriptor == null) { // Neither are entities.
+            if (aValue == bValue) {
+                return false;
+            }
+            return areDifferentPlanningIds(aValue, bValue);
+        } else if (aValueEntityDescriptor != null && bValueEntityDescriptor != null) {
+            /*
+             * Both are entities.
+             * Entities will all be cloned and therefore different.
+             * But maybe they have the same planning ID?
+             */
+            if (aValueEntityDescriptor != bValueEntityDescriptor) {
+                // Different entities means mutation guaranteed.
+                return true;
+            }
+            return areDifferentPlanningIds(aValue, bValue);
+        } else { // One is entity and the other one is not.
+            return true;
+        }
+    }
+
+    private boolean areDifferentPlanningIds(Object aValue, Object bValue) {
+        MemberAccessor aIdAccessor = solutionDescriptor.getPlanningIdAccessor(aValue.getClass());
+        MemberAccessor bIdAccessor = solutionDescriptor.getPlanningIdAccessor(bValue.getClass());
+        if (aIdAccessor != null && bIdAccessor != null) {
+            Object aId = aIdAccessor.executeGetter(aValue);
+            Object bId = bIdAccessor.executeGetter(bValue);
+            return !aId.equals(bId);
+        } else {
+            return aValue != bValue; // This counts all entities that get as far as here.
+        }
+    }
+
     @Override
     public String toString() {
         return "MutationCounter(" + solutionDescriptor + ")";

core/core-impl/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/move/generic/list/kopt/TwoOptListMove.java

@@ -212,6 +212,9 @@ public String getSimpleMoveTypeDescription() {
 
     @Override
     public Collection<?> getPlanningEntities() {
+        if (firstEntity == secondEntity) {
+            return Collections.singleton(firstEntity);
+        }
         return Set.of(firstEntity, secondEntity);
     }
 

core/core-impl/src/main/java/ai/timefold/solver/core/impl/score/director/AbstractScoreDirector.java

@@ -4,7 +4,6 @@
 
 import java.util.Collection;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.IdentityHashMap;
 import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
@@ -25,7 +24,6 @@
 import ai.timefold.solver.core.api.score.stream.ConstraintJustification;
 import ai.timefold.solver.core.api.score.stream.DefaultConstraintJustification;
 import ai.timefold.solver.core.config.solver.EnvironmentMode;
-import ai.timefold.solver.core.config.util.ConfigUtils;
 import ai.timefold.solver.core.impl.domain.common.accessor.MemberAccessor;
 import ai.timefold.solver.core.impl.domain.constraintweight.descriptor.ConstraintConfigurationDescriptor;
 import ai.timefold.solver.core.impl.domain.entity.descriptor.EntityDescriptor;
@@ -58,7 +56,6 @@ public abstract class AbstractScoreDirector<Solution_, Score_ extends Score<Scor
 
     protected final transient Logger logger = LoggerFactory.getLogger(getClass());
 
-    private final Map<Class, MemberAccessor> planningIdAccessorCacheMap = new HashMap<>(0);
     protected final Factory_ scoreDirectorFactory;
     protected final boolean lookUpEnabled;
     protected final LookUpManager lookUpManager;
@@ -175,15 +172,8 @@ public void assertNonNullPlanningIds() {
     }
 
     private void assertNonNullPlanningId(Object fact) {
-        Class factClass = fact.getClass();
-        // Cannot use Map.computeIfAbsent(), as we also want to cache null values.
-        if (!planningIdAccessorCacheMap.containsKey(factClass)) {
-            SolutionDescriptor<Solution_> solutionDescriptor = getSolutionDescriptor();
-            planningIdAccessorCacheMap.put(factClass,
-                    ConfigUtils.findPlanningIdMemberAccessor(factClass, solutionDescriptor.getMemberAccessorFactory(),
-                            solutionDescriptor.getDomainAccessType()));
-        }
-        MemberAccessor planningIdAccessor = planningIdAccessorCacheMap.get(factClass);
+        Class<?> factClass = fact.getClass();
+        MemberAccessor planningIdAccessor = getSolutionDescriptor().getPlanningIdAccessor(factClass);
         if (planningIdAccessor == null) { // There is no planning ID annotation.
             return;
         }

docs/src/modules/ROOT/pages/benchmarking-and-tweaking/benchmarking-and-tweaking.adoc

@@ -761,6 +761,11 @@ image::benchmarking-and-tweaking/bestSolutionMutationStatistic.png[align="center
 
 Use Tabu Search - an algorithm that behaves like a human - to get an estimation on how difficult it would be for a human to improve the previous best solution to that new best solution.
 
+[NOTE]
+====
+This statistic can slow down the solver noticeably, which affects the benchmark results.
+That's why it is optional and not enabled by default.
+====
 
 [[benchmarkReportMoveCountPerStepStatistic]]
 === Move count per step statistic (graph and CSV)
@@ -842,7 +847,7 @@ Multiple `singleStatisticType` elements are allowed.
 
 [NOTE]
 ====
-These statistic per single benchmark can slow down the solver noticeably, which affects the benchmark results.
+This statistic per single benchmark can slow down the solver noticeably, which affects the benchmark results.
 That's why they are optional and not enabled by default.
 ====