Merge pull request opentripplanner#7440 from entur/fix/mc-pareto-comparator-regression

Fix MC Raptor Pareto comparator performance regression

Author: t2gran

application/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/mappers/RaptorRequestMapper.java

@@ -180,7 +180,7 @@ public RaptorRequest<T> mapRaptorRequest() {
 
     if (raptorDebugging.isEnabled()) {
       var debug = builder.debug();
-      var debugLogger = new SystemErrDebugLogger(true, false);
+      var debugLogger = new SystemErrDebugLogger(null, true);
 
       debug
         .withStops(mapDebugStopList(raptorDebugging.stops()))

raptor/src/main/java/org/opentripplanner/raptor/api/view/ArrivalView.java

@@ -140,11 +140,6 @@ default RaptorTransfer transfer() {
 
   /* Egress */
 
-  /** @return true if destination arrival, otherwise false. */
-  default boolean arrivedAtDestination() {
-    return false;
-  }
-
   default EgressPathView egressPath() {
     throw new UnsupportedOperationException();
   }

raptor/src/main/java/org/opentripplanner/raptor/rangeraptor/RangeRaptor.java

@@ -40,7 +40,8 @@
  *     (generating randomized schedules)
  * </ul>
  * <p>
- * This class originated as a rewrite of Conveyals RAPTOR code: https://github.com/conveyal/r5.
+ * This class originated as a rewrite of
+ * <a href="https://github.com/conveyal/r5">Conveyals RAPTOR code</a>.
  *
  * @param <T> The TripSchedule type defined by the user of the raptor API.
  */
@@ -84,7 +85,7 @@ public RangeRaptor(
     this.calculator = requireNonNull(calculator);
     this.timers = requireNonNull(timers);
     this.accessPaths = requireNonNull(accessPaths);
-    this.minNumberOfRounds = accessPaths.calculateMaxNumberOfRides();
+    this.minNumberOfRounds = accessPaths.maxNumberOfRides();
     this.roundTracker = requireNonNull(roundTracker);
     this.lifeCycle = requireNonNull(lifeCyclePublisher);
     this.timeoutHook = requireNonNull(timeoutHook);

raptor/src/main/java/org/opentripplanner/raptor/rangeraptor/SystemErrDebugLogger.java

@@ -21,7 +21,7 @@
 import org.opentripplanner.raptor.api.view.ArrivalView;
 import org.opentripplanner.raptor.api.view.PatternRideView;
 import org.opentripplanner.raptor.rangeraptor.transit.TripTimesSearch;
-import org.opentripplanner.utils.lang.IntUtils;
+import org.opentripplanner.raptor.spi.RaptorStopNameResolver;
 import org.opentripplanner.utils.lang.OtpNumberFormat;
 import org.opentripplanner.utils.lang.StringUtils;
 import org.opentripplanner.utils.text.Table;
@@ -38,32 +38,29 @@ public class SystemErrDebugLogger implements DebugLogger {
   private static final int NOT_SET = Integer.MIN_VALUE;
 
   private final boolean enableDebugLogging;
-  private final boolean eventLoggingDryRun;
+  private final RaptorStopNameResolver stopNameResolver;
   private final NumberFormat numFormat = NumberFormat.getInstance(Locale.FRANCE);
   private final Table arrivalTable = Table.of()
     .withAlights(Center, Center, Right, Right, Right, Right, Left, Left)
     .withHeaders("ARRIVAL", "LEG", "RND", "STOP", "ARRIVE", "C₁", "TRIP", "DETAILS")
-    .withMinWidths(9, 7, 3, 5, 8, 9, 24, 0)
+    .withMinWidths(9, 7, 3, 5, 11, 9, 24, 0)
     .build();
   private final Table pathTable = Table.of()
     .withAlights(Center, Center, Right, Right, Right, Right, Right, Right, Left)
     .withHeaders(">>> PATH", "TR", "FROM", "TO", "START", "END", "DURATION", "C₁", "DETAILS")
-    .withMinWidths(9, 2, 5, 5, 8, 8, 8, 9, 0)
+    .withMinWidths(9, 2, 5, 5, 11, 11, 8, 9, 0)
     .build();
   private boolean forwardSearch = true;
   private int lastIterationTime = NOT_SET;
   private int lastRound = NOT_SET;
   private boolean printPathHeader = true;
 
   /**
-   * @param enableDebugLogging Log debug information on {@link DebugTopic}s.
-   * @param eventLoggingDryRun DryRun will do the Raptor event logging, except printing the result.
-   *                           This is used to test the logging framework without logging. To turn
-   *                           off logging completely used the {@link #noop()} logger.
+   * @param enableDebugLogging Enable debug information on {@link DebugTopic}s.
    */
-  public SystemErrDebugLogger(boolean enableDebugLogging, boolean eventLoggingDryRun) {
+  public SystemErrDebugLogger(RaptorStopNameResolver stopNameResolver, boolean enableDebugLogging) {
+    this.stopNameResolver = stopNameResolver;
     this.enableDebugLogging = enableDebugLogging;
-    this.eventLoggingDryRun = eventLoggingDryRun;
   }
 
   /**
@@ -138,7 +135,7 @@ public void setSearchDirection(boolean forward) {
 
   @Override
   public void debug(DebugTopic topic, String message) {
-    if (enableDebugLogging) {
+    if (isEnabled()) {
       // We log to info - since debugging is controlled by the application
       if (message.contains("\n")) {
         System.err.printf("%s\n%s", topic, message);
@@ -176,7 +173,7 @@ private static boolean isNotBlank(String text) {
   }
 
   private void printIterationHeader(int iterationTime) {
-    if (iterationTime == lastIterationTime) {
+    if (!isEnabled() || iterationTime == lastIterationTime) {
       return;
     }
     lastIterationTime = iterationTime;
@@ -193,7 +190,7 @@ private void print(ArrivalView<?> a, String action, String optReason) {
         action,
         legType(a),
         a.round(),
-        IntUtils.intToString(a.stop(), NOT_SET),
+        stop(a.stop()),
         timeToStrLong(a.arrivalTime()),
         numFormat.format(a.c1()),
         pattern,
@@ -208,7 +205,7 @@ private void print(PatternRideView<?, ?> p, String action) {
         action,
         "OnRide",
         p.prevArrival().round() + 1,
-        p.boardStopIndex(),
+        stop(p.boardStopIndex()),
         timeToStrLong(p.boardTime()),
         numFormat.format(p.relativeC1()),
         p.trip().pattern().debugInfo(),
@@ -218,7 +215,7 @@ private void print(PatternRideView<?, ?> p, String action) {
   }
 
   private String path(ArrivalView<?> a) {
-    return path(a, new PathStringBuilder(null)).summary(a.c1(), a.c2()).toString();
+    return path(a, new PathStringBuilder(stopNameResolver)).summary(a.c1(), a.c2()).toString();
   }
 
   private PathStringBuilder path(ArrivalView<?> a, PathStringBuilder buf) {
@@ -250,7 +247,7 @@ private PathStringBuilder path(ArrivalView<?> a, PathStringBuilder buf) {
   }
 
   private void printRoundHeader(int round) {
-    if (round == lastRound) {
+    if (!isEnabled() || round == lastRound) {
       return;
     }
     lastRound = round;
@@ -269,14 +266,19 @@ private String legType(ArrivalView<?> a) {
   }
 
   private void println() {
-    if (!eventLoggingDryRun) {
-      System.err.println();
-    }
+    System.err.println();
   }
 
   private void println(String text) {
-    if (!eventLoggingDryRun) {
-      System.err.println(text);
+    System.err.println(text);
+  }
+
+  String stop(int stopIndex) {
+    if (stopIndex == NOT_SET) {
+      return "NA";
     }
+    return stopNameResolver == null
+      ? Integer.toString(stopIndex)
+      : stopNameResolver.apply(stopIndex);
   }
 }

raptor/src/main/java/org/opentripplanner/raptor/rangeraptor/context/SearchContextViaSegments.java

@@ -27,6 +27,7 @@
  * following segment. This is implemented using Pareto set event listeners.
  * </p>
  */
+@SuppressWarnings("ClassCanBeRecord")
 public class SearchContextViaSegments<T extends RaptorTripSchedule> {
 
   private final SearchContext<T> parent;
@@ -76,7 +77,6 @@ public ViaConnections viaConnections() {
   /**
    * Returns the set of egress paths that lead directly to the destination in this segment.
    */
-  @Nullable
   public EgressPaths egressPaths() {
     return egressPaths;
   }

raptor/src/main/java/org/opentripplanner/raptor/rangeraptor/multicriteria/arrivals/ArrivalParetoSetComparatorFactory.java

@@ -11,7 +11,7 @@ public interface ArrivalParetoSetComparatorFactory<T extends McStopArrival<?>> {
    * {@code paretoRound} and {@code c1} to compare arrivals. It does NOT include
    * {@code arrivedOnBoard}. Normally arriving on-board should give the arrival an advantage
    * - you can continue on foot, walking to the next stop. But, we only do this if it happens
-   * in the same Raptor iteration and round - if it does it is taken care of by the order
+   * in the same Raptor iteration and round - if it does, it is taken care of by the order
    * which the algorithm works - not by this comparator.
    */
   ParetoComparator<T> compareArrivalTimeRoundAndCost();
@@ -40,7 +40,7 @@ static <T extends McStopArrival<?>> ArrivalParetoSetComparatorFactory<T> factory
   private static <T extends McStopArrival<?>> ArrivalParetoSetComparatorFactory<
     T
   > createFactoryC1() {
-    return new ArrivalParetoSetComparatorFactory<T>() {
+    return new ArrivalParetoSetComparatorFactory<>() {
       @Override
       public ParetoComparator<T> compareArrivalTimeRoundAndCost() {
         return McStopArrival::compareBase;
@@ -57,7 +57,7 @@ public ParetoComparator<T> compareArrivalTimeRoundCostAndOnBoardArrival() {
   private static <T extends McStopArrival<?>> ArrivalParetoSetComparatorFactory<
     T
   > createFactoryC1AndC2(DominanceFunction c2DominanceFunction) {
-    return new ArrivalParetoSetComparatorFactory<T>() {
+    return new ArrivalParetoSetComparatorFactory<>() {
       @Override
       public ParetoComparator<T> compareArrivalTimeRoundAndCost() {
         return (l, r) ->

…rrivals/ArrivalsEventListenerMapper.java …vals/McArrivalsEventListenerFactory.javaraptor/src/main/java/org/opentripplanner/raptor/rangeraptor/multicriteria/arrivals/ArrivalsEventListenerMapper.java renamed to raptor/src/main/java/org/opentripplanner/raptor/rangeraptor/multicriteria/arrivals/McArrivalsEventListenerFactory.java raptor/src/main/java/org/opentripplanner/raptor/rangeraptor/multicriteria/arrivals/ArrivalsEventListenerMapper.java renamed to raptor/src/main/java/org/opentripplanner/raptor/rangeraptor/multicriteria/arrivals/McArrivalsEventListenerFactory.java

@@ -2,8 +2,9 @@
 
 import gnu.trove.map.TIntObjectMap;
 import gnu.trove.map.hash.TIntObjectHashMap;
+import gnu.trove.set.TIntSet;
+import gnu.trove.set.hash.TIntHashSet;
 import java.util.List;
-import javax.annotation.Nullable;
 import org.opentripplanner.raptor.api.model.RaptorAccessEgress;
 import org.opentripplanner.raptor.api.view.ArrivalView;
 import org.opentripplanner.raptor.rangeraptor.debug.DebugHandlerFactory;
@@ -15,77 +16,82 @@
 
 /// This class creates {@link ParetoSetEventListener}s to glue a stop-arrival to (in order):
 /// - debug stop arrival events
-/// - updating next connection event
+/// - updating next via connection event
 /// - updating destination path collection using egress paths.
 ///
 /// If the debugger is not done first, then the logging will be strange -> events
 /// arriving at the destination, before arriving at the egress stop.
-public class ArrivalsEventListenerMapper<T extends RaptorTripSchedule> {
+public class McArrivalsEventListenerFactory<T extends RaptorTripSchedule> {
 
   private final DebugHandlerFactory<T> debugHandlerFactory;
   private final TIntObjectMap<ParetoSetEventListener<ArrivalView<T>>> nextConnectionListener;
-  private final @Nullable EgressPaths egressPaths;
-  private final DestinationArrivalPaths<T> paths;
+  private final EgressPaths egressPaths;
+  private final DestinationArrivalPaths<T> destinationPaths;
+  private boolean processed = false;
 
-  private final TIntObjectMap<ParetoSetEventListener<ArrivalView<T>>> result =
+  private final TIntObjectMap<ParetoSetEventListener<ArrivalView<T>>> arrivalListeners =
     new TIntObjectHashMap<>();
 
-  private ArrivalsEventListenerMapper(
+  public McArrivalsEventListenerFactory(
     DebugHandlerFactory<T> debugHandlerFactory,
     TIntObjectMap<ParetoSetEventListener<ArrivalView<T>>> nextConnectionListener,
-    @Nullable EgressPaths egressPaths,
-    DestinationArrivalPaths<T> paths
+    EgressPaths egressPaths,
+    DestinationArrivalPaths<T> destinationPaths
   ) {
     this.debugHandlerFactory = debugHandlerFactory;
     this.nextConnectionListener = nextConnectionListener;
     this.egressPaths = egressPaths;
-    this.paths = paths;
+    this.destinationPaths = destinationPaths;
   }
 
-  public static <T extends RaptorTripSchedule> TIntObjectMap<
-    ParetoSetEventListener<ArrivalView<T>>
-  > map(
-    DebugHandlerFactory<T> debugHandlerFactory,
-    TIntObjectMap<ParetoSetEventListener<ArrivalView<T>>> connectionListeners,
-    EgressPaths egressPaths,
-    DestinationArrivalPaths<T> destinationPaths
-  ) {
-    return new ArrivalsEventListenerMapper<>(
-      debugHandlerFactory,
-      connectionListeners,
-      egressPaths,
-      destinationPaths
-    ).map();
+  public TIntObjectMap<ParetoSetEventListener<ArrivalView<T>>> arrivalListeners() {
+    return assertProcessed(arrivalListeners);
   }
 
   /**
    * This method creates a ParetoSet for the given egress stop. When arrivals are added to the stop,
    * the "glue" make sure new destination arrivals are added to the destination arrivals.
    */
-  private TIntObjectMap<ParetoSetEventListener<ArrivalView<T>>> map() {
-    for (int stop : nextConnectionListener.keys()) {
-      append(stop, nextConnectionListener.get(stop));
-    }
+  public McArrivalsEventListenerFactory<T> create() {
     var egressByStop = egressPaths.byStop();
 
-    for (int stop : egressByStop.keys()) {
-      List<RaptorAccessEgress> egressList = egressByStop.get(stop);
-      append(stop, new CalculateTransferToDestination<>(egressList, paths));
+    TIntSet stops = new TIntHashSet();
+    stops.addAll(nextConnectionListener.keySet());
+    stops.addAll(egressByStop.keySet());
+
+    var it = stops.iterator();
+    while (it.hasNext()) {
+      createListenerForStop(it.next(), egressByStop);
     }
-    return result;
+
+    this.processed = true;
+    return this;
+  }
+
+  private void createListenerForStop(
+    int stop,
+    TIntObjectMap<List<RaptorAccessEgress>> egressByStop
+  ) {
+    var l = ParetoSetEventListenerComposite.of(
+      debugHandlerFactory.paretoSetStopArrivalListener(stop),
+      nextConnectionListener.get(stop),
+      egressStopListeners(egressByStop.get(stop))
+    );
+    this.arrivalListeners.put(stop, l);
   }
 
-  private void append(int stop, ParetoSetEventListener<ArrivalView<T>> listener) {
-    var e = result.get(stop);
-    var l = listener;
-    if (e != null) {
-      // existing listeners should be inserted before new one
-      l = ParetoSetEventListenerComposite.of(e, listener);
-    } else if (debugHandlerFactory.isDebugStopArrival(stop)) {
-      // Debug listerner is inserted first
-      var debug = debugHandlerFactory.paretoSetStopArrivalListener(stop);
-      l = ParetoSetEventListenerComposite.of(debug, l);
+  private ParetoSetEventListener<ArrivalView<T>> egressStopListeners(
+    List<RaptorAccessEgress> egressList
+  ) {
+    return egressList == null
+      ? null
+      : new CalculateTransferToDestination<>(egressList, destinationPaths);
+  }
+
+  private <R> R assertProcessed(R result) {
+    if (!processed) {
+      throw new IllegalStateException("Call the create() method before accessing the result!");
     }
-    result.put(stop, l);
+    return result;
   }
 }

raptor/src/main/java/org/opentripplanner/raptor/rangeraptor/multicriteria/arrivals/McStopArrivals.java

@@ -3,6 +3,7 @@
 import static org.opentripplanner.raptor.api.view.PathLegType.TRANSIT;
 
 import gnu.trove.map.TIntObjectMap;
+import gnu.trove.set.TIntSet;
 import java.util.BitSet;
 import java.util.Collections;
 import java.util.function.Function;
@@ -32,28 +33,36 @@ public final class McStopArrivals<T extends RaptorTripSchedule> {
   private final DebugStopArrivalsStatistics debugStats;
   private final ParetoComparator<McStopArrival<T>> comparator;
 
-  /**
-   * Set the time at a transit index if it is optimal. This sets both the best time and the
-   * transfer time.
-   *
-   * @param nextLegArrivals When chaining two Raptor searches together, the next-leg is the next
-   *                search we are copying state into.
-   */
   public McStopArrivals(
     int nStops,
+    TIntSet onBoardArrivalStops,
     TIntObjectMap<ParetoSetEventListener<ArrivalView<T>>> arrivalListeners,
     ArrivalParetoSetComparatorFactory<McStopArrival<T>> comparatorFactory,
     DebugHandlerFactory<T> debugHandlerFactory
   ) {
     //noinspection unchecked
     this.arrivals = (ParetoSet<McStopArrival<T>>[]) new ParetoSet[nStops];
     this.touchedStops = new BitSet(nStops);
-    this.comparator = comparatorFactory.compareArrivalTimeRoundCostAndOnBoardArrival();
+    this.comparator = comparatorFactory.compareArrivalTimeRoundAndCost();
     this.debugHandlerFactory = debugHandlerFactory;
     this.debugStats = new DebugStopArrivalsStatistics(debugHandlerFactory.debugLogger());
 
+    // Comparator for stops that have arrival listeners with a on-board trip-to-trip via
+    // connection.
+    var onBoardComparator = comparatorFactory.compareArrivalTimeRoundCostAndOnBoardArrival();
     for (int stop : arrivalListeners.keys()) {
-      this.arrivals[stop] = ParetoSet.of(comparator, arrivalListeners.get(stop));
+      var comp = onBoardArrivalStops.contains(stop) ? onBoardComparator : comparator;
+      this.arrivals[stop] = ParetoSet.of(comp, arrivalListeners.get(stop));
+    }
+
+    for (var it = onBoardArrivalStops.iterator(); it.hasNext(); ) {
+      int stop = it.next();
+      if (this.arrivals[stop] == null) {
+        this.arrivals[stop] = ParetoSet.of(
+          onBoardComparator,
+          debugHandlerFactory.paretoSetStopArrivalListener(stop)
+        );
+      }
     }
   }