MantisBT - Open CASCADE
View Issue Details
0029702Open CASCADE[OCCT] OCCT:Foundation Classespublic2018-04-18 18:442018-12-18 22:24
oan 
apn 
normalminor 
verifiedfixed 
 
[OCCT] 7.4.0* 
Not needed
0029702: Foundation Classes - Introduce possibility to control parallel execution of BVH tools
BVH 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.
Not needed
No tags attached.
related to 0029177verified apn Foundation Classes - Adapt BVH package for use of OSD_Parallel 
Issue History
2018-04-18 18:44oanNew Issue
2018-04-18 18:44oanAssigned To => abv
2018-04-18 18:44oanRelationship addedrelated to 0029177
2018-04-19 01:17kgvSummaryIntroduce possibility to control parallel execution of BVH tools => Foundation Classes - Introduce possibility to control parallel execution of BVH tools
2018-04-19 01:17kgvDescription Updatedbug_revision_view_page.php?rev_id=18967#r18967
2018-08-03 11:49oanDescription Updatedbug_revision_view_page.php?rev_id=19651#r19651
2018-11-23 12:15gitNote Added: 0081236
2018-11-23 13:11gitNote Added: 0081237
2018-11-23 14:33oanNote Added: 0081239
2018-11-23 14:33oanAssigned Toabv => msv
2018-11-23 14:33oanStatusnew => resolved
2018-11-23 14:33oanSteps to Reproduce Updatedbug_revision_view_page.php?rev_id=20421#r20421
2018-11-23 17:58oanNote Edited: 0081239bug_revision_view_page.php?bugnote_id=81239#r20423
2018-11-30 17:25msvDescription Updatedbug_revision_view_page.php?rev_id=20444#r20444
2018-12-02 17:59kgvNote Added: 0081324
2018-12-03 11:18oanNote Added: 0081339
2018-12-03 11:28msvNote Added: 0081340
2018-12-03 11:28msvAssigned Tomsv => oan
2018-12-03 11:28msvStatusresolved => assigned
2018-12-03 11:41gitNote Added: 0081342
2018-12-03 11:58gitNote Added: 0081346
2018-12-03 12:14gitNote Added: 0081347
2018-12-03 12:34gitNote Added: 0081348
2018-12-03 13:44oanNote Added: 0081351
2018-12-03 13:44oanAssigned Tooan => msv
2018-12-03 13:44oanStatusassigned => resolved
2018-12-03 13:47gitNote Added: 0081353
2018-12-03 15:28msvNote Added: 0081360
2018-12-03 15:49gitNote Added: 0081361
2018-12-03 15:57msvNote Added: 0081362
2018-12-03 15:58msvAssigned Tomsv => oan
2018-12-03 15:58msvStatusresolved => assigned
2018-12-03 15:59msvNote Added: 0081363
2018-12-03 16:30kgvNote Added: 0081367
2018-12-03 16:42gitNote Added: 0081369
2018-12-03 16:53oanNote Added: 0081370
2018-12-03 16:53oanAssigned Tooan => msv
2018-12-03 16:53oanStatusassigned => resolved
2018-12-03 16:53oanNote Edited: 0081370bug_revision_view_page.php?bugnote_id=81370#r20451
2018-12-03 16:54oanNote Edited: 0081370bug_revision_view_page.php?bugnote_id=81370#r20452
2018-12-03 17:55msvNote Added: 0081372
2018-12-03 17:56msvAssigned Tomsv => oan
2018-12-03 17:56msvStatusresolved => assigned
2018-12-03 20:15gitNote Added: 0081375
2018-12-03 20:15oanNote Added: 0081376
2018-12-03 20:15oanAssigned Tooan => msv
2018-12-03 20:15oanStatusassigned => resolved
2018-12-04 09:59msvNote Added: 0081377
2018-12-04 09:59msvAssigned Tomsv => bugmaster
2018-12-04 09:59msvStatusresolved => reviewed
2018-12-04 11:33apnTest case number => Not needed
2018-12-04 11:33apnNote Added: 0081380
2018-12-04 11:33apnStatusreviewed => tested
2018-12-05 12:04apnNote Added: 0081385
2018-12-05 12:05gitNote Added: 0081386
2018-12-05 14:00oanNote Added: 0081388
2018-12-05 14:00oanAssigned Tobugmaster => msv
2018-12-05 14:00oanStatustested => assigned
2018-12-05 14:00oanStatusassigned => resolved
2018-12-05 15:12msvNote Added: 0081391
2018-12-05 15:12msvAssigned Tomsv => bugmaster
2018-12-05 15:12msvStatusresolved => reviewed
2018-12-05 18:22apnStatusreviewed => tested
2018-12-06 11:32apnNote Added: 0081406
2018-12-06 11:34apnAssigned Tobugmaster => oan
2018-12-06 11:34apnStatustested => feedback
2018-12-10 12:14gitNote Added: 0081431
2018-12-11 11:21apnNote Added: 0081451
2018-12-11 11:21apnAssigned Tooan => bugmaster
2018-12-11 11:21apnStatusfeedback => tested
2018-12-15 20:28apnChangeset attached => occt master 3c7a61ea
2018-12-15 20:28apnAssigned Tobugmaster => apn
2018-12-15 20:28apnStatustested => verified
2018-12-15 20:28apnResolutionopen => fixed
2018-12-18 21:52gitNote Added: 0081560
2018-12-18 22:24gitNote Added: 0081572

