Refactor matrix_action

CHANGELOG.md

@@ -80,6 +80,7 @@
    * UPDATED: tz to 2023d [#4519](https://github.com/valhalla/valhalla/pull/4519)
    * FIXED: Fix segfault in costmatrix (date_time and time zone always added). [#4530](https://github.com/valhalla/valhalla/pull/4530)
    * CHANGED: libvalhalla.pc generation to have finer controls; install third_party public headers; overhaul lots of CMake; remove conan support [#4516](https://github.com/valhalla/valhalla/pull/4516)
+   * CHANGED: refactored matrix code to include a base class for all matrix algorithms to prepare for second passes on matrix [#4535](https://github.com/valhalla/valhalla/pull/4535)
 
 ## Release Date: 2023-05-11 Valhalla 3.4.0
 * **Removed**

bench/thor/costmatrix.cc

@@ -102,7 +102,7 @@ static void BM_UtrechtCostMatrix(benchmark::State& state) {
   thor::CostMatrix matrix;
   for (auto _ : state) {
     matrix.SourceToTarget(request, reader, costs, mode, 100000.);
-    matrix.clear();
+    matrix.Clear();
     request.clear_matrix();
   }
   state.counters["Routes"] = benchmark::Counter(size, benchmark::Counter::kIsIterationInvariantRate);

src/thor/costmatrix.cc

@@ -49,16 +49,15 @@ class CostMatrix::ReachedMap : public robin_hood::unordered_map<uint64_t, std::v
 
 // Constructor with cost threshold.
 CostMatrix::CostMatrix(const boost::property_tree::ptree& config)
-    : max_reserved_labels_count_(config.get<uint32_t>("max_reserved_labels_count_bidir_dijkstras",
+    : MatrixAlgorithm(config),
+      max_reserved_labels_count_(config.get<uint32_t>("max_reserved_labels_count_bidir_dijkstras",
                                                       kInitialEdgeLabelCountBidirDijkstra)),
-      clear_reserved_memory_(config.get<bool>("clear_reserved_memory", false)),
       max_reserved_locations_count_(
           config.get<uint32_t>("max_reserved_locations_costmatrix", kMaxLocationReservation)),
       check_reverse_connections_(config.get<bool>("costmatrix_check_reverse_connection", false)),
       access_mode_(kAutoAccess),
       mode_(travel_mode_t::kDrive), locs_count_{0, 0}, locs_remaining_{0, 0},
-      current_cost_threshold_(0),
-      has_time_(false), targets_{new ReachedMap}, sources_{new ReachedMap} {
+      current_cost_threshold_(0), targets_{new ReachedMap}, sources_{new ReachedMap} {
 }
 
 CostMatrix::~CostMatrix() {
@@ -86,7 +85,7 @@ float CostMatrix::GetCostThreshold(const float max_matrix_distance) {
 
 // Clear the temporary information generated during time + distance matrix
 // construction.
-void CostMatrix::clear() {
+void CostMatrix::Clear() {
   // Clear the target edge markings
   targets_->clear();
   if (check_reverse_connections_)
@@ -132,19 +131,17 @@ void CostMatrix::SourceToTarget(Api& request,
                                 baldr::GraphReader& graphreader,
                                 const sif::mode_costing_t& mode_costing,
                                 const sif::travel_mode_t mode,
-                                const float max_matrix_distance,
-                                const bool has_time,
-                                const bool invariant,
-                                const ShapeFormat& shape_format) {
+                                const float max_matrix_distance) {
 
   LOG_INFO("matrix::CostMatrix");
   request.mutable_matrix()->set_algorithm(Matrix::CostMatrix);
+  bool invariant = request.options().date_time_type() == Options::invariant;
+  auto shape_format = request.options().shape_format();
 
   // Set the mode and costing
   mode_ = mode;
   costing_ = mode_costing[static_cast<uint32_t>(mode_)];
   access_mode_ = costing_->access_mode();
-  has_time_ = has_time;
 
   auto& source_location_list = *request.mutable_options()->mutable_sources();
   auto& target_location_list = *request.mutable_options()->mutable_targets();

src/thor/expansion_action.cc

@@ -130,7 +130,10 @@ std::string thor_worker_t::expansion(Api& request) {
        }) {
     alg->set_track_expansion(track_expansion);
   }
-  costmatrix_.set_track_expansion(track_expansion);
+  for (auto* alg : std::vector<MatrixAlgorithm*>{&costmatrix_, &time_distance_matrix_,
+                                                 &time_distance_bss_matrix_}) {
+    alg->set_track_expansion(track_expansion);
+  }
   isochrone_gen.SetInnerExpansionCallback(track_expansion);
 
   try {

src/thor/matrix_action.cc

@@ -21,43 +21,9 @@
 namespace valhalla {
 namespace thor {
 
-std::string thor_worker_t::matrix(Api& request) {
-  // time this whole method and save that statistic
-  auto _ = measure_scope_time(request);
-
-  auto& options = *request.mutable_options();
-  adjust_scores(options);
-  auto costing = parse_costing(request);
-
-  // TODO: do this for others as well
-  costmatrix_.set_interrupt(interrupt);
-
-  // lambdas to do the real work
-  auto costmatrix = [&](const bool has_time) {
-    return costmatrix_.SourceToTarget(request, *reader, mode_costing, mode,
-                                      max_matrix_distance.find(costing)->second, has_time,
-                                      options.date_time_type() == Options::invariant,
-                                      options.shape_format());
-  };
-  auto timedistancematrix = [&]() {
-    if (options.shape_format() != no_shape)
-      add_warning(request, 207);
-    return time_distance_matrix_.SourceToTarget(request, *reader, mode_costing, mode,
-                                                max_matrix_distance.find(costing)->second,
-                                                options.matrix_locations(),
-                                                options.date_time_type() == Options::invariant);
-  };
+MatrixAlgorithm* thor_worker_t::get_matrix_algorithm(Api& request, const bool has_time) {
 
-  if (costing == "bikeshare") {
-    if (options.shape_format() != no_shape)
-      add_warning(request, 207);
-    time_distance_bss_matrix_.SourceToTarget(request, *reader, mode_costing, mode,
-                                             max_matrix_distance.find(costing)->second,
-                                             options.matrix_locations());
-    return tyr::serializeMatrix(request);
-  }
-
-  Matrix::Algorithm matrix_algo = Matrix::CostMatrix;
+  Matrix::Algorithm config_algo = Matrix::CostMatrix;
   switch (source_to_target_algorithm) {
     case SELECT_OPTIMAL:
       // TODO - Do further performance testing to pick the best algorithm for the job
@@ -66,13 +32,13 @@
         case travel_mode_t::kBicycle:
           // Use CostMatrix if number of sources and number of targets
           // exceeds some threshold
-          if (static_cast<uint32_t>(options.sources().size()) <= kCostMatrixThreshold ||
-              static_cast<uint32_t>(options.targets().size()) <= kCostMatrixThreshold) {
-            matrix_algo = Matrix::TimeDistanceMatrix;
+          if (static_cast<uint32_t>(request.options().sources().size()) <= kCostMatrixThreshold ||
+              static_cast<uint32_t>(request.options().targets().size()) <= kCostMatrixThreshold) {
+            config_algo = Matrix::TimeDistanceMatrix;
           }
           break;
         case travel_mode_t::kPublicTransit:
-          matrix_algo = Matrix::TimeDistanceMatrix;
+          config_algo = Matrix::TimeDistanceMatrix;
           break;
         default:
           break;
@@ -81,32 +47,60 @@
     case COST_MATRIX:
       break;
     case TIME_DISTANCE_MATRIX:
-      matrix_algo = Matrix::TimeDistanceMatrix;
+      config_algo = Matrix::TimeDistanceMatrix;
       break;
   }
 
   // similar to routing: prefer the exact unidirectional algo if not requested otherwise
   // don't use matrix_type, we only need it to set the right warnings for what will be used
-  bool has_time =
-      check_matrix_time(request, options.prioritize_bidirectional() ? Matrix::CostMatrix
-                                                                    : Matrix::TimeDistanceMatrix);
-  if (has_time && !options.prioritize_bidirectional() && source_to_target_algorithm != COST_MATRIX) {
-    timedistancematrix();
-  } else if (has_time && options.prioritize_bidirectional() &&
+  if (has_time && !request.options().prioritize_bidirectional() &&
+      source_to_target_algorithm != COST_MATRIX) {
+    return &time_distance_matrix_;
+  } else if (has_time && request.options().prioritize_bidirectional() &&
              source_to_target_algorithm != TIME_DISTANCE_MATRIX) {
-    costmatrix(has_time);
-  } else if (matrix_algo == Matrix::CostMatrix) {
-    // if this happens, the server config only allows for timedist matrix
-    if (has_time && !options.prioritize_bidirectional()) {
+    return &costmatrix_;
+  } else if (config_algo == Matrix::CostMatrix) {
+    if (has_time && !request.options().prioritize_bidirectional()) {
       add_warning(request, 301);
     }
-    costmatrix(has_time);
+    return &costmatrix_;
   } else {
-    if (has_time && options.prioritize_bidirectional()) {
+    // if this happens, the server config only allows for timedist matrix
+    if (has_time && request.options().prioritize_bidirectional()) {
       add_warning(request, 300);
     }
-    timedistancematrix();
+    return &time_distance_matrix_;
+  }
+}
+
+std::string thor_worker_t::matrix(Api& request) {
+  // time this whole method and save that statistic
+  auto _ = measure_scope_time(request);
+
+  auto& options = *request.mutable_options();
+  adjust_scores(options);
+  auto costing = parse_costing(request);
+
+  bool has_time =
+      check_matrix_time(request, options.prioritize_bidirectional() ? Matrix::CostMatrix
+                                                                    : Matrix::TimeDistanceMatrix);
+
+  // allow all algos to be cancelled
+  for (auto* alg : std::vector<MatrixAlgorithm*>{
+           &costmatrix_,
+           &time_distance_matrix_,
+           &time_distance_bss_matrix_,
+       }) {
+    alg->set_interrupt(interrupt);
+    alg->set_has_time(has_time);
   }
+
+  auto* algo =
+      costing == "bikeshare" ? &time_distance_bss_matrix_ : get_matrix_algorithm(request, has_time);
+
+  algo->SourceToTarget(request, *reader, mode_costing, mode,
+                       max_matrix_distance.find(costing)->second);
+
   return tyr::serializeMatrix(request);
 }
 } // namespace thor

src/thor/optimized_route_action.cc

@@ -28,10 +28,9 @@ void thor_worker_t::optimized_route(Api& request) {
 
   // Use CostMatrix to find costs from each location to every other location
   CostMatrix costmatrix;
+  costmatrix.set_has_time(check_matrix_time(request, Matrix::CostMatrix));
   costmatrix.SourceToTarget(request, *reader, mode_costing, mode,
-                            max_matrix_distance.find(costing)->second,
-                            check_matrix_time(request, Matrix::CostMatrix),
-                            options.date_time_type() == Options::invariant);
+                            max_matrix_distance.find(costing)->second);
 
   // Return an error if any locations are totally unreachable
   const auto& correlated =

src/thor/timedistancebssmatrix.cc

@@ -7,22 +7,6 @@
 using namespace valhalla::sif;
 
 namespace {
-static bool IsTrivial(const uint64_t& edgeid,
-                      const valhalla::Location& origin,
-                      const valhalla::Location& destination) {
-  for (const auto& destination_edge : destination.correlation().edges()) {
-    if (destination_edge.graph_id() == edgeid) {
-      for (const auto& origin_edge : origin.correlation().edges()) {
-        if (origin_edge.graph_id() == edgeid &&
-            origin_edge.percent_along() <= destination_edge.percent_along()) {
-          return true;
-        }
-      }
-    }
-  }
-  return false;
-}
-
 static travel_mode_t get_other_travel_mode(const travel_mode_t current_mode) {
   static const auto bss_modes =
       std::vector<travel_mode_t>{travel_mode_t::kPedestrian, travel_mode_t::kBicycle};
@@ -35,7 +19,7 @@
 
 // Constructor with cost threshold.
 TimeDistanceBSSMatrix::TimeDistanceBSSMatrix(const boost::property_tree::ptree& config)
-    : settled_count_(0), current_cost_threshold_(0),
+    : MatrixAlgorithm(config), settled_count_(0), current_cost_threshold_(0),
       max_reserved_labels_count_(config.get<uint32_t>("max_reserved_labels_count_dijkstras",
                                                       kInitialEdgeLabelCountDijkstras)),
       clear_reserved_memory_(config.get<bool>("clear_reserved_memory", false)) {
@@ -180,10 +164,10 @@
 template <const ExpansionType expansion_direction, const bool FORWARD>
 void TimeDistanceBSSMatrix::ComputeMatrix(Api& request,
                                           baldr::GraphReader& graphreader,
-                                          const float max_matrix_distance,
-                                          const uint32_t matrix_locations) {
-  // Run a series of one to many calls and concatenate the results.
+                                          const float max_matrix_distance) {
+  uint32_t matrix_locations = request.options().matrix_locations();
 
+  // Run a series of one to many calls and concatenate the results.
   auto& origins = FORWARD ? *request.mutable_options()->mutable_sources()
                           : *request.mutable_options()->mutable_targets();
   auto& destinations = FORWARD ? *request.mutable_options()->mutable_targets()
@@ -210,6 +194,7 @@
     SetOrigin<expansion_direction>(graphreader, origin);
     SetDestinationEdges();
 
+    uint32_t n = 0;
     // Find shortest path
     graph_tile_ptr tile;
     while (true) {
@@ -259,6 +244,11 @@
       // Expand forward from the end node of the predecessor edge.
       Expand<expansion_direction>(graphreader, pred.endnode(), pred, predindex, false, false,
                                   pred.mode());
+
+      // Allow this process to be aborted
+      if (interrupt_ && (n++ % kInterruptIterationsInterval) == 0) {
+        (*interrupt_)();
+      }
     }
     reset();
   }
@@ -267,13 +257,11 @@
 template void
 TimeDistanceBSSMatrix::ComputeMatrix<ExpansionType::forward, true>(Api& request,
                                                                    baldr::GraphReader& graphreader,
-                                                                   const float max_matrix_distance,
-                                                                   const uint32_t matrix_locations);
+                                                                   const float max_matrix_distance);
 template void
 TimeDistanceBSSMatrix::ComputeMatrix<ExpansionType::reverse, false>(Api& request,
                                                                     baldr::GraphReader& graphreader,
-                                                                    const float max_matrix_distance,
-                                                                    const uint32_t matrix_locations);
+                                                                    const float max_matrix_distance);
 
 // Add edges at the origin to the adjacency list
 template <const ExpansionType expansion_direction, const bool FORWARD>