View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0029702Open CASCADEOCCT:Foundation Classespublic2018-04-18 18:442018-12-18 22:24
Reporteroan Assigned Toapn  
PrioritynormalSeverityminor 
Status closedResolutionfixed 
Target Version7.4.0Fixed in Version7.4.0 
Summary0029702: Foundation Classes - Introduce possibility to control parallel execution of BVH tools
DescriptionBVH tools like BVH_RadixSorter are always executed in parallel when HAVE_TBB is defined.
It is suggested to implement Boolean flag to control parallel execution as it is done for BOP, BRepMesh, etc.
By default this option should be disabled (0029177:0075515).

BOPAlgo_Options class should be taken as an example of common approach.
Steps To ReproduceNot needed
TagsNo tags attached.
Test case numberNot needed

Relationships

related to 0029177 closedapn Foundation Classes - Adapt BVH package for use of OSD_Parallel 

Activities

git

2018-11-23 12:15

administrator   ~0081236

Branch CR29702 has been created by oan.

SHA-1: 58807f315fe9525bc41dbefae584605276a1ea54


Detailed log of new commits:

Author: oan
Date: Fri Nov 23 12:00:05 2018 +0300

    0029702: Foundation Classes - Introduce possibility to control parallel execution of BVH tools
    
    Static methods IsParallel() and SetParallel(const Standard_Boolean isParallel) have been added to BVH_Properties class to control parallel execution globally (disabled by default).
    New Draw command bvhparallel has been registered in context of BRepTest_BasicCommands.

git

2018-11-23 13:11

administrator   ~0081237

Branch CR29702 has been updated forcibly by oan.

SHA-1: 910c01b2cfa6490cf1ce3c3a8ef4fdb1033d023a

oan

2018-11-23 14:33

developer   ~0081239

Last edited: 2018-11-23 17:58

 Please review in couple with 0029177.

Test results:
http://occt-tests/CR29702-master-OAN-OCCT/Debian80-64/diff_summary.html
http://occt-tests/CR29702-master-OAN-OCCT/Windows-64-VC14/diff_summary.html

kgv

2018-12-02 17:59

developer   ~0081324 

-        OSD_Parallel::ForEach (std::begin (aSplits), std::end (aSplits), Functor ());
+        OSD_Parallel::ForEach (std::begin (aSplits), std::end (aSplits), Functor (), !BVH_Properties::IsParallel());

A global flag as the only way to control algorithm execution is the misconception, which we are trying to weed out from OCCT, not to add a new one.

BVH_Properties is a wrong place for a global properties, since this class is defined to be an interface for per-object properties.

Moreover, BVH contains several collections and algorithms are used in different places (BVH builders are used in 3D Viewer Selection, Ray-Tracing, Frustum culling, BRepExtrema and probably in other places) - defining the single global flag within BVH itself might be awkward, because in different scenarios such an option might be managed in place of specific algorithm differently.

Specifically, 3D Viewer Selection routines has conception of default builder, so that application might set another BVH builder or modify properties of default BVH builder - so that instead of a global Boolean flag, application is expected to change a global BVH builder object.

BVH_RadixSorter is used only by BVH_LinearBuilder, so that BVH_LinearBuilder will initialize BVH_RadixSorter parallelization flag basing on BVH_LinearBuilder flag.

oan

2018-12-03 11:18

developer   ~0081339

Dear Kirill,

So, your direct suggestions are... What?


>> BVH_Properties is a wrong place for a global properties

It is taken by the name which is most suitable amongst other classes without introduction of new one for a single reason only. And if we can run BOP in parallel using direct calls to algortihm, BVH is mostly used implicitly and we have no handle to control its execution, e.g. on the level of visualization. Note that BRepMesh is also called implicitly from visualization part and uses global static variable to control it parallel execution without necessity to recompile entire OCCT.

>> different scenarios such an option might be managed in place of specific algorithm differently

As I understand currently there is no option to control it in this way at all and parallel execution is enabled persistently when tbb is used. Do you really have such a need?

>> Specifically, 3D Viewer Selection routines has conception of default builder, so that application might set another BVH builder or modify properties of default BVH builder - so that instead of a global Boolean flag, application is expected to change a global BVH builder object.

Excuse me, but from my view I don't see the dependency between default BVH builder and a global Boolean flag. If algorithm changes builder to another one, it is builder's option (by implementation) to run itself in parallel. If builder does not implement such option, influence of global flag will be minimal.

I would ask you to avoid pieses of implicit advice, as far as it is difficult to fulfil such remarks due to absent proposals of solutions.

msv

2018-12-03 11:28

developer   ~0081340