Notes
(0081236)
git   
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   
2018-11-23 13:11   
Branch CR29702 has been updated forcibly by oan.

SHA-1: 910c01b2cfa6490cf1ce3c3a8ef4fdb1033d023a
(0081239)
oan   
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   
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   
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   
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   
2018-12-03 11:41   
Branch CR29702 has been updated forcibly by oan.

SHA-1: f9574de0c547b2e4ea3774a9d62ecfc39d8269ab
(0081346)
git   
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   
2018-12-03 12:14   
Branch CR29702_pure has been updated forcibly by oan.

SHA-1: ca073fbadc94505ea4884d81429b339ab44bc94f
(0081348)
git   
2018-12-03 12:34   
Branch CR29702_pure has been updated forcibly by oan.

SHA-1: 50c806fd80f2c7d903bca9f365b3fbb136096f79
(0081351)
oan   
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   
2018-12-03 13:47   
Branch CR29702_pure has been updated forcibly by oan.

SHA-1: 6e936df5d569e04bf8a97dac7be3ef2f8b7f1e2b
(0081360)
msv   
2018-12-03 15:28   
The commit dc657e66e44077aaf3c1d4ee05354d70eba53ba7 is extra.
(0081361)
git   
2018-12-03 15:49   
Branch CR29702_pure has been updated forcibly by oan.

SHA-1: 4abe2342a979b34e586b2ef5e976a92173883ed0
(0081362)
msv   
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   
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   
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   
2018-12-03 16:42   
Branch CR29702_pure has been updated forcibly by oan.

SHA-1: 5c6c03d62af3b2cd6843e1c8391de45abd467ab0
(0081370)
oan   
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   
2018-12-03 17:55   
Misprint in the commit message: "exectuion".
(0081375)
git   
2018-12-03 20:15   
Branch CR29702_pure has been updated forcibly by oan.

SHA-1: 18184fcdbbdbff26ea1359c13735ebe5b513867b
(0081376)
oan   
2018-12-03 20:15   
Misprint has been corrected.
(0081377)
msv   
2018-12-04 09:59   
Reviewed.
(0081380)
apn   
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   
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   
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   
2018-12-05 14:00   
Dear Mikhail,

please have a look on the changes made due to limitations of old copilers.
(0081391)
msv   
2018-12-05 15:12   
Reviewed.
(0081406)
apn   
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   
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   
2018-12-11 11:21   
Warnings were eliminated.
(0081560)
git   
2018-12-18 21:52   
Branch CR29702 has been deleted by kgv.

SHA-1: f9574de0c547b2e4ea3774a9d62ecfc39d8269ab
(0081572)
git   
2018-12-18 22:24   
Branch CR29702_pure has been deleted by kgv.

SHA-1: eac68c17ea98dee68b2c164619a49adf7a6b11d0