Merge pull request #5926 from mjjbell/mbell/parameter_improvements

Reduce copying in API parameter constructors
Project-OSRM · Jan 21, 2021 · a05d7a8 · a05d7a8
2 parents 58ba3fc + e554624
commit a05d7a8
Show file tree

Hide file tree

Showing 5 changed files with 31 additions and 27 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,7 @@
       - REMOVED: we no longer publish Node 8 binary modules (they are still buildable from source) [#5918](https://github.com/Project-OSRM/osrm-backend/pull/5918)
     - Routing:
       - FIXED: Avoid copying ManyToMany table results [#5923](https://github.com/Project-OSRM/osrm-backend/pull/5923)
+      - FIXED: Reduce copying in API parameter constructors [#5925](https://github.com/Project-OSRM/osrm-backend/pull/5925)
     - Misc:
       - CHANGED: Unify `.osrm.turn_penalites_index` dump processing same with `.osrm.turn_weight_penalties` and `.osrm.turn_duration_penalties` [#5868](https://github.com/Project-OSRM/osrm-backend/pull/5868)
     - Profile:

diff --git a/include/engine/api/base_parameters.hpp b/include/engine/api/base_parameters.hpp
@@ -92,21 +92,21 @@ struct BaseParameters
 
     SnappingType snapping = SnappingType::Default;
 
-    BaseParameters(const std::vector<util::Coordinate> coordinates_ = {},
-                   const std::vector<boost::optional<Hint>> hints_ = {},
+    BaseParameters(std::vector<util::Coordinate> coordinates_ = {},
+                   std::vector<boost::optional<Hint>> hints_ = {},
                    std::vector<boost::optional<double>> radiuses_ = {},
                    std::vector<boost::optional<Bearing>> bearings_ = {},
                    std::vector<boost::optional<Approach>> approaches_ = {},
                    bool generate_hints_ = true,
                    std::vector<std::string> exclude = {},
                    const SnappingType snapping_ = SnappingType::Default)
-        : coordinates(coordinates_), hints(hints_), radiuses(radiuses_), bearings(bearings_),
-          approaches(approaches_), exclude(std::move(exclude)), generate_hints(generate_hints_),
-          snapping(snapping_)
+        : coordinates(std::move(coordinates_)), hints(std::move(hints_)),
+          radiuses(std::move(radiuses_)), bearings(std::move(bearings_)),
+          approaches(std::move(approaches_)), exclude(std::move(exclude)),
+          generate_hints(generate_hints_), snapping(snapping_)
     {
     }
 
-    // FIXME add validation for invalid bearing values
     bool IsValid() const
     {
         return (hints.empty() || hints.size() == coordinates.size()) &&
@@ -115,7 +115,7 @@ struct BaseParameters
                (approaches.empty() || approaches.size() == coordinates.size()) &&
                std::all_of(bearings.begin(),
                            bearings.end(),
-                           [](const boost::optional<Bearing> bearing_and_range) {
+                           [](const boost::optional<Bearing> &bearing_and_range) {
                                if (bearing_and_range)
                                {
                                    return bearing_and_range->IsValid();

diff --git a/include/engine/api/match_parameters.hpp b/include/engine/api/match_parameters.hpp
@@ -68,17 +68,20 @@ struct MatchParameters : public RouteParameters
     }
 
     template <typename... Args>
-    MatchParameters(std::vector<unsigned> timestamps_, GapsType gaps_, bool tidy_, Args... args_)
-        : MatchParameters(std::move(timestamps_), gaps_, tidy_, {}, std::forward<Args>(args_)...)
+    MatchParameters(const std::vector<unsigned> &timestamps_,
+                    GapsType gaps_,
+                    bool tidy_,
+                    Args &&... args_)
+        : MatchParameters(timestamps_, gaps_, tidy_, {}, std::forward<Args>(args_)...)
     {
     }
 
     template <typename... Args>
     MatchParameters(std::vector<unsigned> timestamps_,
                     GapsType gaps_,
                     bool tidy_,
-                    std::vector<std::size_t> waypoints_,
-                    Args... args_)
+                    const std::vector<std::size_t> &waypoints_,
+                    Args &&... args_)
         : RouteParameters{std::forward<Args>(args_)..., waypoints_}, timestamps{std::move(
                                                                          timestamps_)},
           gaps(gaps_), tidy(tidy_)

diff --git a/include/engine/api/route_parameters.hpp b/include/engine/api/route_parameters.hpp
@@ -87,7 +87,7 @@ struct RouteParameters : public BaseParameters
                     const GeometriesType geometries_,
                     const OverviewType overview_,
                     const boost::optional<bool> continue_straight_,
-                    Args... args_)
+                    Args &&... args_)
         // Once we perfectly-forward `args` (see #2990) this constructor can delegate to the one
         // below.
         : BaseParameters{std::forward<Args>(args_)...}, steps{steps_}, alternatives{alternatives_},
@@ -105,7 +105,7 @@ struct RouteParameters : public BaseParameters
                     const GeometriesType geometries_,
                     const OverviewType overview_,
                     const boost::optional<bool> continue_straight_,
-                    Args... args_)
+                    Args &&... args_)
         : BaseParameters{std::forward<Args>(args_)...}, steps{steps_}, alternatives{alternatives_},
           number_of_alternatives{alternatives_ ? 1u : 0u}, annotations{annotations_},
           annotations_type{annotations_ ? AnnotationsType::All : AnnotationsType::None},
@@ -123,12 +123,12 @@ struct RouteParameters : public BaseParameters
                     const GeometriesType geometries_,
                     const OverviewType overview_,
                     const boost::optional<bool> continue_straight_,
-                    Args... args_)
+                    Args &&... args_)
         : BaseParameters{std::forward<Args>(args_)...}, steps{steps_}, alternatives{alternatives_},
           number_of_alternatives{alternatives_ ? 1u : 0u},
-          annotations{annotations_ == AnnotationsType::None ? false : true},
-          annotations_type{annotations_}, geometries{geometries_}, overview{overview_},
-          continue_straight{continue_straight_}, waypoints()
+          annotations{annotations_ != AnnotationsType::None}, annotations_type{annotations_},
+          geometries{geometries_}, overview{overview_}, continue_straight{continue_straight_},
+          waypoints()
     {
     }
 
@@ -141,12 +141,12 @@ struct RouteParameters : public BaseParameters
                     const OverviewType overview_,
                     const boost::optional<bool> continue_straight_,
                     std::vector<std::size_t> waypoints_,
-                    const Args... args_)
+                    const Args &&... args_)
         : BaseParameters{std::forward<Args>(args_)...}, steps{steps_}, alternatives{alternatives_},
           number_of_alternatives{alternatives_ ? 1u : 0u}, annotations{annotations_},
           annotations_type{annotations_ ? AnnotationsType::All : AnnotationsType::None},
           geometries{geometries_}, overview{overview_},
