Skip to content

core: improve doc for setNumThreads#22889

Merged
asmorkalov merged 2 commits intoopencv:4.xfrom
D-Alex:patch-1
Apr 7, 2023
Merged

core: improve doc for setNumThreads#22889
asmorkalov merged 2 commits intoopencv:4.xfrom
D-Alex:patch-1

Conversation

@D-Alex
Copy link
Copy Markdown
Contributor

@D-Alex D-Alex commented Nov 30, 2022

The old documentation implies that the call is only valid for the next parallel region and must be called again if addtional regions should be affected as well.

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
  • The PR is proposed to the proper branch
  • There is a reference to the original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

The old documentation implies that the call is only valid for the next parallel region and must be called again if addtional regions should be affected as well.
@asmorkalov asmorkalov requested a review from alalek November 30, 2022 11:26
@asmorkalov asmorkalov added the category: documentation Documentation fix or update label Nov 30, 2022
@asmorkalov asmorkalov added this to the 4.7.0 milestone Nov 30, 2022
/** @brief OpenCV will try to set the number of threads for the next parallel region.
/** @brief OpenCV will try to set the number of threads for subsequent parallel regions.

If threads == 0, OpenCV will disable threading optimizations and run all it's functions
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Proposed change looks good to me.


However there is another design issue. Which contradicts to the real implementation of some parallel backends.
There is mess with threads == 0 and threads == 1 values.

E.g., OpenMP case:

https://github.com/opencv/opencv/blame/913a2dbdaa3fbd011f7d4f08b381deb9dfe43b97/modules/core/src/parallel.cpp#L469

omp_set_num_threads(threads > 0 ? threads : numThreadsMax);

(code is from 2012 at least)

Docs states this:

If threads == 0, OpenCV will disable threading optimizations and run all it's functions sequentially

So, using '0' parameter value is not recommended at all and it is a subject to the change.


Another important note, setNumThreads() is not a thread-safe function.
It is fatal to call it simultaneously in case of TBB at least (TBB's arena may become corrupted).

@asmorkalov
Copy link
Copy Markdown
Contributor

@D-Alex Please add note on the thread-safety for setNumThreads.

@asmorkalov
Copy link
Copy Markdown
Contributor

@mshabunin I extended the patch according to Alexander's recommendation. Could you take a look?

@asmorkalov asmorkalov merged commit f5a92cb into opencv:4.x Apr 7, 2023
@asmorkalov asmorkalov assigned asmorkalov and unassigned mshabunin Apr 7, 2023
@asmorkalov asmorkalov mentioned this pull request May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants