From b0914548b753efdbffb9b76db7563289e3841114 Mon Sep 17 00:00:00 2001 From: David Legg Date: Thu, 2 May 2024 18:11:07 -0700 Subject: [PATCH 1/5] Make name computation lazy Moves as much string manipulation for naming as possible to lazy computation. In particular, 1. In Resources.forward, we were calling getName whenever we emitted a forwarding effect. 2. In CellRefV2, we were calling getName when combining effects. 3. In MutableResource, we were calling getName when emitting an effect. 4. In several places where we emitted effects, we were calling toString on values to name the effect. To do this, we also augment the Naming class to call Objects.toString in some configurations. This allows us to add arbitrary objects to the naming graph, like effect values, without explicitly naming them, provided they have a decent toString implementation. --- .../contrib/streamline/core/CellRefV2.java | 15 ++++-------- .../streamline/core/MutableResource.java | 22 ++++++++---------- .../contrib/streamline/core/Resources.java | 3 +-- .../contrib/streamline/debugging/Naming.java | 23 ++++++++++++++----- .../modeling/black_box/Approximation.java | 2 +- .../black_box/UnstructuredResources.java | 3 ++- .../modeling/discrete/DiscreteEffects.java | 23 ++++++++++--------- .../polynomial/PolynomialEffects.java | 23 +++++++++++-------- 8 files changed, 60 insertions(+), 54 deletions(-) diff --git a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/core/CellRefV2.java b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/core/CellRefV2.java index c747d0a689..294736ea97 100644 --- a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/core/CellRefV2.java +++ b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/core/CellRefV2.java @@ -39,8 +39,8 @@ public Cell duplicate(Cell cell) { public void apply(Cell cell, E effect) { cell.initialDynamics = effect.apply(cell.dynamics).match( ErrorCatching::success, - error -> failure(new RuntimeException( - "Applying '%s' failed.".formatted(getEffectName(effect)), error))); + error -> failure(new RuntimeException( + "Applying effect '%s' failed.".formatted(getName(effect, "...")), error))); cell.dynamics = cell.initialDynamics; cell.elapsedTime = ZERO; } @@ -120,7 +120,7 @@ public DynamicsEffect empty() { @Override public DynamicsEffect sequentially(final DynamicsEffect prefix, final DynamicsEffect suffix) { final DynamicsEffect result = x -> suffix.apply(prefix.apply(x)); - name(result, "(%s) then (%s)".formatted(getEffectName(prefix), getEffectName(suffix))); + name(result, "(%s) then (%s)", prefix, suffix); return result; } @@ -135,22 +135,17 @@ public DynamicsEffect concurrently(final DynamicsEffect left, final Dynami return failure(e); } }; - name(result, "(%s) and (%s)".formatted(getEffectName(left), getEffectName(right))); + name(result, "(%s) and (%s)", left, right); return result; } catch (Throwable e) { final DynamicsEffect result = $ -> failure(e); - name(result, "Failed to combine concurrent effects: (%s) and (%s)".formatted( - getEffectName(left), getEffectName(right))); + name(result, "Failed to combine concurrent effects: (%s) and (%s)", left, right); return result; } } }; } - private static , E extends DynamicsEffect> String getEffectName(E effect) { - return getName(effect).orElse("anonymous effect"); - } - public static class Cell { public ErrorCatching> initialDynamics; public ErrorCatching> dynamics; diff --git a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/core/MutableResource.java b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/core/MutableResource.java index fb450d3320..7692a5ee2e 100644 --- a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/core/MutableResource.java +++ b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/core/MutableResource.java @@ -22,8 +22,7 @@ public interface MutableResource> extends Resource { void emit(DynamicsEffect effect); default void emit(String effectName, DynamicsEffect effect) { - name(effect, effectName); - emit(effect); + emit(name(effect, effectName)); } static > MutableResource resource(D initial) { @@ -47,21 +46,18 @@ static > MutableResource resource(ErrorCatching effect) { - augmentEffectName(effect); - cell.emit(effect); + // NOTE: The strange pattern of naming effect::apply is to create a new object, identical in behavior to effect, + // which we can assign a more informative name without actually getting the name of effect. + // Replacing effect::apply with effect would create a self-loop in the naming graph on effect, which isn't allowed. + // Using Naming.getName to get effect's current name and use that when elaborating is correct but potentially slow, + // depending on how deep the naming graph is. + cell.emit(name(effect::apply, "%s on %s" + Context.get().stream().map(c -> " during " + c).collect(joining()), effect, this)); } @Override public ErrorCatching> getDynamics() { return cell.get().dynamics; } - - private void augmentEffectName(DynamicsEffect effect) { - String effectName = getName(effect).orElse("anonymous effect"); - String resourceName = getName(this).orElse("anonymous resource"); - String augmentedName = effectName + " on " + resourceName + Context.get().stream().map(c -> " during " + c).collect(joining()); - name(effect, augmentedName); - } }; if (MutableResourceFlags.DETECT_BUSY_CELLS) { result = profileEffects(result); @@ -73,11 +69,11 @@ private void augmentEffectName(DynamicsEffect effect) { } static > void set(MutableResource resource, D newDynamics) { - resource.emit("Set " + newDynamics, DynamicsMonad.effect(x -> newDynamics)); + resource.emit(name(DynamicsMonad.effect(x -> newDynamics), "Set %s", newDynamics)); } static > void set(MutableResource resource, Expiring newDynamics) { - resource.emit("Set " + newDynamics, ErrorCatchingMonad., Expiring>map($ -> newDynamics)::apply); + resource.emit(name(ErrorCatchingMonad., Expiring>map($ -> newDynamics)::apply, "Set %s", newDynamics)); } /** diff --git a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/core/Resources.java b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/core/Resources.java index 998851f285..0b1313a02f 100644 --- a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/core/Resources.java +++ b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/core/Resources.java @@ -183,8 +183,7 @@ public static Condition expires(Resource resource) { */ public static > void forward(Resource source, MutableResource destination) { wheneverDynamicsChange(source, s -> destination.emit( - "Forward %s dynamics: %s".formatted(getName(source).orElse("anonymous"), s), - $ -> s)); + name($ -> s, "Forward %s dynamics to %s", source, destination))); addDependency(destination, source); } diff --git a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/debugging/Naming.java b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/debugging/Naming.java index 575f7688c3..9d00043cf1 100644 --- a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/debugging/Naming.java +++ b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/debugging/Naming.java @@ -28,15 +28,19 @@ private Naming() {} // Use a WeakHashMap so that naming a thing doesn't prevent it from being garbage-collected. private static final WeakHashMap>> NAMES = new WeakHashMap<>(); - private record NamingContext(Set visited, Optional anonymousName) { + private record NamingContext(Set visited, Function> anonymousName) { NamingContext visit(Object thing) { var newVisited = new HashSet<>(visited); newVisited.add(thing); return new NamingContext(newVisited, anonymousName); } + public NamingContext() { + this(Set.of(), $ -> Optional.empty()); + } + public NamingContext(String anonymousName) { - this(Set.of(), Optional.ofNullable(anonymousName)); + this(Set.of(), anonymousName == null ? $ -> Optional.of(Objects.toString($)) : $ -> Optional.of(anonymousName)); } } @@ -52,7 +56,9 @@ public static T name(T thing, String nameFormat, Object... args) { for (int i = 0; i < args$.length; ++i) { var argName$ = Optional.ofNullable(args$[i].get()) .flatMap(argRef -> getName(argRef, context)); - if (argName$.isEmpty()) return context.anonymousName(); + if (argName$.isEmpty()) { + return context.anonymousName.apply(args$[i].get()); + } argNames[i] = argName$.get(); } return Optional.of(nameFormat.formatted(argNames)); @@ -66,9 +72,14 @@ public static T name(T thing, String nameFormat, Object... args) { * returns empty. */ public static Optional getName(Object thing) { - return getName(thing, new NamingContext(null)); + return getName(thing, new NamingContext()); } + // TODO - build a version of getName, or make this the default behavior, (maybe if anonymousName is null?) + // which calls Object.toString on things that don't otherwise have a name. + // This supports the use of literals and dynamics objects in effect names, + // without needing to eagerly compute their names, which involves costly string ops. + /** * Get the name for thing. * Use anonymousName for anything without a name instead of returning empty. @@ -80,8 +91,8 @@ public static String getName(Object thing, String anonymousName) { private static Optional getName(Object thing, NamingContext context) { return context.visited.contains(thing) - ? context.anonymousName - : NAMES.getOrDefault(thing, NamingContext::anonymousName).apply(context.visit(thing)); + ? context.anonymousName.apply(thing) + : NAMES.getOrDefault(thing, ctx -> ctx.anonymousName.apply(thing)).apply(context.visit(thing)); } public static String argsFormat(Collection collection) { diff --git a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/black_box/Approximation.java b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/black_box/Approximation.java index 75197d3001..a89531b4d7 100644 --- a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/black_box/Approximation.java +++ b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/black_box/Approximation.java @@ -46,7 +46,7 @@ public static , E extends Dynamics> Resource a private static , E extends Dynamics> void updateApproximation( ErrorCatching> resourceDynamics, Function, Expiring> approximation, MutableResource result) { var newDynamics = resourceDynamics.map(approximation); - result.emit("Update approximation to " + newDynamics, $ -> newDynamics); + result.emit(name($ -> newDynamics, "Update approximation to %s", newDynamics)); } /** diff --git a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/black_box/UnstructuredResources.java b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/black_box/UnstructuredResources.java index 8b90f68fad..ea643d7c3c 100644 --- a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/black_box/UnstructuredResources.java +++ b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/black_box/UnstructuredResources.java @@ -23,7 +23,8 @@ private UnstructuredResources() {} public static Resource> constant(A value) { var result = UnstructuredResourceApplicative.pure(value); - name(result, value.toString()); + // Use an edge in the naming graph directly to the value to lazily compute the name + name(result, "%s", value); return result; } diff --git a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/discrete/DiscreteEffects.java b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/discrete/DiscreteEffects.java index b62f341180..db47acd64f 100644 --- a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/discrete/DiscreteEffects.java +++ b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/discrete/DiscreteEffects.java @@ -8,6 +8,7 @@ import java.util.Optional; import static gov.nasa.jpl.aerie.contrib.streamline.core.Resources.currentValue; +import static gov.nasa.jpl.aerie.contrib.streamline.debugging.Naming.name; import static gov.nasa.jpl.aerie.contrib.streamline.modeling.discrete.monads.DiscreteDynamicsMonad.effect; public final class DiscreteEffects { @@ -19,7 +20,7 @@ private DiscreteEffects() {} * Set the resource to the given value. */ public static void set(MutableResource> resource, A newValue) { - resource.emit("Set " + newValue, effect(x -> newValue)); + resource.emit(name(effect(x -> newValue), "Set %s", newValue)); } // Flag/Switch style operations @@ -58,7 +59,7 @@ public static void increment(MutableResource> resource) { * Add the given amount to the resource's value. */ public static void increment(MutableResource> resource, int amount) { - resource.emit("Increment by " + amount, effect(x -> x + amount)); + resource.emit(name(effect(x -> x + amount), "Increment by %s", amount)); } /** @@ -72,7 +73,7 @@ public static void decrement(MutableResource> resource) { * Subtract the given amount from the resource's value. */ public static void decrement(MutableResource> resource, int amount) { - resource.emit("Decrement by " + amount, effect(x -> x - amount)); + resource.emit(name(effect(x -> x - amount), "Decrement by %s", amount)); } // General numeric resources @@ -81,14 +82,14 @@ public static void decrement(MutableResource> resource, int am * Add amount to resource's value */ public static void increase(MutableResource> resource, double amount) { - resource.emit("Increase by " + amount, effect(x -> x + amount)); + resource.emit(name(effect(x -> x + amount), "Increase by %s", amount)); } /** * Subtract amount from resource's value */ public static void decrease(MutableResource> resource, double amount) { - resource.emit("Decrease by " + amount, effect(x -> x - amount)); + resource.emit(name(effect(x -> x - amount), "Decrease by %s", amount)); } // Queue style operations, mirroring the Queue interface @@ -97,11 +98,11 @@ public static void decrease(MutableResource> resource, double a * Add element to the end of the queue resource */ public static void add(MutableResource>> resource, T element) { - resource.emit("Add %s to queue".formatted(element), effect(q -> { + resource.emit(name(effect(q -> { var q$ = new LinkedList<>(q); q$.add(element); return q$; - })); + }), "Add %s to queue", element)); } /** @@ -115,14 +116,14 @@ public static Optional remove(MutableResource>> resource if (currentQueue.isEmpty()) return Optional.empty(); final T result = currentQueue.get(currentQueue.size() - 1); - resource.emit("Remove %s from queue".formatted(result), effect(q -> { + resource.emit(name(effect(q -> { var q$ = new LinkedList<>(q); T purportedResult = q$.removeLast(); if (!result.equals(purportedResult)) { throw new IllegalStateException("Detected effect conflicting with queue remove operation"); } return q$; - })); + }), "Remove %s from queue", result)); return Optional.of(result); } @@ -132,14 +133,14 @@ public static Optional remove(MutableResource>> resource * Subtract the given amount from resource. */ public static void consume(MutableResource> resource, double amount) { - resource.emit("Consume " + amount, effect(x -> x - amount)); + resource.emit(name(effect(x -> x - amount), "Consume %s", amount)); } /** * Add the given amount to resource. */ public static void restore(MutableResource> resource, double amount) { - resource.emit("Restore " + amount, effect(x -> x + amount)); + resource.emit(name(effect(x -> x + amount), "Restore %s", amount)); } // Non-consumable style operations diff --git a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/polynomial/PolynomialEffects.java b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/polynomial/PolynomialEffects.java index 45362d221e..366567ea90 100644 --- a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/polynomial/PolynomialEffects.java +++ b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/polynomial/PolynomialEffects.java @@ -6,6 +6,7 @@ import gov.nasa.jpl.aerie.merlin.protocol.types.Duration; import gov.nasa.jpl.aerie.contrib.streamline.unit_aware.UnitAware; +import static gov.nasa.jpl.aerie.contrib.streamline.debugging.Naming.name; import static gov.nasa.jpl.aerie.contrib.streamline.modeling.polynomial.Polynomial.polynomial; import static gov.nasa.jpl.aerie.merlin.framework.ModelActions.delay; import static gov.nasa.jpl.aerie.merlin.framework.ModelActions.replaying; @@ -22,9 +23,8 @@ private PolynomialEffects() {} * Consume some amount of a resource instantaneously. */ public static void consume(MutableResource resource, double amount) { - resource.emit( - "Consume %.1e discretely".formatted(amount), - effect($ -> $.subtract(polynomial(amount)))); + resource.emit(name(effect($ -> $.subtract(polynomial(amount))), + "Consume %s discretely", amount)); } /** @@ -66,9 +66,8 @@ public static void restoring(MutableResource resource, Polynomial pr * Consume some amount of a resource instantaneously. */ public static void restore(MutableResource resource, double amount) { - resource.emit( - "Restore %.1e discretely".formatted(amount), - effect($ -> $.add(polynomial(amount)))); + resource.emit(name(effect($ -> $.add(polynomial(amount))), + "Restore %s discretely", amount)); } /** @@ -93,7 +92,8 @@ public static void restore(MutableResource resource, double rate, Du } private static void withConsumableEffects(String verb, MutableResource resource, Polynomial profile, Runnable action) { - resource.emit("Start %s according to profile %s".formatted(verb, profile), effect($ -> $.subtract(profile))); + resource.emit(name(effect($ -> $.subtract(profile)), + "Start %s according to profile %s", verb, profile)); final Duration start = Resources.currentTime(); action.run(); final Duration elapsedTime = Resources.currentTime().minus(start); @@ -101,7 +101,8 @@ private static void withConsumableEffects(String verb, MutableResource $.add(counteractingProfile))); + resource.emit(name(effect($ -> $.add(counteractingProfile)), + "End %s according to profile %s", verb, profile)); } // Non-consumable style operations @@ -155,13 +156,15 @@ public static void providing(MutableResource resource, double amount } private static void withNonConsumableEffect(String verb, MutableResource resource, Polynomial profile, Runnable action) { - resource.emit("Start %s profile %s".formatted(verb, profile), effect($ -> $.subtract(profile))); + resource.emit(name(effect($ -> $.subtract(profile)), + "Start %s profile %s", verb, profile)); final Duration start = Resources.currentTime(); action.run(); final Duration elapsedTime = Resources.currentTime().minus(start); // Reset by adding a counteracting profile final Polynomial counteractingProfile = profile.step(elapsedTime); - resource.emit("Finish %s profile %s".formatted(verb, profile), effect($ -> $.add(counteractingProfile))); + resource.emit(name(effect($ -> $.add(counteractingProfile)), + "Finish %s profile %s", verb, profile)); } // Consumable style operations From dae9247023c2a9607e36e4539c5b6551ecb0691b Mon Sep 17 00:00:00 2001 From: David Legg Date: Wed, 19 Jun 2024 23:44:26 -0700 Subject: [PATCH 2/5] Performance improvements Adds a few performance improvements suggested by profiling the CADRE simplified data model. In particular, 1. Refraining from adding duplicate constraints to update queue in LBCS, which drives the clampedIntegrate method. 2. Optimizing Expiry.or to use Optional methods rather than converting to Streams. This is used heavily when deriving resource values. 3. Using effectively immutable lists for Context, and sharing references across child tasks. Since *many* daemon threads run in the same context as their parent, this reduces the amount of object creation. --- .../aerie/contrib/streamline/core/Expiry.java | 10 +++- .../contrib/streamline/debugging/Context.java | 33 +++++++---- .../LinearBoundaryConsistencySolver.java | 59 +++++++++++++++---- 3 files changed, 78 insertions(+), 24 deletions(-) diff --git a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/core/Expiry.java b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/core/Expiry.java index f7e245fa16..fbdb6bf1ae 100644 --- a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/core/Expiry.java +++ b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/core/Expiry.java @@ -20,8 +20,14 @@ public static Expiry expiry(Optional value) { } public Expiry or(Expiry other) { - return expiry( - Stream.concat(value().stream(), other.value().stream()).reduce(Duration::min)); + // If this has a value... + // If other has a value, compare and return the minimum + // Else other is NEVER, so return this + // Else (this is NEVER), so return other + return this.value.map(thisValue -> + other.value.map(otherValue -> Expiry.at(Duration.min(thisValue, otherValue))) + .orElse(this)) + .orElse(other); } public Expiry minus(Duration t) { diff --git a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/debugging/Context.java b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/debugging/Context.java index f68f27b05b..2d43ad4312 100644 --- a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/debugging/Context.java +++ b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/debugging/Context.java @@ -2,8 +2,7 @@ import gov.nasa.jpl.aerie.merlin.protocol.types.Unit; -import java.util.ArrayDeque; -import java.util.Deque; +import java.util.ArrayList; import java.util.List; import java.util.function.Supplier; @@ -15,7 +14,7 @@ public final class Context { private Context() {} - private static final ThreadLocal> contexts = ThreadLocal.withInitial(ArrayDeque::new); + private static final ThreadLocal> contexts = ThreadLocal.withInitial(List::of); /** * @see Context#inContext(String, Supplier) @@ -30,13 +29,19 @@ public static void inContext(String contextName, Runnable action) { */ public static R inContext(String contextName, Supplier action) { // Using a thread-local context stack maintains isolation for threaded tasks. + var outerContext = contexts.get(); try { - contexts.get().push(contextName); + // Since copying the context into spawned children actually dominates performance, + // use an effectively-immutable list for the stack, and copy it to push/pop a layer. + var innerContext = new ArrayList(outerContext.size() + 1); + innerContext.addAll(outerContext); + innerContext.add(contextName); + contexts.set(innerContext); return action.get(); // TODO: Should we add a catch clause here that would add context to the error? } finally { // Doing the tear-down in a finally block maintains isolation for replaying tasks. - contexts.get().pop(); + contexts.set(outerContext); } } @@ -61,12 +66,12 @@ public static void inContext(List contextStack, Runnable action) { * @see Context#contextualized */ public static R inContext(List contextStack, Supplier action) { - if (contextStack.isEmpty()) { + var outerContext = contexts.get(); + try { + contexts.set(contextStack); return action.get(); - } else { - int n = contextStack.size() - 1; - return inContext(contextStack.get(n), () -> - inContext(contextStack.subList(0, n), action)); + } finally { + contexts.set(outerContext); } } @@ -133,9 +138,15 @@ public static Supplier contextualized(String childContext, Supplier ac /** * Returns the list of contexts, from innermost context out. + * + *

+ * Note: For efficiency, this returns the actual context stack, not a copy. + * Do not modify the result of this method! + * Doing so may corrupt the context-tracking system. + *

*/ public static List get() { - return contexts.get().stream().toList(); + return contexts.get(); } private static Supplier asSupplier(Runnable action) { diff --git a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/polynomial/LinearBoundaryConsistencySolver.java b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/polynomial/LinearBoundaryConsistencySolver.java index 8eea6498ef..93ad6a1663 100644 --- a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/polynomial/LinearBoundaryConsistencySolver.java +++ b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/polynomial/LinearBoundaryConsistencySolver.java @@ -9,14 +9,7 @@ import gov.nasa.jpl.aerie.contrib.streamline.core.monads.ExpiringMonad; import gov.nasa.jpl.aerie.merlin.framework.Condition; -import java.util.Arrays; -import java.util.HashMap; -import java.util.HashSet; -import java.util.LinkedList; -import java.util.List; -import java.util.Map; -import java.util.Queue; -import java.util.Set; +import java.util.*; import java.util.function.Function; import java.util.stream.Stream; @@ -132,8 +125,11 @@ private void solve() { "LinearBoundaryConsistencySolver %s failed. Domain for %s is empty: [%s, %s]".formatted( getName(this).orElseThrow(), D.variable, D.lowerBound, D.upperBound)); } - // TODO: Make this more efficient by not adding constraints that are already in the queue. - remainingConstraints.addAll(neighboringConstraints.get(D.variable)); + for (DirectionalConstraint constraintToAdd : neighboringConstraints.get(D.variable)) { + if (!remainingConstraints.contains(constraintToAdd)) { + remainingConstraints.add(constraintToAdd); + } + } } } // If that didn't fully solve all variables, choose the first unsolved variable @@ -327,9 +323,50 @@ private Stream directionalConstraints(Variable constraine }, drivingVariables)); } } + // Directional constraints are useful for arc consistency, since they have input (driving) and output (constrained) variables. // However, many directional constraints are required in general to express one General constraint. - private record DirectionalConstraint(Variable constrainedVariable, InequalityComparison comparison, Function, Expiring> bound, Set drivingVariables) {} + private static final class DirectionalConstraint { + private final Variable constrainedVariable; + private final InequalityComparison comparison; + private final Function, Expiring> bound; + private final Set drivingVariables; + + private DirectionalConstraint(Variable constrainedVariable, InequalityComparison comparison, Function, Expiring> bound, Set drivingVariables) { + this.constrainedVariable = constrainedVariable; + this.comparison = comparison; + this.bound = bound; + this.drivingVariables = drivingVariables; + } + + public Variable constrainedVariable() { + return constrainedVariable; + } + + public InequalityComparison comparison() { + return comparison; + } + + public Function, Expiring> bound() { + return bound; + } + + public Set drivingVariables() { + return drivingVariables; + } + + // *Don't* override hashCode and equals - we want to use object identity here instead of value equality. + // This makes it more performant during solving to check whether a constraint is already in the queue to be propagated. + + @Override + public String toString() { + return "DirectionalConstraint[" + + "constrainedVariable=" + constrainedVariable + ", " + + "comparison=" + comparison + ", " + + "bound=" + bound + ", " + + "drivingVariables=" + drivingVariables + ']'; + } + } public static final class Domain { public final Variable variable; From 7fc3ca8bbcd244f610165acacb7fd52ff3bd0977 Mon Sep 17 00:00:00 2001 From: David Legg Date: Thu, 20 Jun 2024 14:31:10 -0700 Subject: [PATCH 3/5] Optimize clampedIntegrate Rewrites PolynomialResources.clampedIntegrate to not use the LinearBoundaryConsistencySolver. Profiling on the CADRE mission model suggests that clampedIntegrate was a primary bottleneck in time performance. To address this, I'm applying the lessons learned from writing the LinearBoundaryConsistencySolver, mainly the importance of using `dominates` as the basis of polynomial comparisons, directly to clampedIntegrate. Using this, I was able to rewrite the solver constraints as a non-iterative procedure, which queries the inputs exactly once each time the integral needs updating and limits the conditions on which we recompute the integral. --- .../streamline/debugging/Profiling.java | 4 +- .../modeling/polynomial/Polynomial.java | 18 +- .../polynomial/PolynomialResources.java | 165 +++++++++++++----- 3 files changed, 138 insertions(+), 49 deletions(-) diff --git a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/debugging/Profiling.java b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/debugging/Profiling.java index 78ee47592b..180c7a2920 100644 --- a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/debugging/Profiling.java +++ b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/debugging/Profiling.java @@ -152,7 +152,7 @@ public static > MutableResource profileEffects(Mutab MutableResource result = new MutableResource<>() { @Override public void emit(DynamicsEffect effect) { - resource.emit(x -> accrue(effectsEmitted, getName(this, "..."), () -> effect.apply(x))); + resource.emit(x -> accrue(effectsEmitted, getName(this, null), () -> effect.apply(x))); } @Override @@ -167,7 +167,7 @@ public ErrorCatching> getDynamics() { private static Supplier computeName(String explicitName, Object profiledThing) { return explicitName != null ? () -> explicitName - : () -> getName(profiledThing, "..."); + : () -> getName(profiledThing, null); } private static long ANONYMOUS_ID = 0; diff --git a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/polynomial/Polynomial.java b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/polynomial/Polynomial.java index bc7be13927..6390a5c531 100644 --- a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/polynomial/Polynomial.java +++ b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/polynomial/Polynomial.java @@ -229,7 +229,18 @@ public Expiring> lessThanOrEquals(Polynomial other) { return other.greaterThanOrEquals(this); } - private boolean dominates$(Polynomial other) { + /** + * Computes if this polynomial "dominates" another right now. + *

+ * A polynomial P dominates Q in this context if it is greater than Q in the first coefficient that differs, + * or equal to Q exactly. + *

+ *

+ * Intuitively, P dominates Q when max(P, Q) = P, potentially with a shorter expiry if Q crosses P sometime in the future. + * Consider using {@link Polynomial#dominates(Polynomial)} instead to get this expiry / crossover time as well. + *

+ */ + public boolean dominates$(Polynomial other) { for (int i = 0; i <= Math.max(this.degree(), other.degree()); ++i) { if (this.getCoefficient(i) > other.getCoefficient(i)) return true; if (this.getCoefficient(i) < other.getCoefficient(i)) return false; @@ -238,7 +249,10 @@ public Expiring> lessThanOrEquals(Polynomial other) { return true; } - private Expiring> dominates(Polynomial other) { + /** + * Returns a boolean dynamics, describing the value of {@link Polynomial#dominates$(Polynomial)} and when it will change. + */ + public Expiring> dominates(Polynomial other) { boolean result = this.dominates$(other); var expiry = this.subtract(other).findExpiryNearRoot(t -> this.step(t).dominates$(other.step(t)) != result); return expiring(discrete(result), expiry); diff --git a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/polynomial/PolynomialResources.java b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/polynomial/PolynomialResources.java index 67aa0c7032..fd7cb4231c 100644 --- a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/polynomial/PolynomialResources.java +++ b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/polynomial/PolynomialResources.java @@ -1,9 +1,7 @@ package gov.nasa.jpl.aerie.contrib.streamline.modeling.polynomial; +import gov.nasa.jpl.aerie.contrib.streamline.core.*; import gov.nasa.jpl.aerie.contrib.streamline.core.CellRefV2.CommutativityTestInput; -import gov.nasa.jpl.aerie.contrib.streamline.core.MutableResource; -import gov.nasa.jpl.aerie.contrib.streamline.core.Expiring; -import gov.nasa.jpl.aerie.contrib.streamline.core.Resource; import gov.nasa.jpl.aerie.contrib.streamline.core.monads.DynamicsMonad; import gov.nasa.jpl.aerie.contrib.streamline.debugging.Naming; import gov.nasa.jpl.aerie.contrib.streamline.modeling.black_box.*; @@ -18,11 +16,14 @@ import gov.nasa.jpl.aerie.contrib.streamline.unit_aware.UnitAwareOperations; import gov.nasa.jpl.aerie.contrib.streamline.unit_aware.UnitAwareResources; import gov.nasa.jpl.aerie.contrib.streamline.utils.DoubleUtils; +import gov.nasa.jpl.aerie.merlin.framework.Condition; import gov.nasa.jpl.aerie.merlin.protocol.types.Duration; +import org.apache.commons.lang3.mutable.MutableObject; import java.time.Instant; import java.time.temporal.ChronoUnit; import java.util.NavigableMap; +import java.util.Optional; import java.util.TreeMap; import java.util.stream.IntStream; import java.util.stream.Stream; @@ -33,6 +34,7 @@ import static gov.nasa.jpl.aerie.contrib.streamline.core.Expiring.expiring; import static gov.nasa.jpl.aerie.contrib.streamline.core.Expiring.neverExpiring; import static gov.nasa.jpl.aerie.contrib.streamline.core.Expiry.NEVER; +import static gov.nasa.jpl.aerie.contrib.streamline.core.MutableResource.set; import static gov.nasa.jpl.aerie.contrib.streamline.core.Reactions.wheneverDynamicsChange; import static gov.nasa.jpl.aerie.contrib.streamline.core.Resources.*; import static gov.nasa.jpl.aerie.contrib.streamline.core.monads.DynamicsMonad.bindEffect; @@ -359,50 +361,123 @@ public static Resource integrate(Resource integrand, dou * 0 / \----- * time 0 10 20 * + *

+ * NOTE: This method assumes that lowerBound <= upperBound at all times. + * Failure to meet this precondition may incorrect outputs, crashing the simulation, stalling in an infinite loop, + * or other misbehavior. + * If this condition cannot be guaranteed a priori, consider using + * {@link gov.nasa.jpl.aerie.contrib.streamline.modeling.discrete.DiscreteResources#assertThat(String, Resource)} + * to guarantee this condition at runtime. + *

*/ public static ClampedIntegrateResult clampedIntegrate( - Resource integrand, Resource lowerBound, Resource upperBound, double startingValue) { - LinearBoundaryConsistencySolver rateSolver = new LinearBoundaryConsistencySolver("clampedIntegrate rate solver"); - var integral = resource(polynomial(startingValue)); - var neverExpiringIntegral = eraseExpiry(integral); - - // Solve for the rate as a function of value - var overflowRate = rateSolver.variable("overflowRate", Domain::lowerBound); - var underflowRate = rateSolver.variable("underflowRate", Domain::lowerBound); - var rate = rateSolver.variable("rate", Domain::upperBound); - - // Set up slack variables for under-/overflow - rateSolver.declare(lx(overflowRate), GreaterThanOrEquals, lx(0)); - rateSolver.declare(lx(underflowRate), GreaterThanOrEquals, lx(0)); - rateSolver.declare(lx(rate).subtract(lx(underflowRate)).add(lx(overflowRate)), Equals, lx(integrand)); - - // Set up rate clamping conditions - var integrandUB = choose( - greaterThanOrEquals(neverExpiringIntegral, upperBound), - differentiate(upperBound), - constant(Double.POSITIVE_INFINITY)); - var integrandLB = choose( - lessThanOrEquals(neverExpiringIntegral, lowerBound), - differentiate(lowerBound), - constant(Double.NEGATIVE_INFINITY)); - - rateSolver.declare(lx(rate), LessThanOrEquals, lx(integrandUB)); - rateSolver.declare(lx(rate), GreaterThanOrEquals, lx(integrandLB)); - - // Use a simple feedback loop on volumes to do the integration and clamping. - // Clamping here takes care of discrete under-/overflows and overshooting bounds due to discrete time steps. - var clampedCell = clamp(neverExpiringIntegral, lowerBound, upperBound); - var correctedCell = map(clampedCell, rate.resource(), (v, r) -> r.integral(v.extract())); - // Use the corrected integral values to set volumes, but erase expiry information in the process to avoid loops - forward(correctedCell, integral); - - name(integral, "Clamped Integral (%s)", integrand); - name(overflowRate.resource(), "Overflow of %s", integral); - name(underflowRate.resource(), "Underflow of %s", integral); - return new ClampedIntegrateResult( - integral, - overflowRate.resource(), - underflowRate.resource()); + Resource integrand, Resource lowerBound, Resource upperBound, double startingValue) { + // There are more elegant ways to write this method, but profiling suggests this is often a bottleneck to models that use it. + // So, we're writing it very carefully to maximize performance. + + MutableResource integral = polynomialResource(startingValue); + MutableResource overflow = polynomialResource(0); + MutableResource underflow = polynomialResource(0); + + MutableObject condition = new MutableObject<>(Condition.TRUE); // Run once immediately to get the loop started. + Reactions.whenever(condition::getValue, () -> { + // Note we need to call dynamicsChange on each iteration because it depends on the resource's current value, + // so *don't* factor these out of the loop. + Condition someInputChanges = dynamicsChange(integrand) + .or(dynamicsChange(lowerBound)) + .or(dynamicsChange(upperBound)); + + // Get all dynamics once per update loop to minimize sampling cost. + ErrorCatching> result = DynamicsMonad.map( + integrand.getDynamics(), + lowerBound.getDynamics(), + upperBound.getDynamics(), + (integrandDynamics$, lowerBoundDynamics$, upperBoundDynamics$) -> { + // Clamp the integral value to take care of small overshoots due to the discretization of time + // and discrete changes in bounds that cut into the integral. + // Also, just get the current value, don't try to derive a value. + // This intentionally erases expiry info from the integral since this is a resource-graph back-edge, + // and will throw and blow up this resource if integral fails. + var integralValue = Double.max( + Double.min(currentValue(integral), upperBoundDynamics$.extract()), + lowerBoundDynamics$.extract()); + var integralDynamics$ = integrandDynamics$.integral(integralValue); + + if (lowerBoundDynamics$.dominates$(integralDynamics$)) { + // Lower bound dominates "real" integral, so we clamp to the lower bound + // We stop clamping to the lower bound when the integrand rate exceeds the lowerBound rate. + // Since we already know that lowerBound dominates integral and integral value is at least lower value, + // we know that lower rate dominates integrand. Hence, there's no need to check that value here. + var lowerRate = lowerBoundDynamics$.derivative(); + var clampingStops = fixedTimeCondition(lowerRate.dominates(integrandDynamics$).expiry().value()); + // We change out of this condition when we stop clamping to the lower bound, or something changes. + return new ClampedIntegrateInternalResult( + lowerBoundDynamics$, + polynomial(0), + lowerRate.subtract(integrandDynamics$), + clampingStops.or(someInputChanges)); + } + + if (!upperBoundDynamics$.dominates$(integralDynamics$)) { + // Upper bound doesn't dominate integral, so we clamp to the upper bound + // We stop clamping to the upper bound when the integrand rate falls below the upperBound rate. + // Since we already know that lowerBound dominates integral and integral value is at least lower value, + // we know that lower rate dominates integrand. Hence, there's no need to check that value here. + var upperRate = upperBoundDynamics$.derivative(); + var clampingStops = fixedTimeCondition(upperRate.dominates(integrandDynamics$).expiry().value()); + // We change out of this condition when we stop clamping to the upper bound, or something changes. + return new ClampedIntegrateInternalResult( + upperBoundDynamics$, + integrandDynamics$.subtract(upperRate), + polynomial(0), + clampingStops.or(someInputChanges)); + } + + // Otherwise, the integral is between the bounds, so we just set it as-is. + // We start clamping when one or the other bound impacts this integral, i.e., when a dominates value changes. + // Although we re-compute the value of dominates$ here, that's cheap. + // We avoid computing the more expensive expiry when we're clamping, which is a win overall. + var startClampingToLowerBound = lowerBoundDynamics$.dominates(integralDynamics$).expiry(); + var startClampingToUpperBound = upperBoundDynamics$.dominates(integralDynamics$).expiry(); + var clampingStarts = fixedTimeCondition(startClampingToLowerBound.or(startClampingToUpperBound).value()); + return new ClampedIntegrateInternalResult( + integralDynamics$, + polynomial(0), + polynomial(0), + clampingStarts.or(someInputChanges) + ); + }); + + var newIntegralDynamics = DynamicsMonad.map(result, ClampedIntegrateInternalResult::integral); + var newOverflowDynamics = DynamicsMonad.map(result, ClampedIntegrateInternalResult::overflow); + var newUnderflowDynamics = DynamicsMonad.map(result, ClampedIntegrateInternalResult::underflow); + // If there was an error, that disturbs the integral value, and we can't recover. Don't bother retrying in that case. + var newRetryCondition = result.match($ -> $.data().retryCondition(), $ -> Condition.FALSE); + + integral.emit("Update clamped integral", $ -> newIntegralDynamics); + overflow.emit("Update clamped integral overflow", $ -> newOverflowDynamics); + underflow.emit("Update clamped integral underflow", $ -> newUnderflowDynamics); + condition.setValue(newRetryCondition); + }); + + return new ClampedIntegrateResult(integral, overflow, underflow); + } + + private static Condition fixedTimeCondition(Optional time) { + return time.map(time$ -> (positive, atEarliest, atLatest) -> { + if (positive) { + return time.filter(atLatest::noShorterThan).map(t -> Duration.max(atEarliest, t)); + } else { + return Optional.of(atEarliest).filter(time$::longerThan); + } + }).orElse(Condition.FALSE); + } + + private record ClampedIntegrateInternalResult( + Polynomial integral, + Polynomial overflow, + Polynomial underflow, + Condition retryCondition) { } /** From 6b624d7255a5711068d666531e262a6c45667b63 Mon Sep 17 00:00:00 2001 From: David Legg Date: Tue, 25 Jun 2024 10:34:36 -0700 Subject: [PATCH 4/5] Use full names in cell effect errors --- .../gov/nasa/jpl/aerie/contrib/streamline/core/CellRefV2.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/core/CellRefV2.java b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/core/CellRefV2.java index 294736ea97..2324dcf00b 100644 --- a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/core/CellRefV2.java +++ b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/core/CellRefV2.java @@ -40,7 +40,7 @@ public void apply(Cell cell, E effect) { cell.initialDynamics = effect.apply(cell.dynamics).match( ErrorCatching::success, error -> failure(new RuntimeException( - "Applying effect '%s' failed.".formatted(getName(effect, "...")), error))); + "Applying effect '%s' failed.".formatted(getName(effect, null)), error))); cell.dynamics = cell.initialDynamics; cell.elapsedTime = ZERO; } From 5255f99682cf239cccaa2c60fe2e16953ffaa9b4 Mon Sep 17 00:00:00 2001 From: DavidLegg Date: Tue, 25 Jun 2024 11:16:32 -0700 Subject: [PATCH 5/5] Fix typo in PolynomialResources Co-authored-by: Matt Dailis --- .../streamline/modeling/polynomial/PolynomialResources.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/polynomial/PolynomialResources.java b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/polynomial/PolynomialResources.java index fd7cb4231c..7378d97a4b 100644 --- a/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/polynomial/PolynomialResources.java +++ b/contrib/src/main/java/gov/nasa/jpl/aerie/contrib/streamline/modeling/polynomial/PolynomialResources.java @@ -363,7 +363,7 @@ public static Resource integrate(Resource integrand, dou * *

* NOTE: This method assumes that lowerBound <= upperBound at all times. - * Failure to meet this precondition may incorrect outputs, crashing the simulation, stalling in an infinite loop, + * Failure to meet this precondition may lead to incorrect outputs, crashing the simulation, stalling in an infinite loop, * or other misbehavior. * If this condition cannot be guaranteed a priori, consider using * {@link gov.nasa.jpl.aerie.contrib.streamline.modeling.discrete.DiscreteResources#assertThat(String, Resource)}