-
Notifications
You must be signed in to change notification settings - Fork 260
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
Cleanup the rosbag2_transport tests #1518
Conversation
All right, ament/ament_cmake#497 is merged now. So converting this to a real PR and running CI on it. |
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.
@clalancette Thanks for the PR.
Since the #872 popup again I was considering to do something similar sonner.
You mentioned that build speed increased 3x, that is a good result and could be expectable, if we now compiling for 1 RMW implementation instead of 3 + other optimizations.
But what about memory consumption during compilation? Do you see any improvements?
Sorry, I have not had a chance to experiment with it myself.
I am curious if can do even better.
I did not measure it during build times. I expect it will be better, just from the fact that we are compiling fewer things. In terms of improvement, I think this is the best we can do with changes to the build system only; using less memory would probably require changes to the templates, in particular. But I think that is a follow-up. |
In particular, hide more of the dependencies as PRIVATE for the main library, so less is exported to downstream consumers. Signed-off-by: Chris Lalancette <[email protected]>
Instead of compiling once for each RMW, just compile the test files once and then use environment variables to switch between the various RMW implementations. Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
That is, for each test, make this call. It puts the registration of the test along side the compilation of the test, at the expense of duplicating the boilerplate for each one. Signed-off-by: Chris Lalancette <[email protected]>
09555c8
to
1fce988
Compare
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.
@clalancette Thank you! LGTM.
The main goal here is to reduce compile times and memory usage. The way we do it:
With this PR in place, build times drop by about 3x for me locally (from about 3 minutes to about 1 minute).
A draft until ament/ament_cmake#497 lands.
Fixes #872