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.

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

Performance tests demo #288

Merged
merged 9 commits into from
Mar 11, 2021
Merged

Performance tests demo #288

merged 9 commits into from
Mar 11, 2021

Conversation

Blast545
Copy link
Contributor

@Blast545 Blast545 commented Sep 1, 2020

Testing performance test usage for rcutils/error_handling.h and rcutils/logging.h.

IMO it would be a good idea to combine coverage measurements to see which parts of the code are run with the performance tests, investigation pending.

Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
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.

It would be nice to integrate performance benchmark test. but for doing that, we should be able to have some criteria or threshold to decide if performance is not good enough? I think that most likely this performance check is gonna be useful to make sure if the staging PR does not affect the performance aspect. Could you share your thoughts about this kind of plan?

test/test_benchmark_error_handling.cpp Outdated Show resolved Hide resolved
Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
@Blast545
Copy link
Contributor Author

Blast545 commented Sep 2, 2020

@fujitatomoya There is a pending discussion on #278 for this package, we will be discussing what's reasonable to test for each package in terms of performance and include checks on CI to be able of checking when a PR affects considerably performance to be considered before merging.

@Blast545 Blast545 self-assigned this Sep 8, 2020
CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

Does it make sense to add a new folder for the benchmark tests?

CMakeLists.txt Outdated Show resolved Hide resolved
test/test_benchmark_logging.cpp Outdated Show resolved Hide resolved
test/test_benchmark_logging.cpp Outdated Show resolved Hide resolved
Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
@Blast545
Copy link
Contributor Author

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

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

I have two suggestions:

  • call the target benchmark_logging instead of benchmark_test_logging. I think is redundant somehow.
  • remove the word test from the filename. (test_benchmark_logging.cpp -> benchmark_logging.cpp)

Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
@Blast545
Copy link
Contributor Author

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

@clalancette
Copy link
Contributor

@Blast545 What's the status of this one? Should we merge it?

@ahcorde
Copy link
Contributor

ahcorde commented Oct 23, 2020

@clalancette I believe that rhe idea it's to create end-to-end performance tests instead of microbenchmarks.

Maybe @chapulina can add more context here.

@chapulina
Copy link

Yes, the new plan is to create end-to-end tests. But it looks like this PR is already approved and close to getting in, so it would be a shame to throw it away. What's missing to merge it?

@clalancette
Copy link
Contributor

Yes, the new plan is to create end-to-end tests. But it looks like this PR is already approved and close to getting in, so it would be a shame to throw it away. What's missing to merge it?

Yeah, agreed. If this is almost ready to go, then I would say we get this in, but don't build any new micro-benchmarks.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
@Blast545
Copy link
Contributor Author

CI:

  • Linux Build Status

@Blast545 Blast545 requested a review from cottsay January 14, 2021 20:49
@clalancette
Copy link
Contributor

Here is full CI on this to make sure everything is happy. If it is, I'll go ahead and merge it:

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

@clalancette clalancette dismissed stale reviews from cottsay and ahcorde March 11, 2021 14:24

Stale

@clalancette clalancette merged commit ab9ee5e into master Mar 11, 2021
@delete-merged-branch delete-merged-branch bot deleted the blast545/perf_tests_demo branch March 11, 2021 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants