Issue 209 cpu timer #5
Conversation
| parallel_for_(cv::Range(0, 2), [&](const cv::Range& range){ | ||
| for (int i = range.start; i < range.end; i++) { |
There was a problem hiding this comment.
Out of curiosity, does this spawn 2 threads, or 3 threads (where the 3rd thread fails the for guard immediately)?
There was a problem hiding this comment.
cv::Range is inclusive of the lower-bound and exclusive of the upper-bound, so the range contains 0 and 1.
This sounds like a nice improvement (maybe as its own PR) as long as updating OpenVINS with upstream changes isn't affected too much. |
If the size will ever greater than 2 (swapchains?), then this is likely worth it. |
|
It's not swapchains; it's the number of eyes on the stereo camera. |
mhuzai
left a comment
mhuzai
left a comment
There was a problem hiding this comment.
Looks good in general. My only concern is that the threading changes will make rebasing on top of latest OV more painful. But I wonder if mainline OV will be willing to accept the threading changes (I think they are good for them too). Would you like to ask them?
|
|
||
| { | ||
| #ifdef ILLIXR_INTEGRATION | ||
| CPU_TIMER_TIME_BLOCK("preform_detection"); |
| // Disabling OpenCV threading is faster on x86 desktop but slower on | ||
| // jetson. Keeping this here for manual disabling. | ||
| // cv::setNumThreads(0); |
There was a problem hiding this comment.
I think it's useful to keep this around.
|
See upstream, open_vins #167. |
Once #3 is merged, the target branch should change (done).
This PR fixes #2 and fixes #4. It's not too much trouble to review them simultaneously.
I elected to use OpenCV
parallel_for_because it means I wouldn't have to implement my own threadpool, bring in the multithread queue, it responds to the samecv::setNumThreads, and its two-level for-loop unifies the single-threaded and mult-threaded cases.When numthreads is greater than 2, the ranges will be [0, 1] and [1, 2], so one thread will go
for (size_t i = 0; i < 1; ++i)(akaigets 1, and then terminates).In many places we have
i == 0 ? img0 : img1. A slightly better way of doing this would be:std::array<Img, 2> imgs;andimgs[i], but this would require changing every usage of these in the rest of OpenVINS, which I considered too burdensome.The reviewer should:
std::array<T, 2>trick or others are worth it or worth it in specific cases.