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

Add benchmark tests #973

Open
wants to merge 3 commits into
base: rolling
Choose a base branch
from
Open

Add benchmark tests #973

wants to merge 3 commits into from

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Jul 19, 2022 •
edited
Loading

This PR adds benchmarks that measure how quickly different types of entities are handled. This would be useful for testing performance improvements.

The benchmark tests are skipped unless AMENT_RUN_PERFORMANCE_TESTS is set, which is the same behavior as as ament_add_google_benchmark_test.

WIP because this needs a new rosdep key python3-pytest-benchmark for https://pypi.org/project/pytest-benchmark/
Requires ros/rosdistro#33918

@sloretz sloretz added the enhancement New feature or request label Jul 19, 2022
@sloretz sloretz self-assigned this Jul 19, 2022
@sloretz sloretz mentioned this pull request Jul 20, 2022
Merged
@sloretz sloretz force-pushed the sloretz__add_benchmark_tests branch from 60cbd2c to 1527a9e Compare July 22, 2022 19:13
@sloretz sloretz mentioned this pull request Jul 22, 2022
Open
fujitatomoya
fujitatomoya reviewed Jul 25, 2022
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.

benchmark test would be really useful. Do we have any plan to integrate this kind of benchmark in CI/CD so that it can detect the performance degression?

@sloretz sloretz marked this pull request as ready for review July 25, 2022 17:59
@sloretz
Copy link
Contributor Author

sloretz commented Jul 25, 2022

Do we have any plan to integrate this kind of benchmark in CI/CD so that it can detect the performance degression?

There are benchmark jobs run for each release on build.ros2.org. Here's the one for Rolling https://build.ros2.org/view/Rci/job/Rci__benchmark_ubuntu_jammy_amd64/ . It looks like it runs nightly, but I think it wouldn't show an issue until the package got released, and I don't think there's an automatic comparison with past jobs.

@sloretz
Copy link
Contributor Author

sloretz commented Jul 25, 2022

@ros-pull-request-builder retest this please

@sloretz
Copy link
Contributor Author

sloretz commented Jul 25, 2022

CI (build: --packages-up-to rclpy test: --packages-select rclpy)

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

And one with the benchmark tests enabled (build: -DAMENT_RUN_PERFORMANCE_TESTS=ON --packages-up-to rclpy test: --packages-select rclpy)

  • Linux Build Status

@sloretz sloretz mentioned this pull request Jul 27, 2022
Open
@sloretz
Copy link
Contributor Author

sloretz commented Jul 27, 2022

Benchmark tests one more time with ros2/ci#674

  • Linux: Build Status

@sloretz
Copy link
Contributor Author

sloretz commented Jul 27, 2022

CI LGTM!

jacobperron
jacobperron approved these changes Jul 28, 2022
View reviewed changes
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

One minor comment and a question. Otherwise, LGTM

rclpy/CMakeLists.txt Outdated
APPEND_ENV AMENT_PREFIX_PATH=${ament_index_build_path}
PYTHONPATH=${CMAKE_CURRENT_BINARY_DIR}
TIMEOUT 120
WERROR ON
)
# Skip benchmark tests by default
Copy link
Member

Choose a reason for hiding this comment

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

This comment looks like it's in the wrong place. Maybe at L209?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved comment to correct spot in 5a0f651

rclpy/test/benchmark/test_benchmark_actions.py
executor.spin_until_future_complete(result_fut)
assert GoalStatus.STATUS_SUCCEEDED == result_fut.result().status

benchmark(bm)
Copy link
Member

Choose a reason for hiding this comment

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

In the other tests we are asserting the result of benchmark, should we do that here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the assertions now I don't think they were adding value. I removed them in 76fd188

rclpy/test/benchmark/test_benchmark_client_service.py
executor.spin_until_future_complete(fut)
assert fut.result() # Raises if there was an error

benchmark(bm)
Copy link
Member

Choose a reason for hiding this comment

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

Same here, should we assert?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed assertions in 76fd188

@sloretz
Add benchmark tests
a0e2392 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Move comment to correct spot
5a0f651 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Remove assertions
76fd188 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz sloretz force-pushed the sloretz__add_benchmark_tests branch from 1527a9e to 76fd188 Compare October 14, 2022 01:51
@sloretz
Copy link
Contributor Author

sloretz commented Oct 14, 2022

CI re-run

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

Benchmark job: Build Status

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fujitatomoya fujitatomoya fujitatomoya left review comments

@jacobperron jacobperron jacobperron approved these changes

Assignees

@sloretz sloretz

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sloretz @jacobperron @fujitatomoya