Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Reserve vector capacities and use emplace_back for constructing vectors #1464

Merged
merged 3 commits into from
Nov 17, 2020
Merged

Reserve vector capacities and use emplace_back for constructing vectors #1464

merged 3 commits into from
Nov 17, 2020

Conversation

brawner
Copy link
Contributor

@brawner brawner commented Nov 17, 2020 •
edited
Loading

While benchmarking, I noticed some good performance improvements with get_available_states and get_available_transitions. By reserving, allocations can be reduced and using emplace_back where possible can save a duplicate construction. The biggest benefits are in the get_available_states() and get_available_transitions() method, where there is a 3-5x performance increase.

Target branch benchmarks:

  • Linux Build Status

This branch:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Signed-off-by: Stephen Brawner brawner@gmail.com

@brawner
Copy link
Contributor Author

brawner commented Nov 17, 2020

Looks like I forgot to run in release for the above jobs. Rerun below in Release

Without these improvements:
Build Status

benchmark_lifecycle_node.cpp

BenchmarkLifecycleNode/get_available_states                       1495 ns         1495 ns       465907 heap_allocations=5
BenchmarkLifecycleNode/get_available_transitions                  2306 ns         2305 ns       305477 heap_allocations=6

With these improvements:
Build Status

benchmark_lifecycle_node.cpp

BenchmarkLifecycleNode/get_available_states                        368 ns          368 ns      1916816 heap_allocations=1
BenchmarkLifecycleNode/get_available_transitions                   589 ns          589 ns      1177915 heap_allocations=1

fujitatomoya
fujitatomoya approved these changes Nov 17, 2020
View reviewed changes
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 👍 this is informative, thanks!

clalancette
clalancette reviewed Nov 17, 2020
View reviewed changes
rclcpp_lifecycle/src/lifecycle_node_interface_impl.hpp Outdated
Comment on lines 271 to 272
lifecycle_msgs::msg::State state;
state.id = static_cast<uint8_t>(state_machine_.transition_map.states[i].id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@brawner
Copy link
Contributor Author

brawner commented Nov 17, 2020

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

clalancette
clalancette reviewed Nov 17, 2020
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanup, I like it.

It looks like the benchmark tests are failing for some reason, though.

@brawner
Reserve vector capacities and use emplace_back for constructing vectors
a42b195 
Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner
Use resize instead of reserve
c853773 
Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner brawner force-pushed the brawner/lifecycle-node-optimizations branch from 11f35bc to efd067a Compare November 17, 2020 19:28
@brawner brawner changed the base branch from brawner/rclcpp_lifecycle-benchmarks to master November 17, 2020 19:28
@brawner
Copy link
Contributor Author

brawner commented Nov 17, 2020 •
edited
Loading

Since the benchmark infrastructure is still under review, I'm rebasing this PR onto master.

Test results after rebasing onto master (no benchmarks):

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@brawner
Remove push_back
012ec64 
Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner brawner force-pushed the brawner/lifecycle-node-optimizations branch from efd067a to 012ec64 Compare November 17, 2020 19:36
clalancette
clalancette approved these changes Nov 17, 2020
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good, but I'd feel slightly better if the CI was run with --packages-above rclcpp_lifecycle.

@brawner
Copy link
Contributor Author

brawner commented Nov 17, 2020

Here it is for linux
Build Status

@brawner brawner merged commit de353f9 into master Nov 17, 2020
@delete-merged-branch delete-merged-branch bot deleted the brawner/lifecycle-node-optimizations branch November 17, 2020 21:07
@brawner brawner added this to Proposed in Foxy Patch Release 4 via automation Nov 17, 2020
@brawner brawner moved this from Proposed to Needs backport in Foxy Patch Release 4 Nov 17, 2020
@brawner brawner mentioned this pull request Nov 18, 2020
Merged
jacobperron pushed a commit that referenced this pull request Dec 8, 2020
@brawner @jacobperron
Reserve vector capacities and use emplace_back for constructing vecto…
ad6aaba 
…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>
jacobperron pushed a commit that referenced this pull request Dec 8, 2020
@brawner @jacobperron
Reserve vector capacities and use emplace_back for constructing vecto…
d70c075 
…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>
@jacobperron jacobperron mentioned this pull request Dec 8, 2020
Merged
jacobperron added a commit that referenced this pull request Dec 8, 2020
@jacobperron @brawner
Reserve vector capacities and use emplace_back for constructing vecto…
3dddfd7 
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@brawner @clalancette @fujitatomoya