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

Cleanup the rosbag2_transport tests #1518

Merged
merged 4 commits into from
Dec 28, 2023
Merged

Conversation

clalancette
Copy link
Contributor

@clalancette clalancette commented Dec 20, 2023

The main goal here is to reduce compile times and memory usage. The way we do it:

  1. Make as many of the dependencies private as possible. That way the main executable is exporting the minimum of dependencies to downstream consumers.
  2. Compile the test files once, instead of once per RMW. We can do this because we change the RMW at runtime with an environment variable. This requires some changes to ament_cmake, which are in Split ament_add_gmock into _executable and _test. ament/ament_cmake#497

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

@clalancette
Copy link
Contributor Author

All right, ament/ament_cmake#497 is merged now. So converting this to a real PR and running CI on it.

@clalancette clalancette marked this pull request as ready for review December 21, 2023 01:47
@clalancette clalancette requested a review from a team as a code owner December 21, 2023 01:47
@clalancette clalancette requested review from emersonknapp and james-rms and removed request for a team December 21, 2023 01:47
@clalancette
Copy link
Contributor Author

CI:

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

@clalancette
Copy link
Contributor Author

After a small fix here, let's try CI again:

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

Copy link
Contributor

@MichaelOrlov MichaelOrlov left a 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.

@clalancette
Copy link
Contributor Author

But what about memory consumption during compilation? Do you see any improvements?

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]>
@clalancette clalancette force-pushed the clalancette/cleanup-tests branch from 09555c8 to 1fce988 Compare December 26, 2023 16:05
Copy link
Contributor

@MichaelOrlov MichaelOrlov left a 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.

@clalancette
Copy link
Contributor Author

CI:

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

@clalancette clalancette merged commit aceeb38 into rolling Dec 28, 2023
13 checks passed
@delete-merged-branch delete-merged-branch bot deleted the clalancette/cleanup-tests branch December 28, 2023 13:28
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.

👨‍🌾 rosbag2_transport can run out of memory in coverage jobs
3 participants