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

Add multiprocessing support to ament_clang_tidy #288

Merged
merged 3 commits into from
Jan 25, 2021

Conversation

mm318
Copy link
Member

@mm318 mm318 commented Jan 19, 2021

This can speedup the amount of time ament_clang_tidy spends analyzing/linting packages.

Without this change:

time ament_clang_tidy build/ > log_base.txt
real    2m18.908s
user    2m13.533s
sys     0m4.656s

With this change, using 1 job (default):

time ament_clang_tidy build/ > log_1cpu.txt
real    2m20.998s
user    2m15.870s
sys     0m4.364s

See log_1cpu.txt for

With this change, using 4 parallel jobs:

time ament_clang_tidy --jobs 4 build/ > log_4cpu.txt
real    0m49.372s
user    2m20.114s
sys     0m4.619s

The attached log files also show that analysis results have not changed:
log_base.txt
log_1cpu.txt
log_4cpu.txt

Signed-off-by: Miaofei <miaofei@amazon.com>
Signed-off-by: Miaofei <miaofei@amazon.com>
Copy link
Contributor

@tylerjw tylerjw left a comment

Choose a reason for hiding this comment

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

I tested this locally and it greatly speeds up using ament_clang_tidy.

@mm318
Copy link
Member Author

mm318 commented Jan 20, 2021

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

@tylerjw
Copy link
Contributor

tylerjw commented Jan 20, 2021

  • Linux Build Status

  • Linux-aarch64 Build Status

  • macOS Build Status

  • Windows Build Status

Minor aside, in the future would you like to migrate PR CI testing into GitHub actions?

@mm318
Copy link
Member Author

mm318 commented Jan 20, 2021

Minor aside, in the future would you like to migrate PR CI testing into GitHub actions?

I would like that, but it's not my call.

From my understanding, GitHub Actions now supports macOS and Windows, which is good.

It would probably be hard to make GitHub Actions support building and testing a arbitrary collection of ROS packages that depend on each other. This may be acceptable for the ament_lint repo, but probably not for the other ament repos or the ROS2 core repos.

@tylerjw
Copy link
Contributor

tylerjw commented Jan 20, 2021

It would probably be hard to make GitHub Actions support building and testing a arbitrary collection of ROS packages that depend on each other. This may be acceptable for the ament_lint repo, but probably not for the other ament repos or the ROS2 core repos.

I think this is actually fairly doable using https://github.com/ros-industrial/industrial_ci

If you'd like I can try to create a PR to do this. Can you point me to how the CI on jenkins is configured?

Caveat I wouldn't know the least bit about how to go about doing this on Windows/MacOS even though github actions support those platforms.

@mm318
Copy link
Member Author

mm318 commented Jan 21, 2021

If you'd like I can try to create a PR to do this. Can you point me to how the CI on jenkins is configured?

I don't mind. But it would probably take convincing more than a couple of people at OSRF to move PR CI testing from Jenkins to GitHub Actions.

If you follow the link in the badges (for example, https://ci.ros2.org/job/ci_linux/13454/), it shows the repos file, the Ubuntu distro, ROS distro, colcon build --packages-up-to, and colcon test --packages-select options used. These options were chosen by me specifically for this PR. Not sure if that's the kind of configuration details you were looking for.

OSRF does require testing on Windows and macOS before merging. So while GitHub Actions does support Windows and macOS, ROS Industrial's CI might not.

@tylerjw
Copy link
Contributor

tylerjw commented Jan 21, 2021

On topic, this looks like it needs to be rebased for the doc change. I'm looking forward to using this feature. I'm curious if you have any idea how it could be used from colcon/cmake. Do linters already run in parallel when launched from colcon (once thread project up to system threads or -j option)?

Signed-off-by: Miaofei <miaofei@amazon.com>

# Conflicts:
#	ament_clang_tidy/doc/index.rst
@mm318
Copy link
Member Author

mm318 commented Jan 21, 2021

Re-running CI after merging in master branch:

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

@mm318
Copy link
Member Author

mm318 commented Jan 21, 2021

@claireyywang, would you mind reviewing this PR, too? I think it is a highly sought after feature :)

@mm318
Copy link
Member Author

mm318 commented Jan 21, 2021

I'm curious if you have any idea how it could be used from colcon/cmake.

I am not familiar with how colcon invokes things. But I believe changes need to made on the CMake side, particularly passing --jobs into ament_add_test() at


What that number should be passed in should probably be dynamically determined by CMake or make, so that when you're building multiple packages in parallel, you don't get a massive number of clang-tidy processes spawned.

Do linters already run in parallel when launched from colcon (once thread project up to system threads or -j option)?

I think colcon test invokes make test -j <number of cores>, which probably runs different linters in parallel, but each linter will use its default number of threads.

@mm318
Copy link
Member Author

mm318 commented Jan 25, 2021

@claireyywang, friendly ping. Or if you could get another maintainer to take a look. Thanks!

Copy link
Contributor

@claireyywang claireyywang left a comment

Choose a reason for hiding this comment

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

LGTM

@claireyywang claireyywang merged commit a89090c into ament:master Jan 25, 2021
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.

None yet

3 participants