MantisBT
Mantis Bug Tracker Workflow

View Issue Details Jump to Notes ] Related Changesets ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0029177Open CASCADE[OCCT] OCCT:Foundation Classespublic2017-10-03 14:482018-12-15 20:28
Reporteroan 
Assigned Toapn 
PrioritynormalSeverityminor 
StatusverifiedResolutionfixed 
PlatformOSOS Version
Product Version 
Target Version[OCCT] 7.4.0*Fixed in Version 
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
Attached Files

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

-  Notes
(0075507)
git (administrator)
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 (developer)
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 (administrator)
2018-04-18 12:26

Branch CR29177 has been updated forcibly by oan.

SHA-1: b2d0160a71c78066c71ce3563390d8ad3f038683
(0075511)
kgv (developer)
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 (developer)
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 (developer)
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 (developer)
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 (developer)
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 (developer)
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 (developer)
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 (developer)
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 (administrator)
2018-11-23 12:08

Branch CR29177 has been updated forcibly by oan.

SHA-1: 868d09ac0d09d89d4213db7ba318b373f6a25c48
(0081319)
msv (developer)
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 (developer)
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 (developer)
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 (developer)
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 (administrator)
2018-12-03 11:40

Branch CR29177 has been updated forcibly by oan.

SHA-1: 64c699f6a975a7007ed5542a1f8dfe3b79102e18
(0081343)
oan (developer)
2018-12-03 11:42

OK, done
(0081345)
msv (developer)
2018-12-03 11:55
edited on: 2018-12-03 11:55

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

(0081352)
git (administrator)
2018-12-03 13:47

Branch CR29177 has been updated forcibly by oan.

SHA-1: fd25ed94ab817fefc24b99bae9c9e64e4cd26784
(0081354)
oan (developer)
2018-12-03 13:48

Done.
(0081365)
msv (developer)
2018-12-03 16:00

Reviewed.
Please do not integrate without 0029702.
(0081381)
apn (administrator)
2018-12-04 11:33

Tested with 0029702.

- Related Changesets
occt: master da555fc2
Timestamp: 2018-04-18 08:21:00
Author: 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.
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 View Revisions
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 View Revisions
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 View Revisions
2018-04-18 13:45 kgv Note Edited: 0075515 View Revisions
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 View Revisions
2018-12-01 11:57 oan Note Edited: 0081322 View Revisions
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 View Revisions
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


Copyright © 2000 - 2018 MantisBT Team
Powered by Mantis Bugtracker