Support throttling serialized graph publisher #204
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
efernandez
requested review from
ayrton04 and
svwilliams
and removed request for
ayrton04
ayrton04
approved these changes
Nov 25, 2020
svwilliams
approved these changes
Nov 25, 2020
This fix is based on: https://answers.ros.org/question/199680/catkin-doesnt-play-nice-with-google-mock/?answer=318629#post-id-318629 By using catkin_add_gmock instead of catkin_add_gtest the sensor proc test can use both gtest and gmock. Otherwise, we get a compilation error because the gmock/gmock.h header cannot be found. More information about catkin_add_gmock in: http://docs.ros.org/en/melodic/api/catkin/html/dev_guide/generated_cmake_api.html#catkin-add-gmock
efernandez
force-pushed
the
throttle-serialized-publisher-upstream
branch
from
2a7d11f
to
0458f04
Compare
|
Not sure why the checks failed, so I've rebase this PR on top of #207 |
svwilliams
approved these changes
Nov 30, 2020
This was referenced Jul 13, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
I've been throttling the serialized graph message published by the
fuse_publishers::SerializedPublisherusing thethrottlenode fromtopic_tools, so I only record a few graphs for debugging purposes. Recording all would take too much disk and bandwidth.So far so good, but the serialized publisher is still serializing and publishing all the graphs it gets notified from the optimizer. It turns out that serializing the graph is quite fast, but it still takes a noticeable amount of time. If the optimization frequency is high, we could get too many graphs and the impact becomes more significant.
Since I'm throttling the serialized graph messages after anyway, it makes more sense to throttle the publisher directly in the serialized publisher, so we can save some cycles.
Changelog
fuse_models::ThrottledCallbacktofuse_coreThrottledCallback. An alias template calledThrottledMessageCallbackis provided for ROS messages. Unfortunately, the automatic type deduction inros::NodeHandle::subcribe()no longer works due to the genericThrottledCallback::callback()we have now. For this reason we now have to dosubscribe<Message>()instead of justsubscribe(). If you know how to make this cleaner, I'd like to know. 馃槂parametersfolder and a class to encapsulate thefuse_publishers::SerializedPublisherconfiguration, as it's done infuse_models.Results
If the optimization frequency or the rate of the sensor models inputs are low, the impact of this is very low. I've evaluated this with the following relevant configuration:
The plots below show the CPU% of the fuse optimizer node in blue, and the average in red:
We save approx. 10% of CPU.