View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0029177Open CASCADEOCCT:Foundation Classespublic2017-10-03 14:482018-12-18 21:52
Reporteroan Assigned Toapn  
PrioritynormalSeverityminor 
Status closedResolutionfixed 
Target Version7.4.0Fixed in Version7.4.0 
Summary0029177: Foundation Classes - Adapt BVH package for use of OSD_Parallel
DescriptionCurrently 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.
Steps To ReproduceNothing to do
TagsNo tags attached.
Test case numberNot needed

Relationships

related to 0029702 closedapn Foundation Classes - Introduce possibility to control parallel execution of BVH tools 
child of 0028931 closedbugmaster Eliminate dependency from TBB in OSD_Parallel header 

Activities

git

2018-04-18 11:21

administrator   ~0075507

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.

oan

2018-04-18 11:27

developer   ~0075508

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).

git

2018-04-18 12:26

administrator   ~0075509

Branch CR29177 has been updated forcibly by oan.

SHA-1: b2d0160a71c78066c71ce3563390d8ad3f038683

kgv

2018-04-18 12:31

developer   ~0075511

Last edited: 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).

oan

2018-04-18 13:01

developer   ~0075514

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.

kgv

2018-04-18 13:44

developer   ~0075515

Last edited: 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).

oan

2018-04-18 14:07

developer   ~0075518

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.

kgv

2018-04-18 14:20

developer   ~0075520

> 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".

oan

2018-04-27 15:23

developer   ~0075720

Please review.

Test results are available by the following link:

http://jenkins-test-11.nnov.opencascade.com/view/CR29177-master-oan/view/COMPARE/

oan

2018-07-27 16:57

developer   ~0078108

Dear Andrey,

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

Regards,
Oleg.

msv

2018-08-03 11:36

developer   ~0078344

>But this issue will be a prerequisites for integration of this patch into "master".
First the bug 0029702 has to be fixed.

git

2018-11-23 12:08

administrator   ~0081235

Branch CR29177 has been updated forcibly by oan.

SHA-1: 868d09ac0d09d89d4213db7ba318b373f6a25c48

msv

2018-11-30 17:58

developer   ~0081319

src/BVH/BVH_DistanceField.lxx
- Needs inclusion of OSD_Parallel.hxx.

src/BVH/BVH_LinearBuilder.hxx
- 267: put this line in condition (!aList.empty()).

oan

2018-11-30 18:52

developer   ~0081322

Last edited: 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.

msv

2018-12-03 11:05

developer   ~0081337

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.

msv

2018-12-03 11:07

developer   ~0081338

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.

git

2018-12-03 11:40

administrator   ~0081341

Branch CR29177 has been updated forcibly by oan.

SHA-1: 64c699f6a975a7007ed5542a1f8dfe3b79102e18

oan

2018-12-03 11:42

developer   ~0081343

OK, done

msv

2018-12-03 11:55

developer   ~0081345

Last edited: 2018-12-03 11:55

 Inclusion of BVH_Properties.hxx is extra here in BVH_LinearBuilder.hxx.

git

2018-12-03 13:47

administrator   ~0081352

Branch CR29177 has been updated forcibly by oan.

SHA-1: fd25ed94ab817fefc24b99bae9c9e64e4cd26784

oan

2018-12-03 13:48

developer   ~0081354

Done.

msv

2018-12-03 16:00

developer   ~0081365

Reviewed.
Please do not integrate without 0029702.

apn

2018-12-04 11:33

administrator   ~0081381

Tested with 0029702.

git

2018-12-18 21:52

administrator   ~0081557

Branch CR29177 has been deleted by kgv.

SHA-1: fd25ed94ab817fefc24b99bae9c9e64e4cd26784

Related Changesets

occt: master da555fc2

2018-04-18 08:21:00

oan


Committer: apn Details Diff		 0029177: Foundation Classes - 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.		 Affected Issues
0029177
mod - src/BVH/BVH_DistanceField.lxx Diff File
mod - src/BVH/BVH_LinearBuilder.hxx Diff File
mod - src/BVH/BVH_RadixSorter.hxx Diff File

Issue History

Date Modified Username Field Change
2017-10-03 14:48 oan New Issue
2017-10-03 14:48 oan Assigned To => abv
2017-10-03 14:48 oan Relationship added child of 0028931
2018-04-18 11:21 git Note Added: 0075507
2018-04-18 11:27 oan Note Added: 0075508
2018-04-18 11:27 oan Status new => resolved
2018-04-18 11:27 oan Steps to Reproduce Updated
2018-04-18 12:20 kgv Summary Adapt BVH package for use of OSD_Parallel => Foundation Classes - Adapt BVH package for use of OSD_Parallel
2018-04-18 12:26 git Note Added: 0075509
2018-04-18 12:31 kgv Note Added: 0075511
2018-04-18 12:32 kgv Note Edited: 0075511
2018-04-18 13:01 oan Note Added: 0075514
2018-04-18 13:44 kgv Note Added: 0075515
2018-04-18 13:44 kgv Note Edited: 0075515
2018-04-18 13:45 kgv Note Edited: 0075515
2018-04-18 14:07 oan Note Added: 0075518
2018-04-18 14:20 kgv Note Added: 0075520
2018-04-18 18:44 oan Relationship added related to 0029702
2018-04-27 15:23 oan Note Added: 0075720
2018-07-27 16:57 oan Note Added: 0078108
2018-08-02 15:38 oan Assigned To abv => msv
2018-08-03 11:36 msv Note Added: 0078344
2018-11-23 12:08 git Note Added: 0081235
2018-11-30 17:58 msv Note Added: 0081319
2018-11-30 17:58 msv Assigned To msv => oan
2018-11-30 17:58 msv Status resolved => assigned
2018-11-30 18:52 oan Note Added: 0081322
2018-11-30 18:52 oan Assigned To oan => msv
2018-11-30 18:52 oan Status assigned => feedback
2018-11-30 22:49 oan Note Edited: 0081322
2018-12-01 11:57 oan Note Edited: 0081322
2018-12-03 11:05 msv Note Added: 0081337
2018-12-03 11:05 msv Assigned To msv => oan
2018-12-03 11:07 msv Note Added: 0081338
2018-12-03 11:40 git Note Added: 0081341
2018-12-03 11:42 oan Note Added: 0081343
2018-12-03 11:42 oan Assigned To oan => msv
2018-12-03 11:42 oan Status feedback => resolved
2018-12-03 11:55 msv Note Added: 0081345
2018-12-03 11:55 msv Assigned To msv => oan
2018-12-03 11:55 msv Status resolved => assigned
2018-12-03 11:55 msv Note Edited: 0081345
2018-12-03 13:47 git Note Added: 0081352
2018-12-03 13:48 oan Note Added: 0081354
2018-12-03 13:48 oan Assigned To oan => msv
2018-12-03 13:48 oan Status assigned => resolved
2018-12-03 16:00 msv Note Added: 0081365
2018-12-03 16:00 msv Assigned To msv => bugmaster
2018-12-03 16:00 msv Status resolved => reviewed
2018-12-04 11:33 apn Test case number => Not needed
2018-12-04 11:33 apn Note Added: 0081381
2018-12-04 11:33 apn Status reviewed => tested
2018-12-15 20:28 apn Changeset attached => occt master da555fc2
2018-12-15 20:28 apn Assigned To bugmaster => apn
2018-12-15 20:28 apn Status tested => verified
2018-12-15 20:28 apn Resolution open => fixed
2018-12-18 21:52 git Note Added: 0081557