MantisBT - Open CASCADE
View Issue Details
0029177Open CASCADE[OCCT] OCCT:Foundation Classespublic2017-10-03 14:482018-12-15 20:28
oan 
apn 
normalminor 
verifiedfixed 
 
[OCCT] 7.4.0* 
Not needed
0029177: Foundation Classes - Adapt BVH package for use of OSD_Parallel
Currently BVH package uses raw TBB tools to launch sorting and building tools in parallel. It is suggested to replace explicit calls to TBB by standard OCCT tool called OSD_Parallel.

This feature supposes implementation of task runner on level of OSD_Parallel.
Nothing to do
No tags attached.
related to 0029702verified apn Foundation Classes - Introduce possibility to control parallel execution of BVH tools 
child of 0028931closed bugmaster Eliminate dependency from TBB in OSD_Parallel header 
Issue History
2017-10-03 14:48oanNew Issue
2017-10-03 14:48oanAssigned To => abv
2017-10-03 14:48oanRelationship addedchild of 0028931
2018-04-18 11:21gitNote Added: 0075507
2018-04-18 11:27oanNote Added: 0075508
2018-04-18 11:27oanStatusnew => resolved
2018-04-18 11:27oanSteps to Reproduce Updatedbug_revision_view_page.php?rev_id=18956#r18956
2018-04-18 12:20kgvSummaryAdapt BVH package for use of OSD_Parallel => Foundation Classes - Adapt BVH package for use of OSD_Parallel
2018-04-18 12:26gitNote Added: 0075509
2018-04-18 12:31kgvNote Added: 0075511
2018-04-18 12:32kgvNote Edited: 0075511bug_revision_view_page.php?bugnote_id=75511#r18958
2018-04-18 13:01oanNote Added: 0075514
2018-04-18 13:44kgvNote Added: 0075515
2018-04-18 13:44kgvNote Edited: 0075515bug_revision_view_page.php?bugnote_id=75515#r18960
2018-04-18 13:45kgvNote Edited: 0075515bug_revision_view_page.php?bugnote_id=75515#r18961
2018-04-18 14:07oanNote Added: 0075518
2018-04-18 14:20kgvNote Added: 0075520
2018-04-18 18:44oanRelationship addedrelated to 0029702
2018-04-27 15:23oanNote Added: 0075720
2018-07-27 16:57oanNote Added: 0078108
2018-08-02 15:38oanAssigned Toabv => msv
2018-08-03 11:36msvNote Added: 0078344
2018-11-23 12:08gitNote Added: 0081235
2018-11-30 17:58msvNote Added: 0081319
2018-11-30 17:58msvAssigned Tomsv => oan
2018-11-30 17:58msvStatusresolved => assigned
2018-11-30 18:52oanNote Added: 0081322
2018-11-30 18:52oanAssigned Tooan => msv
2018-11-30 18:52oanStatusassigned => feedback
2018-11-30 22:49oanNote Edited: 0081322bug_revision_view_page.php?bugnote_id=81322#r20446
2018-12-01 11:57oanNote Edited: 0081322bug_revision_view_page.php?bugnote_id=81322#r20447
2018-12-03 11:05msvNote Added: 0081337
2018-12-03 11:05msvAssigned Tomsv => oan
2018-12-03 11:07msvNote Added: 0081338
2018-12-03 11:40gitNote Added: 0081341
2018-12-03 11:42oanNote Added: 0081343
2018-12-03 11:42oanAssigned Tooan => msv
2018-12-03 11:42oanStatusfeedback => resolved
2018-12-03 11:55msvNote Added: 0081345
2018-12-03 11:55msvAssigned Tomsv => oan
2018-12-03 11:55msvStatusresolved => assigned
2018-12-03 11:55msvNote Edited: 0081345bug_revision_view_page.php?bugnote_id=81345#r20449
2018-12-03 13:47gitNote Added: 0081352
2018-12-03 13:48oanNote Added: 0081354
2018-12-03 13:48oanAssigned Tooan => msv
2018-12-03 13:48oanStatusassigned => resolved
2018-12-03 16:00msvNote Added: 0081365
2018-12-03 16:00msvAssigned Tomsv => bugmaster
2018-12-03 16:00msvStatusresolved => reviewed
2018-12-04 11:33apnTest case number => Not needed
2018-12-04 11:33apnNote Added: 0081381
2018-12-04 11:33apnStatusreviewed => tested
2018-12-15 20:28apnChangeset attached => occt master da555fc2
2018-12-15 20:28apnAssigned Tobugmaster => apn
2018-12-15 20:28apnStatustested => verified
2018-12-15 20:28apnResolutionopen => fixed

Notes
(0075507)
git   
2018-04-18 11:21   
Branch CR29177 has been created by oan.

SHA-1: 8f8467c5dbc7ebfe726392b55a6d556280ec3c7e


Detailed log of new commits:

Author: oan
Date: Wed Apr 18 11:21:00 2018 +0300

    0029177: Adapt BVH package for use of OSD_Parallel
    
    Explicit calls to TBB in BVH_DistanceField, BVH_LinearBuilder and BVH_RadixSorter have been replaced.
    Task functors have been separated on execution and state parts for the sake of usage by OSD_Parallel.
(0075508)
oan   
2018-04-18 11:27   
Please review.

I have one remark regarding the patch.
BVH_DistanceField now operates on a single slice instead of a bunch of slices.
This could possibly slow down performance (or not).
(0075509)
git   
2018-04-18 12:26   
Branch CR29177 has been updated forcibly by oan.

