-
Notifications
You must be signed in to change notification settings - Fork 36
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
Conversation
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
There was a problem hiding this 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.
Signed-off-by: ahcorde <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there.
rcl_logging_spdlog/test/benchmark/benchmark_logging_interface.cpp
Outdated
Show resolved
Hide resolved
rcl_logging_spdlog/test/benchmark/benchmark_logging_interface.cpp
Outdated
Show resolved
Hide resolved
rcl_logging_spdlog/test/benchmark/benchmark_logging_interface.cpp
Outdated
Show resolved
Hide resolved
rcl_logging_spdlog/test/benchmark/benchmark_logging_interface.cpp
Outdated
Show resolved
Hide resolved
Signed-off-by: ahcorde <[email protected]>
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 <[email protected]>
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 <[email protected]>
The target's interface will take care of passing this along. Signed-off-by: Scott K Logan <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Added benchmark test to
rcl_logging_spdlog
Signed-off-by: ahcorde [email protected]