Merge pull request #5926 from mjjbell/mbell/parameter_improvements

Reduce copying in API parameter constructors
2021-01-21 15:22:26 +02:00 · 2021-01-21 15:22:26 +02:00 · a05d7a8f73
commit a05d7a8f73
parent 58ba3fc84f e554624438
5 changed files with 31 additions and 27 deletions
--- 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:
--- 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();
--- a/include/engine/api/match_parameters.hpp
+++ b/include/engine/api/match_parameters.hpp
@ -68,8 +68,11 @@ 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_)...)
    {
    }

@ -77,8 +80,8 @@ struct MatchParameters : public RouteParameters
    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_)
--- 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_)}
    {
    }

--- 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;