You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
cvflann::Heap<T>::getPooledInstance() keeps its reusable per-search heap in a single process-wide static map guarded by a static cv::Mutex. the pool here is keyed by cv::utils::getThreadID() at every call site (kdtree, kmeans and hierarchical index search) so each thread only ever touches its own entry but the issue is every search on every thread still has to take that one global lock. Under a multi-threaded FlannBasedMatcher workload the searches serialize on it which is why FlannBasedMatcher ended up slower than BFMatcher (in the issue it measured CPU utilization dropping from ~2000% to ~300%).
This moves the pool into per-thread storage (cv::TLSData) and drops the mutex so that each thread own an independent set of heaps and there is no shared state to lock. I didnt change the per-thread heap semantics so the use_count guard and the iteration-threshold cleanup now simply operate on the calling thread's own map. cv::TLSData is used instead ofa raw thread_local because a thread_local for this dynamically-initialized object seemed unreliable to me in testing and also its the primitive cv::utils::getThreadID() itself is built on aswell.
a shared FlannBasedMatcher hit by radiusMatch from N threads on an Intel Core i5-12450H (8c/12t) goes from ~2200 to ~3700 matches/s at 8 threads (~1.7x) and single-thread cost stays unchanged. The old code stopped scaling after ~4 threads while the new one keeps scaling and gets higher with higher core count :)
I added tests to cover heap ordering, capacity, the per-thread isolation guarantee and concurrent use (threaded cases guarded by OPENCV_DISABLE_THREAD_SUPPORT)
cvflann::Heap<T>::getPooledInstance() is designed for global key-value store, not for thead local stuff and may be used in different ways. The modification changes behaviour. I propose not to modify the Heap class itself, but replace
getPooledInstance() call with C++ thead_local variable inside the function. It changes local behaviour, but not change API behaviour and much simpler.
cvflann::Heap<T>::getPooledInstance() is designed for global key-value store, not for thead local stuff and may be used in different ways. The modification changes behaviour. I propose not to modify the Heap class itself, but replace getPooledInstance() call with C++ thead_local variable inside the function. It changes local behaviour, but not change API behaviour and much simpler.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #25281
cvflann::Heap<T>::getPooledInstance()keeps its reusable per-search heap in a single process-widestaticmap guarded by astatic cv::Mutex. the pool here is keyed bycv::utils::getThreadID()at every call site (kdtree, kmeans and hierarchical index search) so each thread only ever touches its own entry but the issue is every search on every thread still has to take that one global lock. Under a multi-threadedFlannBasedMatcherworkload the searches serialize on it which is whyFlannBasedMatcherended up slower thanBFMatcher(in the issue it measured CPU utilization dropping from ~2000% to ~300%).This moves the pool into per-thread storage (
cv::TLSData) and drops the mutex so that each thread own an independent set of heaps and there is no shared state to lock. I didnt change the per-thread heap semantics so theuse_countguard and the iteration-threshold cleanup now simply operate on the calling thread's own map.cv::TLSDatais used instead ofa rawthread_localbecause athread_localfor this dynamically-initialized object seemed unreliable to me in testing and also its the primitivecv::utils::getThreadID()itself is built on aswell.a shared
FlannBasedMatcherhit byradiusMatchfrom N threads on an Intel Core i5-12450H (8c/12t) goes from ~2200 to ~3700 matches/s at 8 threads (~1.7x) and single-thread cost stays unchanged. The old code stopped scaling after ~4 threads while the new one keeps scaling and gets higher with higher core count :)I added tests to cover heap ordering, capacity, the per-thread isolation guarantee and concurrent use (threaded cases guarded by
OPENCV_DISABLE_THREAD_SUPPORT)Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.