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.

Sign up for GitHub

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

Jump to bottom

Reduce compilation memory #540

Merged
merged 5 commits into from
May 6, 2020
Merged

Reduce compilation memory #540

merged 5 commits into from
May 6, 2020

Conversation

clalancette
Copy link
Contributor

As pointed out in #539 , it can take a lot of memory to compile rviz_default_plugins. This PR aims to reduce the maximum amount of memory necessary to compile by doing two major things:

  1. Only compiling display_test_fixture.cpp one time for the tests. Previously, each test compiled its own copy, and since it takes ~1GB of memory to compile each time, this can push up the maximum consumption if we have many threads compiling. This PR changes that file to be a library, and then all of the tests link that library in.
  2. Split up the compilation of the MOC QT5 stuff. Previously we were using CMAKE_AUTOMOC, which takes all of the MOC stuff and combines it into one large file. When trying to compile that, it can take upwards for 2.5GB of memory, and we can run out of memory if we are compiling it alongside other things. This PR changes that to generate one MOC per header file, and then compiling those MOC files separately.

The rest of the changes are to support the above.

I've done some basic testing of RViz2 locally with these changes, both the tests and RViz2 itself. I believe this will close #539 .

@clalancette
Reduce the number of times we compile display_test_fixture.cpp
37896f6 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Disable CMAKE_AUTOMOC for tests
51c493f 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Various include path fixes.
a85bbe9 
When we aren't doing one huge automoc file, we need these
paths to be more correct.  This fixes them.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Disable CMAKE_AUTOMOC completely.
0f91880 
It ends up generating one huge mocs_compilation.cpp file that
can take up to 2.5GB to compile.  Instead, generate the individual
files; while it slows down compilation a bit, each file is much
smaller and no single process takes nearly as much memory.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Copy link
Contributor Author

CI:

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

sloretz
sloretz previously approved these changes May 6, 2020
View reviewed changes
@sloretz sloretz dismissed their stale review May 6, 2020 15:35

gah, osx failure as soon as I approve :(

@clalancette
Copy link
Contributor Author

gah, osx failure as soon as I approve :(

Bah. Maybe I need to link something into display_test_fixture. Will take a look.

@clalancette
Fix display_test_fixture library on Windows and macOS.
f83a9d8 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Copy link
Contributor Author

I think f83a9d8 fixes the problems on Windows and macOS. Still waiting for local confirmation of that; once I have it, I'll fire up CI again.

wjwwood
wjwwood approved these changes May 6, 2020
View reviewed changes
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

Approach looks reasonable to me. With ci fixed up.

rviz_default_plugins/CMakeLists.txt Outdated Show resolved Hide resolved
@clalancette
Copy link
Contributor Author

New CI:

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

This was referenced May 6, 2020
Open
Closed
@clalancette
Copy link
Contributor Author

All right, the macOS warning is in the nightly as well: https://ci.ros2.org/view/nightly/job/nightly_osx_release/1638/ .

I've opened up ament/ament_cmake#255 to track the addition to ament_cmake, and #543 to track the refactoring of display_test_fixture.cpp.

With that, I'm going to go ahead and merge this. Thanks for the reviews.

@clalancette clalancette merged commit daa3348 into ros2 May 6, 2020
@clalancette clalancette deleted the reduce-display-compilations branch May 6, 2020 21:13
@wjwwood
Copy link
Member

wjwwood commented May 7, 2020

Please squash when merging...

@clalancette
Copy link
Contributor Author

Please squash when merging...

I intentionally didn't here, since the changes were a logical progression from one to the next. I thought it would be easier to understand what I did in the individual commits rather than one squashed one.

@wjwwood
Copy link
Member

wjwwood commented May 8, 2020

From a changelog perspective, it's not any easier to read, because all the same information is there, as the commit messages from the component commits end up in the squashed commit message. The separate commits continue to exist here on this pr, and squashing makes it easier to cherry-pick this set of changes if we wanted to do that. I think squashing is pretty much always better, and at least on this repository I'd like to keep doing that in the future.

@clalancette clalancette mentioned this pull request May 12, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wjwwood wjwwood wjwwood approved these changes

@sloretz sloretz sloretz left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

rviz_default_plugins runs out of memory on CI packaging jobs
3 participants
@clalancette @wjwwood @sloretz