SHA-1: b2d0160a71c78066c71ce3563390d8ad3f038683
(0075511)
kgv   
2018-04-18 12:31   
(edited on: 2018-04-18 12:32)
If I read the patch correctly, it forces BVH_RadixSorter and BVH_LinearBuilder using multiple threads unconditionally, where previously it was done only with HAVE_TBB being enabled.

Although previous solution was no good, it at least protected most users not using TBB from this unexpected implicit parallelization (potentially affecting performance in negative way), and it is also known that TBB has more sophisticated code for managing thread pool than OSD_Parallel currently has (with TBB disabled).

(0075514)
oan   
2018-04-18 13:01   
Hi Kirill,

thanks for comments.

As I understand, described problems related to default implementation of OSD_Parallel available when HAVE_TBB is disabled are also applicable for every OCCT's algorithm using OSD_Parallel tool, e.g. Boolean operations, BRepMesh, probably visualization.

This patch solves only explicit dependency on TBB, nothing more.
(0075515)
kgv   
2018-04-18 13:44   
(edited on: 2018-04-18 13:45)
> As I understand, described problems related to default
> implementation of OSD_Parallel available when HAVE_TBB
> is disabled are also applicable for every OCCT's algorithm
> using OSD_Parallel tool, e.g. Boolean operations,
> BRepMesh, probably visualization.
No, my statement is irrelevant to OSD_Parallel weak points, but rather about making multi-threading manageable for each algorithm.

(Most) other places have flags disabling/enabling multi-threading (with DISABLED being preferred option by default), but not these two places.
So that after the patch as is, user will get implicit multi-threading with no way for disabling it at runtime nor at compile time (while previously it was enabled conditionally at compile time by HAVE_TBB).

(0075518)
oan   
2018-04-18 14:07   
I think I have got the point.

Yes, you are right, this places have become implicitly parallelized.
However, approach with HAVE_TBB is also not a way, because disabling TBB will lead that OCCT will loss all flexibility and scalability provided by TBB at all.

So, despite of described problem we have got consistent behaviour of BVH package regardless of TBB. Please note that old version always runs BVH_RadixSorter in parallel when TBB is available. And always runs it consecutively when it is disabled.

Regarding static flag, I suppose it is good idea to introduce it in usual way, i.e. defining static variable controlled via draw command. However, I'm inclined to think that it should be done in context of separate issue as far as we should make sure that current behaviour is consistent with one implemented in master.
(0075520)
kgv   
2018-04-18 14:20   
> However, I'm inclined to think that it should be done in context of separate issue
Sure, it is better fixing this problem in scope of dedicated issue.
But this issue will be a prerequisites for integration of this patch into "master".
(0075720)
oan   
2018-04-27 15:23   
Please review.

Test results are available by the following link:

http://jenkins-test-11.nnov.opencascade.com/view/CR29177-master-oan/view/COMPARE/ [^]
(0078108)
oan   
2018-07-27 16:57   
Dear Andrey,

could you please review this patch as far as it has become quite expected.

Regards,
Oleg.
(0078344)
msv   
2018-08-03 11:36   
>But this issue will be a prerequisites for integration of this patch into "master".
First the bug 0029702 has to be fixed.
(0081235)
git   
2018-11-23 12:08   
Branch CR29177 has been updated forcibly by oan.

SHA-1: 868d09ac0d09d89d4213db7ba318b373f6a25c48
(0081319)
msv   
2018-11-30 17:58   
src/BVH/BVH_DistanceField.lxx
- Needs inclusion of OSD_Parallel.hxx.

src/BVH/BVH_LinearBuilder.hxx
- 267: put this line in condition (!aList.empty()).
(0081322)
oan   
2018-11-30 18:52   
(edited on: 2018-12-01 11:57)
Hi Mikhail,

what is the necessity to introduce the condition (!aList.empty())?
OSD_Parallel::ForEach should not perform iteration in case of empty lists as is.

As I know none of existing calls to OSD_Parallel::ForEach uses such extra checks.

As for #include, have you got compilation error using some version of visual studio?
If so, please specify, as far as according to 0029702 source code can be compiled without errors either on windows, linux and macos.

Regards.

(0081337)
msv   
2018-12-03 11:05   
Hi Oleg,
If you look at the implementation of the method ForEach you will see that it at least calls operator new(), and then performs not trivial things before evaluation of the list itself. Therefore it is needed to check the list before this call. In other parts of OCCT where ForEach is called the list is always not empty, and it is not so for this code.
(0081338)
msv   
2018-12-03 11:07   
As for #include, I did not compile, but I just see that this include is absent in the appointed file and in all files included by it. So, it can be a problem for some use.
(0081341)
git   
2018-12-03 11:40   
Branch CR29177 has been updated forcibly by oan.

SHA-1: 64c699f6a975a7007ed5542a1f8dfe3b79102e18
(0081343)
oan   
2018-12-03 11:42   
OK, done
(0081345)
msv   
2018-12-03 11:55   
Inclusion of BVH_Properties.hxx is extra here in BVH_LinearBuilder.hxx.

(0081352)
git   
2018-12-03 13:47   
Branch CR29177 has been updated forcibly by oan.

SHA-1: fd25ed94ab817fefc24b99bae9c9e64e4cd26784
(0081354)
oan   
2018-12-03 13:48   
Done.
(0081365)
msv   
2018-12-03 16:00   
Reviewed.
Please do not integrate without 0029702.
(0081381)
apn   
2018-12-04 11:33   
Tested with 0029702.