perf: read value range size instead of the value range

To read an entire value range to get its size, the value range needs to be first converted to a collection. This is costly and we therefore implement a shortcut that allows to read the size without creating a collection.
TimefoldAI · Aug 14, 2023 · 64ff136 · 64ff136
1 parent 0bc4ec1
commit 64ff136
Show file tree

Hide file tree

Showing 11 changed files with 110 additions and 14 deletions.
diff --git a/...main/java/ai/timefold/solver/core/impl/domain/solution/descriptor/SolutionDescriptor.java b/...main/java/ai/timefold/solver/core/impl/domain/solution/descriptor/SolutionDescriptor.java
@@ -1010,7 +1010,9 @@ public long getProblemScale(Solution_ solution) {
      * @return {@code >= 0}
      */
     public int countUninitialized(Solution_ solution) {
-        return countUninitializedVariables(solution) + countUnassignedListVariableValues(solution);
+        int uninitializedVariableCount = countUninitializedVariables(solution);
+        int uninitializedValueCount = countUnassignedListVariableValues(solution);
+        return uninitializedValueCount + uninitializedVariableCount;
     }
 
     /**

diff --git a/...in/java/ai/timefold/solver/core/impl/domain/valuerange/AbstractUncountableValueRange.java b/...in/java/ai/timefold/solver/core/impl/domain/valuerange/AbstractUncountableValueRange.java
@@ -9,7 +9,11 @@
  *
  * @see ValueRange
  * @see ValueRangeFactory
+ * @deprecated Uncountable value ranges were never fully supported in many places throughout the solver
+ *             and therefore never gained traction.
+ *             Use {@link CountableValueRange} instead, and configure a step.
  */
+@Deprecated(forRemoval = true, since = "1.1.0")
 public abstract class AbstractUncountableValueRange<T> implements ValueRange<T> {
 
 }
diff --git a/...a/ai/timefold/solver/core/impl/domain/valuerange/buildin/primdouble/DoubleValueRange.java b/...a/ai/timefold/solver/core/impl/domain/valuerange/buildin/primdouble/DoubleValueRange.java
@@ -5,13 +5,17 @@
 import java.util.Random;
 
 import ai.timefold.solver.core.impl.domain.valuerange.AbstractUncountableValueRange;
+import ai.timefold.solver.core.impl.domain.valuerange.buildin.bigdecimal.BigDecimalValueRange;
 import ai.timefold.solver.core.impl.domain.valuerange.util.ValueRangeIterator;
 
 /**
  * Note: Floating point numbers (float, double) cannot represent a decimal number correctly.
  * If floating point numbers leak into the scoring function, they are likely to cause score corruptions.
  * To avoid that, use either {@link java.math.BigDecimal} or fixed-point arithmetic.
+ *
+ * @deprecated Prefer {@link BigDecimalValueRange}.
  */
+@Deprecated(forRemoval = true, since = "1.1.0")
 public class DoubleValueRange extends AbstractUncountableValueRange<Double> {
 
     private final double from;

diff --git a/...lver/core/impl/domain/valuerange/descriptor/AbstractFromPropertyValueRangeDescriptor.java b/...lver/core/impl/domain/valuerange/descriptor/AbstractFromPropertyValueRangeDescriptor.java
@@ -1,7 +1,9 @@
 package ai.timefold.solver.core.impl.domain.valuerange.descriptor;
 
+import java.lang.reflect.Array;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.LinkedList;
 import java.util.List;
 
 import ai.timefold.solver.core.api.domain.solution.PlanningSolution;
@@ -138,12 +140,49 @@ protected ValueRange<?> readValueRange(Object bean) {
         return valueRange;
     }
 
+    protected long readValueRangeSize(Object bean) {
+        Object valueRangeObject = memberAccessor.executeGetter(bean);
+        if (valueRangeObject == null) {
+            throw new IllegalStateException("The @" + ValueRangeProvider.class.getSimpleName()
+                    + " annotated member (" + memberAccessor
+                    + ") called on bean (" + bean
+                    + ") must not return a null valueRangeObject (" + valueRangeObject + ").");
+        }
+        int size = addNullInValueRange ? 1 : 0;
+        if (collectionWrapping) {
+            return size + ((Collection<Object>) valueRangeObject).size();
+        } else if (arrayWrapping) {
+            return size + Array.getLength(valueRangeObject);
+        }
+        ValueRange<Object> valueRange = (ValueRange<Object>) valueRangeObject;
+        if (valueRange.isEmpty()) {
+            throw new IllegalStateException("The @" + ValueRangeProvider.class.getSimpleName()
+                    + " annotated member (" + memberAccessor
+                    + ") called on bean (" + bean
+                    + ") must not return an empty valueRange (" + valueRangeObject + ").\n"
+                    + "Maybe apply overconstrained planning as described in the documentation.");
+        } else if (valueRange instanceof CountableValueRange<Object> countableValueRange) {
+            return size + countableValueRange.getSize();
+        } else {
+            throw new UnsupportedOperationException("The @" + ValueRangeProvider.class.getSimpleName()
+                    + " annotated member (" + memberAccessor
+                    + ") called on bean (" + bean
+                    + ") is not countable and therefore does not support getSize().");
+        }
+    }
+
     private <T> List<T> transformCollectionToList(Collection<T> collection) {
-        // TODO The user might not be aware of these performance pitfalls with Set and LinkedList:
-        // - If only ValueRange.createOriginalIterator() is used, cloning a Set to a List is a waste of time.
-        // - If the List is a LinkedList, ValueRange.createRandomIterator(Random)
-        //   and ValueRange.get(int) are not efficient.
-        return (collection instanceof List ? (List<T>) collection : new ArrayList<>(collection));
+        if (collection instanceof List<T> list) {
+            if (collection instanceof LinkedList<T> linkedList) {
+                // ValueRange.createRandomIterator(Random) and ValueRange.get(int) wouldn't be efficient.
+                return new ArrayList<>(linkedList);
+            } else {
+                return list;
+            }
+        } else {
+            // TODO If only ValueRange.createOriginalIterator() is used, cloning a Set to a List is a waste of time.
+            return new ArrayList<>(collection);
+        }
     }
 
 }
diff --git a/...timefold/solver/core/impl/domain/valuerange/descriptor/CompositeValueRangeDescriptor.java b/...timefold/solver/core/impl/domain/valuerange/descriptor/CompositeValueRangeDescriptor.java
@@ -70,4 +70,24 @@ public ValueRange<?> extractValueRange(Solution_ solution) {
         return doNullInValueRangeWrapping(new CompositeCountableValueRange(childValueRangeList));
     }
 
+    @Override
+    public long extractValueRangeSize(Solution_ solution, Object entity) {
+        int size = addNullInValueRange ? 1 : 0;
+        for (ValueRangeDescriptor<Solution_> valueRangeDescriptor : childValueRangeDescriptorList) {
+            size += ((CountableValueRange) valueRangeDescriptor.extractValueRange(solution, entity)).getSize();
+        }
+        return size;
+    }
+
+    @Override
+    public long extractValueRangeSize(Solution_ solution) {
+        int size = addNullInValueRange ? 1 : 0;
+        for (ValueRangeDescriptor<Solution_> valueRangeDescriptor : childValueRangeDescriptorList) {
+            EntityIndependentValueRangeDescriptor<Solution_> entityIndependentValueRangeDescriptor =
+                    (EntityIndependentValueRangeDescriptor) valueRangeDescriptor;
+            size += ((CountableValueRange) entityIndependentValueRangeDescriptor.extractValueRange(solution)).getSize();
+        }
+        return size;
+    }
+
 }
