Skip to content

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.

4 participants