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

Use parallel_for_ to replace multicore switch #167

Merged
merged 4 commits into from
Jun 15, 2021

Conversation

charmoniumQ
Copy link
Contributor

This uses parallel_for_ instead of boost:thread. This has three advantages:

  1. No need to maintain two versions of the same code (one parallel, one sequential). One parallel_for_ can be run with or without parallelism.
  2. No need to pass around use_multicore. cv::setNumThreads globally controls whether parallelism is used.
  3. It does not create/destroy threads while running. The old code created/destroyed tens of thousands of threads. This results in lower performance and throws off analysis (like perf and NVIDIA NSight Computer).

@goldbattle
Copy link
Member

Thanks for the PR! Seem like it would be nice to remove the boost::threads as it seems have have some drawbacks. I will try to find time to review (along with the other issues I need to address on the repo). If you are able to debug why the CI is failing that would help me out and ensure that people are able to use the project out of the box.

I see that you included a dockerfile. Is this necessary?

@charmoniumQ
Copy link
Contributor Author

Thanks for the quick response. I fixed the CI. Dumb copy/pasting mistake.

The Dockerfile is not necessary. I can delete it, but I thought that other users might benefit from it. It makes it easier to test OpenVINS without installing ROS to one's physical machine.

@goldbattle
Copy link
Member

goldbattle commented May 19, 2021

You will likely need to use the LambdaBody to have it compile on all platforms.
https://github.com/rpng/open_vins/blob/master/ov_core/src/track/Grider_FAST.h#L37-L52

I am fine with adding a dockerfile if you provide a small guide / information on how to use and develop with it (or a link to a website that I can use as a reference). I would want to add a documentation page for those coming to the project and want to use it.

EDIT: the LambdaBody should probably go into its own header in the ov_core/utils folder so that all files can depend on that instead of a gridder header.

@charmoniumQ charmoniumQ force-pushed the use-parallel-for branch 2 times, most recently from 223a012 to e22f1c6 Compare May 19, 2021 22:16
@charmoniumQ
Copy link
Contributor Author

charmoniumQ commented May 21, 2021

LambdaBody problem is fixed with your suggestion.

@goldbattle
Copy link
Member

Thanks, I will be able to merge later part of next week after ICRA wraps up.
Thanks again for the PR.
This should close issue #166 .
Do you have any number to the overhead of the thread creation / old boost thread method?

@goldbattle goldbattle self-assigned this May 29, 2021
@charmoniumQ
Copy link
Contributor Author

On my machine, thread setup/teardown costs 25us. You can do your own timing here.

I think the bigger sell is that it makes performance much easier to analyze using tools like NSight Systems and perf.

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.

2 participants