diff --git a/.../solver/core/impl/domain/valuerange/descriptor/EntityIndependentValueRangeDescriptor.java b/.../solver/core/impl/domain/valuerange/descriptor/EntityIndependentValueRangeDescriptor.java
@@ -17,4 +17,13 @@ public interface EntityIndependentValueRangeDescriptor<Solution_> extends ValueR
      */
     ValueRange<?> extractValueRange(Solution_ solution);
 
+    /**
+     * As specified by {@link ValueRangeDescriptor#extractValueRangeSize}.
+     *
+     * @param solution never null
+     * @return never null
+     * @see ValueRangeDescriptor#extractValueRangeSize
+     */
+    long extractValueRangeSize(Solution_ solution);
+
 }
diff --git a/...solver/core/impl/domain/valuerange/descriptor/FromEntityPropertyValueRangeDescriptor.java b/...solver/core/impl/domain/valuerange/descriptor/FromEntityPropertyValueRangeDescriptor.java
@@ -30,4 +30,9 @@ public ValueRange<?> extractValueRange(Solution_ solution, Object entity) {
         return readValueRange(entity);
     }
 
+    @Override
+    public long extractValueRangeSize(Solution_ solution, Object entity) {
+        return readValueRangeSize(entity);
+    }
+
 }
diff --git a/...lver/core/impl/domain/valuerange/descriptor/FromSolutionPropertyValueRangeDescriptor.java b/...lver/core/impl/domain/valuerange/descriptor/FromSolutionPropertyValueRangeDescriptor.java
@@ -32,9 +32,19 @@ public ValueRange<?> extractValueRange(Solution_ solution, Object entity) {
         return readValueRange(solution);
     }
 