I agree with Kirill.
Additionally, we do not need to add a Draw command to control parallelization of BVH, because it is too low-level tool, and unlike for example Boolean operation the user cannot run it directly.
All we need is just to add Boolean flag in those classes that make parallel loops. For builders, I propose to add such flag on the level of the base class BVH_BuilderTransient.

git

2018-12-03 11:41

administrator   ~0081342

Branch CR29702 has been updated forcibly by oan.

SHA-1: f9574de0c547b2e4ea3774a9d62ecfc39d8269ab

git

2018-12-03 11:58

administrator   ~0081346

Branch CR29702_pure has been created by oan.

SHA-1: 0fd488803276205d6e9ec1ff29828606f6844fca


Detailed log of new commits:

Author: oan
Date: Mon Dec 3 11:56:11 2018 +0300

    0029702: Foundation Classes - Introduce possibility to control parallel execution of BVH tools
    
    Methods IsParallel() and SetParallel() have been added to BVH_Sorter, BVH_DistanceField and BVH_BuilderTransient to control parallel exectuion on low-level.

git

2018-12-03 12:14

administrator   ~0081347

Branch CR29702_pure has been updated forcibly by oan.

SHA-1: ca073fbadc94505ea4884d81429b339ab44bc94f

git

2018-12-03 12:34

administrator   ~0081348

Branch CR29702_pure has been updated forcibly by oan.

SHA-1: 50c806fd80f2c7d903bca9f365b3fbb136096f79

oan

2018-12-03 13:44

developer   ~0081351

Please review in couple with 0029177.

http://jenkins-test-12.nnov.opencascade.com/view/CR29702_pure-master-OAN/view/COMPARE/

git

2018-12-03 13:47

administrator   ~0081353

Branch CR29702_pure has been updated forcibly by oan.

SHA-1: 6e936df5d569e04bf8a97dac7be3ef2f8b7f1e2b

msv

2018-12-03 15:28

developer   ~0081360

The commit dc657e66e44077aaf3c1d4ee05354d70eba53ba7 is extra.

git

2018-12-03 15:49

administrator   ~0081361

Branch CR29702_pure has been updated forcibly by oan.

SHA-1: 4abe2342a979b34e586b2ef5e976a92173883ed0

msv

2018-12-03 15:57

developer   ~0081362

src/BVH/BVH_LinearBuilder.hxx
- 318: BVH_RadixSorter is created without passing IsParallel flag.
- Inclusion of BVH_Properties.hxx is extra.

@kgv it is needed to provide possibility to set IsParallel flag in all places where BVH_LinearBuilder is used. There are 4 places in OCCT and 1 place in occt-products.

msv

2018-12-03 15:59

developer   ~0081363

With this patch, there will be change of behavior with HAVE_TBB - it will be no more parallelized.

kgv

2018-12-03 16:30

developer   ~0081367

> @kgv it is needed to provide possibility to set IsParallel flag
> in all places where BVH_LinearBuilder is used.
> There are 4 places in OCCT and 1 place in occt-products.
In my experience, performance boost from parallelization of BVH_LinearBuilder was very poor (20% on 4 cores CPU, but this is just a recall - should be checked), so that I don't consider it practically helpful at application level; though probably this boost is not that small to be considered. So that in places, where performance of builder should be reduced, it is better building several trees in parallel considering poor quality of result (Select3D_SensitivePrimitiveArray as an example to work with meshes of millions triangles). Implicit parallelization inside BVH_LinearBuilder is a redundant burden in this approach (calling several BVH_LinearBuilder in parallel for several trees).

Currently, I'm hesitating if it worth bothering with providing multi-threading flag to all BVH_LinearBuilder usage places, or just add it to BVH_LinearBuilder class and consider its usage in future.

git

2018-12-03 16:42

administrator   ~0081369

Branch CR29702_pure has been updated forcibly by oan.

SHA-1: 5c6c03d62af3b2cd6843e1c8391de45abd467ab0

oan

2018-12-03 16:53

developer   ~0081370

Last edited: 2018-12-03 16:54

 The last commit passes parallel flag to BVH_RadixSorter.

According to Kirill's remarks modificaitons related to parallelization of BVH in OCCT and Products were not made.

Reports above say that impact is indifferent comparing to current master.

msv

2018-12-03 17:55

developer   ~0081372

Misprint in the commit message: "exectuion".

git

2018-12-03 20:15

administrator   ~0081375

Branch CR29702_pure has been updated forcibly by oan.

SHA-1: 18184fcdbbdbff26ea1359c13735ebe5b513867b

oan

2018-12-03 20:15

