From e554624438f722181481f66718786d0a945b72a8 Mon Sep 17 00:00:00 2001 From: Michael Bell Date: Wed, 6 Jan 2021 15:24:27 +0000 Subject: [PATCH] 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). --- CHANGELOG.md | 1 + include/engine/api/base_parameters.hpp | 14 +++++++------- include/engine/api/match_parameters.hpp | 11 +++++++---- include/engine/api/route_parameters.hpp | 24 ++++++++++++------------ include/engine/api/table_parameters.hpp | 8 ++++---- 5 files changed, 31 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index af99a218c..2ff20e4f6 100644 --- 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 index ad071faec..e49f71157 100644 --- 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 coordinates_ = {}, - const std::vector> hints_ = {}, + BaseParameters(std::vector coordinates_ = {}, + std::vector> hints_ = {}, std::vector> radiuses_ = {}, std::vector> bearings_ = {}, std::vector> approaches_ = {}, bool generate_hints_ = true, std::vector 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_and_range) { + [](const boost::optional &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 index 73a660caf..774a2c09d 100644 --- a/include/engine/api/match_parameters.hpp +++ b/include/engine/api/match_parameters.hpp @@ -68,8 +68,11 @@ struct MatchParameters : public RouteParameters } template - MatchParameters(std::vector timestamps_, GapsType gaps_, bool tidy_, Args... args_) - : MatchParameters(std::move(timestamps_), gaps_, tidy_, {}, std::forward(args_)...) + MatchParameters(const std::vector ×tamps_, + GapsType gaps_, + bool tidy_, + Args &&... args_) + : MatchParameters(timestamps_, gaps_, tidy_, {}, std::forward(args_)...) { } @@ -77,8 +80,8 @@ struct MatchParameters : public RouteParameters MatchParameters(std::vector timestamps_, GapsType gaps_, bool tidy_, - std::vector waypoints_, - Args... args_) + const std::vector &waypoints_, + Args &&... args_) : RouteParameters{std::forward(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 index afda6c859..8d5dd9ae4 100644 --- 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 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_)...}, steps{steps_}, alternatives{alternatives_}, @@ -105,7 +105,7 @@ struct RouteParameters : public BaseParameters const GeometriesType geometries_, const OverviewType overview_, const boost::optional continue_straight_, - Args... args_) + Args &&... args_) : BaseParameters{std::forward(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 continue_straight_, - Args... args_) + Args &&... args_) : BaseParameters{std::forward(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 continue_straight_, std::vector waypoints_, - const Args... args_) + const Args &&... args_) : BaseParameters{std::forward(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 continue_straight_, std::vector waypoints_, - Args... args_) + Args &&... args_) : BaseParameters{std::forward(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 index a18b6808f..30e17900b 100644 --- a/include/engine/api/table_parameters.hpp +++ b/include/engine/api/table_parameters.hpp @@ -85,7 +85,7 @@ struct TableParameters : public BaseParameters template TableParameters(std::vector sources_, std::vector destinations_, - Args... args_) + Args &&... args_) : BaseParameters{std::forward(args_)...}, sources{std::move(sources_)}, destinations{std::move(destinations_)} { @@ -95,7 +95,7 @@ struct TableParameters : public BaseParameters TableParameters(std::vector sources_, std::vector destinations_, const AnnotationsType annotations_, - Args... args_) + Args &&... args_) : BaseParameters{std::forward(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_)...}, 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;