Improve the compilation time of rviz_default_plugins #1007
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
The main motivation here is to remove an exported absolute path from this package. To do this, mark everything in the CMakeLists.txt private that we can. This ended up exposing a bunch of missing dependencies in rviz_default_plugins, so fix those here as well. Signed-off-by: Chris Lalancette <clalancette@gmail.com>
These are the last RViz packages that need this update. Signed-off-by: Chris Lalancette <clalancette@gmail.com>
These are already found in the main code, so we don't need to re-find them for tests. Signed-off-by: Chris Lalancette <clalancette@gmail.com>
This makes it easier for us to see which tests have which dependencies, and is more correct. Signed-off-by: Chris Lalancette <clalancette@gmail.com>
They are not compilable files, so it is nonsensical to
pass them to ament_add_g{test,mock}
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
That is, stop using ament_target_dependencies and instead switch to target_link_libraries everywhere. This is the more modern CMake way to do things, and gives us finer grain control over whether dependencies should be PUBLIC or PRIVATE. Signed-off-by: Chris Lalancette <clalancette@gmail.com>
By making a library we skip compiling the same file multiple times for different tests. Signed-off-by: Chris Lalancette <clalancette@gmail.com>
By making a library we skip compiling the same file multiple times for different tests. Signed-off-by: Chris Lalancette <clalancette@gmail.com>
By making a library we skip compiling the same file multiple times for different tests. Signed-off-by: Chris Lalancette <clalancette@gmail.com>
By making a library we skip compilingthe same file multiple times for different tests. Signed-off-by: Chris Lalancette <clalancette@gmail.com>
By making a library we skip compiling the same file multiple times for different tests. Signed-off-by: Chris Lalancette <clalancette@gmail.com>
By making a library we skip compiling the same file multiple times for different tests. This particular piece of code should be safe from gtest/gmock ODR errors since it does not use that functionality at all. Signed-off-by: Chris Lalancette <clalancette@gmail.com>
ahcorde
approved these changes
Jul 7, 2023
|
|
Bah humbug. The switch to C++17 is triggering some Windows warnings. Since that is a nice to have and not an integral part of this PR, I'm going to just revert that bit and try again. |
This reverts commit 4c8268a.
|
New CI:
|
|
Not a full apples-to-apples comparison, but this does seem to be about 5 minutes faster on Windows debug than on the nightlies. So a modest improvement, but an improvement nonetheless. @ahcorde This is probably going to conflict with your other PRs. How do you want to handle this? Put this in first and then rebase yours? Or get your other ones in and then rebase this one? |
|
I will merge this first I will update my PRs, thank you for this improvement! |
|
@clalancette are you planning to backport this ? |
|
https://github.com/Mergifyio backport iron |
✅ Backports have been created
|
mergify bot
pushed a commit
that referenced
this pull request
Jul 13, 2023
* Cleanup rviz_visual_testing_framework CMakeLists.txt The main motivation here is to remove an exported absolute path from this package. To do this, mark everything in the CMakeLists.txt private that we can. This ended up exposing a bunch of missing dependencies in rviz_default_plugins, so fix those here as well. Signed-off-by: Chris Lalancette <clalancette@gmail.com> (cherry picked from commit 5af8896)
ahcorde
pushed a commit
that referenced
this pull request
Jul 18, 2023
* Cleanup rviz_visual_testing_framework CMakeLists.txt The main motivation here is to remove an exported absolute path from this package. To do this, mark everything in the CMakeLists.txt private that we can. This ended up exposing a bunch of missing dependencies in rviz_default_plugins, so fix those here as well. Signed-off-by: Chris Lalancette <clalancette@gmail.com> (cherry picked from commit 5af8896) Co-authored-by: Chris Lalancette <clalancette@gmail.com>
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.
The main goal of this PR is to improve the compilation time of rviz_default_plugins. To do that, there are a number of patches in here which do quite a bit of cleanup. The most important to mention are:
target_link_librariesinstead ofament_target_dependencies.The net result of all of this is that in my local testing, compilation time is now 12% better. That doesn't seem like a huge percentage gain, but it is 40 seconds faster locally. If that percentage maintains on the buildfarm, then this will be several minutes faster on Windows.