developer   ~0081376

Misprint has been corrected.

msv

2018-12-04 09:59

developer   ~0081377

Reviewed.

apn

2018-12-04 11:33

administrator   ~0081380

Combination -
OCCT branch : CR29702_pure SHA - 50c806fd80f2c7d903bca9f365b3fbb136096f79
Products branch : master SHA - 7a17ab043115825c2601b4333d82bb3148f6c60d
was compiled on Linux, MacOS and Windows platforms and tested in optimize mode.

Number of compiler warnings:
No new/fixed warnings

Regressions/Differences/Improvements:
No regressions/differences

CPU differences:
Debian80-64:
OCCT
Total CPU difference: 16311.849999999982 / 16340.800000000068 [-0.18%]
Products
Total CPU difference: 7044.090000000036 / 7049.540000000028 [-0.08%]
Windows-64-VC14:
OCCT
Total CPU difference: 17808.25 / 17890.984375 [-0.46%]
Products
Total CPU difference: 8547.8125 / 8537.828125 [+0.12%]

Image differences :
No differences that require special attention

Memory differences :
No differences that require special attention

apn

2018-12-05 12:04

administrator   ~0081385

There are following compilation errors in ​BVH_RadixSorter.hxx in CR29702_pure on platforms CentOS64-64-opt, Windows-32-vc9-opt, Windows-64-vc9-opt (old compilers):

C:/builds/IR-WEEK49_IR-WEEK49/Windows-32-VC9-opt/OCCT/src/BVH/BVH_RadixSorter.hxx(146) : error C2039: 'begin' : is not a member of 'std'
C:/builds/IR-WEEK49_IR-WEEK49/Windows-32-VC9-opt/OCCT/src/BVH/BVH_RadixSorter.hxx(146) : error C2039: 'end' : is not a member of 'std'
C:/builds/IR-WEEK49_IR-WEEK49/Windows-32-VC9-opt/OCCT/src/BVH/BVH_RadixSorter.hxx(146) : error C3861: 'begin': identifier not found
C:/builds/IR-WEEK49_IR-WEEK49/Windows-32-VC9-opt/OCCT/src/BVH/BVH_RadixSorter.hxx(146) : error C3861: 'end': identifier not found

git

2018-12-05 12:05

administrator   ~0081386

Branch CR29702_pure has been updated by oan.

SHA-1: 6a6313642bc48320585dc832ee1fd0cc1f090824


Detailed log of new commits:

Author: oan
Date: Wed Dec 5 12:02:11 2018 +0300

    Fix compilation errors for old compilers without support of c++11 (std::begin, std::end)

oan

2018-12-05 14:00

developer   ~0081388

Dear Mikhail,

please have a look on the changes made due to limitations of old copilers.

msv

2018-12-05 15:12

developer   ~0081391

Reviewed.

apn

2018-12-06 11:32

administrator   ~0081406

There is new warning on all windows platforms except VS2017 (Windows-64-vc10, Windows-64-vc11, Windows-64-vc12, Windows-32-vc10, etc):

BVH_RadixSorter.hxx:133, MSBuild, Priority: Normal
'BVH::RadixSorter::Functor' : assignment operator could not be generated

git

2018-12-10 12:14

administrator   ~0081431

Branch CR29702_pure has been updated by oan.

SHA-1: eac68c17ea98dee68b2c164619a49adf7a6b11d0


Detailed log of new commits:

Author: oan
Date: Mon Dec 10 12:11:21 2018 +0300

    Fix compilation warning.

apn

2018-12-11 11:21

administrator   ~0081451

Warnings were eliminated.

git

2018-12-18 21:52

administrator   ~0081560

Branch CR29702 has been deleted by kgv.

SHA-1: f9574de0c547b2e4ea3774a9d62ecfc39d8269ab

git

2018-12-18 22:24

administrator   ~0081572

Branch CR29702_pure has been deleted by kgv.

SHA-1: eac68c17ea98dee68b2c164619a49adf7a6b11d0

Related Changesets

occt: master 3c7a61ea

2018-12-03 12:46:48

oan


Committer: apn Details Diff		 0029702: Foundation Classes - Introduce possibility to control parallel execution of BVH tools

Methods IsParallel() and SetParallel() have been added to BVH_Sorter, BVH_DistanceField and BVH_BuilderTransient to control parallel execution on low-level.
Fix compilation errors for old compilers without support of c++11 (std::begin, std::end)		 Affected Issues
0029702
mod - src/BVH/BVH_Builder.hxx Diff File
mod - src/BVH/BVH_DistanceField.hxx Diff File
mod - src/BVH/BVH_DistanceField.lxx Diff File
mod - src/BVH/BVH_LinearBuilder.hxx Diff File
mod - src/BVH/BVH_RadixSorter.hxx Diff File
mod - src/BVH/BVH_Sorter.hxx Diff File

