Skip to content

Disable OpenMP by default - #66

Open
magehrig wants to merge 2 commits into
masterfrom
feature/disable-openmp-by-default
Open

magehrig wants to merge 2 commits into
masterfrom
feature/disable-openmp-by-default

Conversation

@magehrig

Copy link
Copy Markdown
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

HannesSommer commented Jul 27, 2017

Copy link
Copy Markdown
Collaborator

👍 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

magehrig commented Aug 4, 2017

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Collaborator

This would also work around #37 .

@porteusconf porteusconf left a comment

Copy link
Copy Markdown

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