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

Disable OpenMP by default #66

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

magehrig
Copy link
Contributor

OpenMP can significantly slow down NN search if the knn function is also running in multiple threads at the same time. It was not straightforward to detect that libnabo was significantly slower (in some cases 20x slower) in that case compared to just using a single threaded version of libnabo. Therefore I suggest, that the parallel option has to be actively set by the user to avoid accidentally slowing down NN search.

I have also experimented with the num_threads function to provide an option to disable multithreading at runtime. However, there is still a small overhead with num_threads(1) compared to not compiling with OpenMP.

fyi: @HannesSommer

@HannesSommer
Copy link
Collaborator

HannesSommer commented Jul 27, 2017

👍 I've made similar negative experience with running NN searches parallel to other tasks. That was why I introduced this option in the first place.

@norlab-ulaval norlab-ulaval deleted a comment from ethzasl-jenkins Jul 27, 2017
@magehrig
Copy link
Contributor Author

magehrig commented Aug 4, 2017

I would also like to add that the current parallelized version is significantly faster for large kNN problems. But it is not clear what "large" means in this context. The best solution would be an option to set the number of threads manually without having an overhead for a single threaded version. As mentioned, OpenMP always has an overhead that is not insignificant. Maybe using a thread-pool implementation with std::thread could be a solution. But I would still like to merge this PR, if possible, because multiple people in the lab have reported that it makes a difference in their applications.

@HannesSommer
Copy link
Collaborator

This would also work around #37 .

Copy link

@porteusconf porteusconf left a comment

Choose a reason for hiding this comment

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

worked for me. Also do the docs discuss thread-safe operation?

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.

3 participants