MantisBT
Mantis Bug Tracker Workflow

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

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

-  Notes
(0081236)
git (administrator)
2018-11-23 12:15

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.
(0081237)
git (administrator)
2018-11-23 13:11

Branch CR29702 has been updated forcibly by oan.

SHA-1: 910c01b2cfa6490cf1ce3c3a8ef4fdb1033d023a
(0081239)
oan (developer)
2018-11-23 14:33
edited on: 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 [^]

(0081324)
kgv (developer)
2018-12-02 17:59

-        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.
(0081339)
oan (developer)
2018-12-03 11:18

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.
(0081340)
msv (developer)
2018-12-03 11:28

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.
(0081342)
git (administrator)
2018-12-03 11:41

Branch CR29702 has been updated forcibly by oan.

SHA-1: f9574de0c547b2e4ea3774a9d62ecfc39d8269ab
(0081346)
git (administrator)
2018-12-03 11:58

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.
(0081347)
git (administrator)
2018-12-03 12:14

Branch CR29702_pure has been updated forcibly by oan.

SHA-1: ca073fbadc94505ea4884d81429b339ab44bc94f
(0081348)
git (administrator)
2018-12-03 12:34

Branch CR29702_pure has been updated forcibly by oan.

SHA-1: 50c806fd80f2c7d903bca9f365b3fbb136096f79
(0081351)
oan (developer)
2018-12-03 13:44

Please review in couple with 0029177.

http://jenkins-test-12.nnov.opencascade.com/view/CR29702_pure-master-OAN/view/COMPARE/ [^]
(0081353)
git (administrator)
2018-12-03 13:47

Branch CR29702_pure has been updated forcibly by oan.

SHA-1: 6e936df5d569e04bf8a97dac7be3ef2f8b7f1e2b
(0081360)
msv (developer)
2018-12-03 15:28

The commit dc657e66e44077aaf3c1d4ee05354d70eba53ba7 is extra.
(0081361)
git (administrator)
2018-12-03 15:49

Branch CR29702_pure has been updated forcibly by oan.

SHA-1: 4abe2342a979b34e586b2ef5e976a92173883ed0
(0081362)
msv (developer)
2018-12-03 15:57

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.
(0081363)
msv (developer)
2018-12-03 15:59

With this patch, there will be change of behavior with HAVE_TBB - it will be no more parallelized.
(0081367)
kgv (developer)
2018-12-03 16:30

> @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.
(0081369)
git (administrator)
2018-12-03 16:42

Branch CR29702_pure has been updated forcibly by oan.

SHA-1: 5c6c03d62af3b2cd6843e1c8391de45abd467ab0
(0081370)
oan (developer)
2018-12-03 16:53
edited on: 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.

(0081372)
msv (developer)
2018-12-03 17:55

Misprint in the commit message: "exectuion".
(0081375)
git (administrator)
2018-12-03 20:15

Branch CR29702_pure has been updated forcibly by oan.

SHA-1: 18184fcdbbdbff26ea1359c13735ebe5b513867b
(0081376)
oan (developer)
2018-12-03 20:15

Misprint has been corrected.
(0081377)
msv (developer)
2018-12-04 09:59

Reviewed.
(0081380)
apn (administrator)
2018-12-04 11:33

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
(0081385)
apn (administrator)
2018-12-05 12:04

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
(0081386)
git (administrator)
2018-12-05 12:05

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)

(0081388)
oan (developer)
2018-12-05 14:00

Dear Mikhail,

please have a look on the changes made due to limitations of old copilers.
(0081391)
msv (developer)
2018-12-05 15:12

Reviewed.
(0081406)
apn (administrator)
2018-12-06 11:32

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
(0081431)
git (administrator)
2018-12-10 12:14

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.

(0081451)
apn (administrator)
2018-12-11 11:21

Warnings were eliminated.

- Related Changesets
occt: master 3c7a61ea
Timestamp: 2018-12-03 12:46:48
Author: 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)
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 View Revisions
2018-08-03 11:49 oan Description Updated View Revisions
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 View Revisions
2018-11-23 17:58 oan Note Edited: 0081239 View Revisions
2018-11-30 17:25 msv Description Updated View Revisions
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 View Revisions
2018-12-03 16:54 oan Note Edited: 0081370 View Revisions
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


Copyright © 2000 - 2018 MantisBT Team
Powered by Mantis Bugtracker