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

MSA: cleanup includes to speedup compiling #1205

Merged
merged 2 commits into from Nov 19, 2018

Conversation

Projects
None yet
2 participants
@rhaschke
Copy link
Collaborator

commented Nov 17, 2018

MSA builds surprisingly slow. Cleaning up the includes a little bit, I'm hoping for a speedup.

@rhaschke rhaschke force-pushed the ubi-agni:cleanup-msa branch from 62e2665 to 5c1e660 Nov 19, 2018

@v4hn

v4hn approved these changes Nov 19, 2018

Copy link
Member

left a comment

Per se there's nothing wrong with these changes, although I wouldn't backport them to kinetic. (changing header includes is API changes)

Did you benchmark the compile times? I would be surprised if this makes a reasonable difference.
If that's not the case I would discourage such patches in the future.
In this respect I would also be interested in how your requests in the past, e.g. #1156 , influenced compile times.

@@ -282,10 +280,10 @@ class MoveItConfigData
// ******************************************************************************************

/// Load a robot model
void setRobotModel(robot_model::RobotModelPtr robot_model);
void setRobotModel(moveit::core::RobotModelPtr robot_model);

This comment has been minimized.

Copy link
@v4hn

v4hn Nov 19, 2018

Member

you're usually more structured with changes like this. I would have expected that in a separate commit.

This comment has been minimized.

Copy link
@rhaschke

rhaschke Nov 19, 2018

Author Collaborator

Oh. That's a left-over from a commit where I used forward-declaration for PlanningScene (and thus RobotModel) as well.

@@ -39,6 +39,7 @@

#include <QItemSelection>
#include <QPainter>
#include <cmath>

This comment has been minimized.

Copy link
@v4hn

v4hn Nov 19, 2018

Member

same here. I guess clang provides better warnings? :)

This comment has been minimized.

Copy link
@rhaschke

rhaschke Nov 19, 2018

Author Collaborator

This was pulled in by other headers before.

@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 19, 2018

Did you benchmark the compile times?

Not yet, because locally I'm using ccache. However, after merging, I'm curious to compare compile times on the ROS build farm, which doesn't use ccache. In the past, I have observed reductions in compile time, particularly when limiting template stuff (boost) to cpps.

@rhaschke rhaschke merged commit 2ad53e7 into ros-planning:melodic-devel Nov 19, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@rhaschke rhaschke deleted the ubi-agni:cleanup-msa branch Nov 19, 2018

@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 19, 2018

Did you benchmark the compile times?

With this PR, the ros build farm is slightly faster in compiling MSA: before: 191,81s after: 184,44s,
which is a speedup of 4%.

@v4hn

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

a speedup of 4%.

That seems quite a lot indeed. Could you link the webpages please?
I wonder what the deviation in build-time is between different compile jobs of previous moveit releases.

@rhaschke

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 30, 2018

Could you link the webpages please?

Not directly. I manually extracted (relative) build times from the absolute time stamps provided by the ROS build farm. There is no web page directly showing the 4% speedup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.