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

Added benchmark test to rcl_logging_spdlog #52

Merged
merged 9 commits into from
Sep 24, 2020

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Sep 17, 2020

Added benchmark test to rcl_logging_spdlog

Signed-off-by: ahcorde ahcorde@gmail.com

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde self-assigned this Sep 17, 2020
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Copy link
Member

@cottsay cottsay left a comment

Choose a reason for hiding this comment

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

I'm getting the sense that you're trying to get good coverage in the performance tests here. I don't think that should be a goal. Arbitrary numbers coming out of the performance test results don't help us figure out what's wrong when they increase, and don't help us make determinations as to whether the performance is "acceptable" because we'd never perform this sort of operation when consuming this API.

I see two scenarios being tested inside of each of these tests: the case where the level of the log operation is a "hit", and the case where it is a "miss" and is ignored. I think we'll be interested in those two cases specifically. I wouldn't expect that to performance to change based on what levels are actually used, only which of those modes occurs.

It would also be good to test the initialize/shutdown sequence.

Nice job with the fixture, though.

rcl_logging_spdlog/CMakeLists.txt Outdated Show resolved Hide resolved
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Sep 23, 2020

Compiling up-to rcl_logging_spdlog and testing rcl_logging_spdlog with -DAMENT_RUN_PERFORMANCE_TESTS=ON:

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

Copy link
Member

@cottsay cottsay left a comment

Choose a reason for hiding this comment

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

Almost there.

ahcorde and others added 3 commits September 23, 2020 23:01
Signed-off-by: ahcorde <ahcorde@gmail.com>
This package's init function has a hot path when it has already been
initialized, so the first iteration of the init test was cold and the
rest were hot. I changed that test to specifically test
re-initialization by performing the cold init outside of the timing
loop. That test also wasn't previously shutting down when the run was
completed.

I added a cold initialize call to the shutdown test, so that test should
give us the full startup/shutdown times now. So the two tests now give
us an idea of what the first startup is like, as well as subsequent
"hot" startups.

Signed-off-by: Scott K Logan <logans@cottsay.net>
For the log-hit test, there must be some heap allocation associated with
the first call, because we aren't getting a consistent number of
allocations on that first call. We can resolve that by explicitly
"priming" the logger, then timing only the consistently behaving calls.

For the re-initialize test, of course the cold startup is performing
more allocations than the hot startup, so we need to exclude those
allocations from the metric.

Signed-off-by: Scott K Logan <logans@cottsay.net>
@cottsay
Copy link
Member

cottsay commented Sep 24, 2020

Fresh builds:

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

The target's interface will take care of passing this along.

Signed-off-by: Scott K Logan <logans@cottsay.net>
@cottsay cottsay merged commit 749e5ad into master Sep 24, 2020
@delete-merged-branch delete-merged-branch bot deleted the ahcorde/benchmark/rcl_logging_spdlog branch September 24, 2020 23:49
ahcorde added a commit that referenced this pull request Oct 5, 2020
Signed-off-by: Alejandro Hernández Cordero <alejandro@openrobotics.org>
ahcorde added a commit that referenced this pull request Oct 8, 2020
Signed-off-by: Alejandro Hernández Cordero <alejandro@openrobotics.org>
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.

None yet

2 participants