Issue History

Date Modified Username Field Change
2018-04-18 18:44 oan New Issue
2018-04-18 18:44 oan Assigned To => abv
2018-04-18 18:44 oan Relationship added related to 0029177
2018-04-19 01:17 kgv Summary Introduce possibility to control parallel execution of BVH tools => Foundation Classes - Introduce possibility to control parallel execution of BVH tools
2018-04-19 01:17 kgv Description Updated
2018-08-03 11:49 oan Description Updated
2018-11-23 12:15 git Note Added: 0081236
2018-11-23 13:11 git Note Added: 0081237
2018-11-23 14:33 oan Note Added: 0081239
2018-11-23 14:33 oan Assigned To abv => msv
2018-11-23 14:33 oan Status new => resolved
2018-11-23 14:33 oan Steps to Reproduce Updated
2018-11-23 17:58 oan Note Edited: 0081239
2018-11-30 17:25 msv Description Updated
2018-12-02 17:59 kgv Note Added: 0081324
2018-12-03 11:18 oan Note Added: 0081339
2018-12-03 11:28 msv Note Added: 0081340
2018-12-03 11:28 msv Assigned To msv => oan
2018-12-03 11:28 msv Status resolved => assigned
2018-12-03 11:41 git Note Added: 0081342
2018-12-03 11:58 git Note Added: 0081346
2018-12-03 12:14 git Note Added: 0081347
2018-12-03 12:34 git Note Added: 0081348
2018-12-03 13:44 oan Note Added: 0081351
2018-12-03 13:44 oan Assigned To oan => msv
2018-12-03 13:44 oan Status assigned => resolved
2018-12-03 13:47 git Note Added: 0081353
2018-12-03 15:28 msv Note Added: 0081360
2018-12-03 15:49 git Note Added: 0081361
2018-12-03 15:57 msv Note Added: 0081362
2018-12-03 15:58 msv Assigned To msv => oan
2018-12-03 15:58 msv Status resolved => assigned
2018-12-03 15:59 msv Note Added: 0081363
2018-12-03 16:30 kgv Note Added: 0081367
2018-12-03 16:42 git Note Added: 0081369
2018-12-03 16:53 oan Note Added: 0081370
2018-12-03 16:53 oan Assigned To oan => msv
2018-12-03 16:53 oan Status assigned => resolved
2018-12-03 16:53 oan Note Edited: 0081370
2018-12-03 16:54 oan Note Edited: 0081370
2018-12-03 17:55 msv Note Added: 0081372
2018-12-03 17:56 msv Assigned To msv => oan
2018-12-03 17:56 msv Status resolved => assigned
2018-12-03 20:15 git Note Added: 0081375
2018-12-03 20:15 oan Note Added: 0081376
2018-12-03 20:15 oan Assigned To oan => msv
2018-12-03 20:15 oan Status assigned => resolved
2018-12-04 09:59 msv Note Added: 0081377
2018-12-04 09:59 msv Assigned To msv => bugmaster
2018-12-04 09:59 msv Status resolved => reviewed
2018-12-04 11:33 apn Test case number => Not needed
2018-12-04 11:33 apn Note Added: 0081380
2018-12-04 11:33 apn Status reviewed => tested
2018-12-05 12:04 apn Note Added: 0081385
2018-12-05 12:05 git Note Added: 0081386
2018-12-05 14:00 oan Note Added: 0081388
2018-12-05 14:00 oan Assigned To bugmaster => msv
2018-12-05 14:00 oan Status tested => assigned
2018-12-05 14:00 oan Status assigned => resolved
2018-12-05 15:12 msv Note Added: 0081391
2018-12-05 15:12 msv Assigned To msv => bugmaster
2018-12-05 15:12 msv Status resolved => reviewed
2018-12-05 18:22 apn Status reviewed => tested
2018-12-06 11:32 apn Note Added: 0081406
2018-12-06 11:34 apn Assigned To bugmaster => oan
2018-12-06 11:34 apn Status tested => feedback
2018-12-10 12:14 git Note Added: 0081431
2018-12-11 11:21 apn Note Added: 0081451
2018-12-11 11:21 apn Assigned To oan => bugmaster
2018-12-11 11:21 apn Status feedback => tested
2018-12-15 20:28 apn Changeset attached => occt master 3c7a61ea
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: 0081560
2018-12-18 22:24 git Note Added: 0081572