-          continue_straight{continue_straight_}, waypoints{waypoints_}
+          continue_straight{continue_straight_}, waypoints{std::move(waypoints_)}
     {
     }
 
@@ -159,12 +159,12 @@ struct RouteParameters : public BaseParameters
                     const OverviewType overview_,
                     const boost::optional<bool> continue_straight_,
                     std::vector<std::size_t> waypoints_,
-                    Args... args_)
+                    Args &&... args_)
         : BaseParameters{std::forward<Args>(args_)...}, steps{steps_}, alternatives{alternatives_},
-          number_of_alternatives{alternatives_ ? 1u : 0u},
-          annotations{annotations_ == AnnotationsType::None ? false : true},
+          number_of_alternatives{alternatives_ ? 1u : 0u}, annotations{annotations_ !=
+                                                                       AnnotationsType::None},
           annotations_type{annotations_}, geometries{geometries_}, overview{overview_},
-          continue_straight{continue_straight_}, waypoints{waypoints_}
+          continue_straight{continue_straight_}, waypoints{std::move(waypoints_)}
     {
     }
 

diff --git a/include/engine/api/table_parameters.hpp b/include/engine/api/table_parameters.hpp
@@ -85,7 +85,7 @@ struct TableParameters : public BaseParameters
     template <typename... Args>
     TableParameters(std::vector<std::size_t> sources_,
                     std::vector<std::size_t> destinations_,
-                    Args... args_)
+                    Args &&... args_)
         : BaseParameters{std::forward<Args>(args_)...}, sources{std::move(sources_)},
           destinations{std::move(destinations_)}
     {
@@ -95,7 +95,7 @@ struct TableParameters : public BaseParameters
     TableParameters(std::vector<std::size_t> sources_,
                     std::vector<std::size_t> destinations_,
                     const AnnotationsType annotations_,
-                    Args... args_)
+                    Args &&... args_)
         : BaseParameters{std::forward<Args>(args_)...}, sources{std::move(sources_)},
           destinations{std::move(destinations_)}, annotations{annotations_}
     {
@@ -108,7 +108,7 @@ struct TableParameters : public BaseParameters
                     double fallback_speed_,
                     FallbackCoordinateType fallback_coordinate_type_,
                     double scale_factor_,
-                    Args... args_)
+                    Args &&... args_)
         : BaseParameters{std::forward<Args>(args_)...}, sources{std::move(sources_)},
           destinations{std::move(destinations_)}, fallback_speed{fallback_speed_},
           fallback_coordinate_type{fallback_coordinate_type_}, annotations{annotations_},
@@ -122,7 +122,7 @@ struct TableParameters : public BaseParameters
         if (!BaseParameters::IsValid())
             return false;
 
-        // Distance Table makes only sense with 2+ coodinates
+        // Distance Table makes only sense with 2+ coordinates
         if (coordinates.size() < 2)
             return false;