+    @Override
+    public long extractValueRangeSize(Solution_ solution, Object entity) {
+        return readValueRangeSize(solution);
+    }
+
     @Override
     public ValueRange<?> extractValueRange(Solution_ solution) {
         return readValueRange(solution);
     }
 
+    @Override
+    public long extractValueRangeSize(Solution_ solution) {
+        return readValueRangeSize(solution);
+    }
+
 }
diff --git a/.../java/ai/timefold/solver/core/impl/domain/valuerange/descriptor/ValueRangeDescriptor.java b/.../java/ai/timefold/solver/core/impl/domain/valuerange/descriptor/ValueRangeDescriptor.java
@@ -42,4 +42,13 @@ public interface ValueRangeDescriptor<Solution_> {
      */
     ValueRange<?> extractValueRange(Solution_ solution, Object entity);
 
+    /**
+     * @param solution never null
+     * @param entity never null. To avoid this parameter,
+     *        use {@link EntityIndependentValueRangeDescriptor#extractValueRangeSize} instead.
+     * @return never null
+     * @throws UnsupportedOperationException if {@link #isCountable()} returns false
+     */
+    long extractValueRangeSize(Solution_ solution, Object entity);
+
 }
diff --git a/...va/ai/timefold/solver/core/impl/domain/variable/descriptor/GenuineVariableDescriptor.java b/...va/ai/timefold/solver/core/impl/domain/variable/descriptor/GenuineVariableDescriptor.java
@@ -10,7 +10,6 @@
 import java.util.stream.Stream;
 
 import ai.timefold.solver.core.api.domain.solution.PlanningSolution;
-import ai.timefold.solver.core.api.domain.valuerange.CountableValueRange;
 import ai.timefold.solver.core.api.domain.valuerange.ValueRange;
 import ai.timefold.solver.core.api.domain.valuerange.ValueRangeProvider;
 import ai.timefold.solver.core.api.domain.variable.PlanningListVariable;
@@ -275,11 +274,7 @@ public SelectionSorter<Solution_, Object> getDecreasingStrengthSorter() {
     }
 
     public long getValueCount(Solution_ solution, Object entity) {
-        if (!valueRangeDescriptor.isCountable()) {
-            // TODO report this better than just ignoring it
-            return 0L;
-        }
-        return ((CountableValueRange<?>) valueRangeDescriptor.extractValueRange(solution, entity)).getSize();
+        return valueRangeDescriptor.extractValueRangeSize(solution, entity);
     }
 
     @Override

diff --git a/...i/timefold/solver/core/impl/heuristic/selector/value/FromEntityPropertyValueSelector.java b/...i/timefold/solver/core/impl/heuristic/selector/value/FromEntityPropertyValueSelector.java
@@ -64,8 +64,7 @@ public boolean isNeverEnding() {
 
     @Override
     public long getSize(Object entity) {
-        ValueRange<?> valueRange = valueRangeDescriptor.extractValueRange(workingSolution, entity);
-        return ((CountableValueRange<?>) valueRange).getSize();
+        return valueRangeDescriptor.extractValueRangeSize(workingSolution, entity);
     }
 
     @Override