Reduce copying in API parameter constructors

When using non-default constructors for the API parameter classes, vector arguments like coordinates and hints are copied at least once (twice when passed as lvalue arguments). Enable perfect forwarding of BaseParameter arguments and pass-by-value in the constructor that uses the argument. This ensures we copy at most once (zero for rvalue arguments).
2021-01-06 15:24:27 +00:00 · 2021-01-06 15:24:27 +00:00 · e554624438
commit e554624438
parent 58ba3fc84f
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;