Reserve vector capacities and use emplace_back for constructing vectors#1464
Reserve vector capacities and use emplace_back for constructing vectors#1464
Conversation
|
Looks like I forgot to run in release for the above jobs. Rerun below in Without these improvements:
With these improvements:
|
fujitatomoya
left a comment
There was a problem hiding this comment.
lgtm 👍 this is informative, thanks!
| lifecycle_msgs::msg::State state; | ||
| state.id = static_cast<uint8_t>(state_machine_.transition_map.states[i].id); |
There was a problem hiding this comment.
This sequence currently does the construction of the state object, sets the fields, and then push_back onto the vector (invoking a copy, if I understand properly).
What if instead we did an emplace_back of the default-constructed object (avoiding the copy), and then set the fields? Do you think that would be any faster? Same question below.
There was a problem hiding this comment.
That's a good point. I think for the service callback loops it makes sense to use resize to allocate and construct in one go. Then each field can be assigned to. I think emplace_back would have similar performance to resize (but better than reserve).
For the service callbacks though, the performance differential is minimal relative to a service call in general though. So the benchmarks don't really show an improvement for this service call.
|
clalancette
left a comment
There was a problem hiding this comment.
Nice cleanup, I like it.
It looks like the benchmark tests are failing for some reason, though.
Signed-off-by: Stephen Brawner <brawner@gmail.com>
Signed-off-by: Stephen Brawner <brawner@gmail.com>
11f35bc to
efd067a
Compare
|
Since the benchmark infrastructure is still under review, I'm rebasing this PR onto master. Test results after rebasing onto master (no benchmarks):
|
Signed-off-by: Stephen Brawner <brawner@gmail.com>
efd067a to
012ec64
Compare
clalancette
left a comment
There was a problem hiding this comment.
The code looks good, but I'd feel slightly better if the CI was run with --packages-above rclcpp_lifecycle.
|
Here it is for linux |
…rs (#1464) * Reserve vector capacities and use emplace_back for constructing vectors Signed-off-by: Stephen Brawner <brawner@gmail.com> * Use resize instead of reserve Signed-off-by: Stephen Brawner <brawner@gmail.com> * Remove push_back Signed-off-by: Stephen Brawner <brawner@gmail.com>
…rs (#1464) * Reserve vector capacities and use emplace_back for constructing vectors Signed-off-by: Stephen Brawner <brawner@gmail.com> * Use resize instead of reserve Signed-off-by: Stephen Brawner <brawner@gmail.com> * Remove push_back Signed-off-by: Stephen Brawner <brawner@gmail.com>
…rs (#1464) (#1489) * Reserve vector capacities and use emplace_back for constructing vectors Signed-off-by: Stephen Brawner <brawner@gmail.com> * Use resize instead of reserve Signed-off-by: Stephen Brawner <brawner@gmail.com> * Remove push_back Signed-off-by: Stephen Brawner <brawner@gmail.com> Co-authored-by: brawner <brawner@gmail.com>
While benchmarking, I noticed some good performance improvements with
get_available_statesandget_available_transitions. By reserving, allocations can be reduced and using emplace_back where possible can save a duplicate construction. The biggest benefits are in theget_available_states()andget_available_transitions()method, where there is a 3-5x performance increase.Target branch benchmarks:
This branch:
Signed-off-by: Stephen Brawner brawner@gmail.com