MantisBT - Open CASCADE
View Issue Details
0030959Open CASCADE[OCCT] OCCT:Foundation Classespublic2019-09-11 11:022019-09-15 10:55
[OCCT] 7.4.0[OCCT] 7.4.0 
Not required
0030959: OSD_Parallel_TBB: number of execution threads is strictly limited by the root scope
In case of TBB usage, number of execution threads is regulated by task_scheduler_init as minimum from number of elements to be processed and default value specfied by OSD_ThreadPool.

Given that parallel task itself performs some computations in parallel, number of threads remains the same and equal to value specified by the root scope even if several more threads could be allocated according to OSD_ThreadPool.

This leads to inconsistency between specified parameter and expected number of threads used by algorithms.
Not required
No tags attached.
child of 0030573closed apn OSD_Parallel_TBB: limit number of execution threads using settings of OSD_ThreadPool::DefaultPool() 
Issue History
2019-09-11 11:02oanNew Issue
2019-09-11 11:02oanAssigned To => abv
2019-09-11 11:03oanRelationship addedchild of 0030573
2019-09-11 11:07gitNote Added: 0086999
2019-09-11 13:11oanNote Added: 0087007
2019-09-11 13:11oanAssigned Toabv => msv
2019-09-11 13:11oanStatusnew => resolved
2019-09-11 13:11oanSteps to Reproduce Updatedbug_revision_view_page.php?rev_id=21809#r21809
2019-09-11 18:39msvNote Added: 0087014
2019-09-11 18:39msvAssigned Tomsv => oan
2019-09-11 18:39msvStatusresolved => assigned
2019-09-12 11:18oanNote Added: 0087018
2019-09-12 11:18oanAssigned Tooan => msv
2019-09-12 11:18oanStatusassigned => feedback
2019-09-12 11:46msvNote Added: 0087020
2019-09-12 11:46msvAssigned Tomsv => bugmaster
2019-09-12 11:46msvStatusfeedback => reviewed
2019-09-12 18:47bugmasterNote Added: 0087029
2019-09-12 18:47bugmasterStatusreviewed => tested
2019-09-12 18:48bugmasterTest case number => Not required
2019-09-15 10:51bugmasterChangeset attached => occt master 3068c3bb
2019-09-15 10:51bugmasterStatustested => verified
2019-09-15 10:51bugmasterResolutionopen => fixed
2019-09-15 10:55gitNote Added: 0087098

2019-09-11 11:07   
Branch CR30959 has been created by oan.

SHA-1: 497e7d52eb77817a2dcd99877442ad5744a7e5aa

Detailed log of new commits:

Author: oan
Date: Wed Sep 11 11:04:03 2019 +0300

    0030959: OSD_Parallel_TBB: number of execution threads is strictly limited by the root scope
    Do not limit number of available threads by number of items processed within the root scope due to possibility of spawning of an additional threads within the subscopes.
2019-09-11 13:11   
Please review:

http://occt-tests/CR30959-master-OAN-OCCT/Windows-64-VC14/diff_summary.html [^]
http://occt-tests/CR30959-master-OAN-OCCT/Debian80-64/diff_summary.html [^]
2019-09-11 18:39   
For tbb usage, it is better not to set number of threads explicitly. So, I would not called task_scheduler_init at all, or called it with default value (automatic).
The quote from tbb ref manual:
"It is preferable option for production code because it helps to avoid nasty surprises when several TBB based components run side-by-side or in a nested fashion inside the same process."
2019-09-12 11:18   

I beg your pardon, but I didn't get your remark completely.

>> For tbb usage, it is better not to set number of threads explicitly.
What does it mean given that the option to control number of threads already exists both for TBB and Native implementation of OSD_Parallel and requested about a year ago by one of our customers and already integrated in OCCT?

Moreover, TBB provides functionaltiy to set number of threads explicitly via task_scheduler_init, it is the aim of this official function provided by TBB, so it is natural feature and it is not "better" to avoid it completely, especially if such feature is needed for various projects (0030573 and 0028199).

>> So, I would not called task_scheduler_init at all, or called it with default value (automatic).
First, what do you suggest to limit number of threads in case of TBB if you removed task_scheduler_init at all?
If you don't have any constructive idea, task_scheduler_init is the only solution in this case.
Let's consider existing code and changes introduced by current patch:

const Standard_Integer aNbThreads = theNbItems > 0 ?
      Min (theNbItems, aThreadPool->NbDefaultThreadsToLaunch()) : -1;

const Standard_Integer aNbThreads = theNbItems > 0 ?
      aThreadPool->NbDefaultThreadsToLaunch() : -1;

Here, the only change is not to limit scheduler by minimum of items to be processed and default number of threads, but use default number of threads explicitly (see description of the issue to figure out the reason).

This is the documentation note about theNbItems:
//! @param theNbItems number of items passed by iterator, -1 if unknown

Value -1 is exactly that "default (automatic)" value that leads to standard behaviour of TBB procedures.
2019-09-12 11:46   
It was just my general remark about implementation of this functionality. It is that tbb implementation is much smarter than our, and such way we restrict tbb's smartness. One feature is supporting of affinity mask. Tbb by default restricts the number of threads taking into account affinity mask if any.

I agree that if we decide to do more smart behavior we will do it in scope of another issue.

So, this patch is reviewed.
2019-09-12 18:47   
Combination -
OCCT branch : CR30959
master SHA - 497e7d52eb77817a2dcd99877442ad5744a7e5aa
Products branch : master SHA - f9d0bd5e3a29d6a97b3f5f6354ea2397253ab4f8
was compiled on Linux, MacOS and Windows platforms and tested in optimize mode.

Number of compiler warnings:
No new/fixed warnings

No regressions/differences

CPU differences:
Total CPU difference: 16840.53000000009 / 16836.290000000165 [+0.03%]
Total CPU difference: 10570.34000000003 / 10553.510000000031 [+0.16%]
Total CPU difference: 18333.484375 / 18300.0625 [+0.18%]
Total CPU difference: 12402.859375 / 12227.453125 [+1.43%]

Image differences :
No differences that require special attention

Memory differences :
No differences that require special attention
2019-09-15 10:55   
Branch CR30959 has been deleted by inv.

SHA-1: 497e7d52eb77817a2dcd99877442ad5744a7e5aa