View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0028931Open CASCADEOCCT:Foundation Classespublic2017-07-21 09:572018-06-29 21:19
Reporteroan Assigned Tobugmaster  
PrioritynormalSeverityfeature 
Status closedResolutionfixed 
Target Version7.3.0Fixed in Version7.3.0 
Summary0028931: Eliminate dependency from TBB in OSD_Parallel header
DescriptionIt is suggested to consider possibility of refactoring OSD_Parallel.hxx in order to move its implementation, thus dependency on TBB (or another 3rd-party parallel tool) to source file. This suggestion is caused by impossibility to use OCCT as a 3rd-party product via CMake system without necessity to specify paths to its 3rd-parties, including TBB headers.
Steps To ReproduceNone
TagsNo tags attached.
Test case numberNot required

Relationships

related to 0028915 closedkgv Open CASCADE Configuration, Font_BRepFont - do not include FreeType headers within OCCT headers 
parent of 0029177 closedapn Open CASCADE Foundation Classes - Adapt BVH package for use of OSD_Parallel 
related to 0026365 closedbugmaster Open CASCADE Optimization of work of OSD_Parallel class members for GeomLib_CheckCurveOnSurface 

Activities

kgv

2017-07-21 10:04

developer   ~0068525

> including TBB headers.
Also including TBB libraries.

oan

2017-07-21 10:19

developer   ~0068527

At least headers.

Please correct me if I am wrong, OCCT supports option to install tbb libraries along with OCCT, but not headers. When you put #include <OSD_Parallel.hxx> to some place outside OCCT environment where OCCT is used as 3rd-party, you immediately obtain compilation error because of absent "tbb/parallel_for.h" and so on.

abv

2017-07-26 14:41

manager   ~0068725

Oleg, the problem is that TBB is template library, thus you cannot incapsulate it in object code. If you need to use TBB directly (or OSD_Parallel in "TBB" mode), you must have TBB available in your environment. If you do not want to use TBB, tweak your CMake scripts to undefine macro HAVE_TBB if it has been occasionally inherited from OCCT -- this should help to avoid this dependency.

oan

2017-07-27 18:01

developer   ~0068807

Last edited: 2017-07-27 18:03

 DC,

I clearly understand that the intention to transform constructive dialog to plain jokes and statements of impossibility of such feature is done to motivate its implementation, as a prove of possibility, as quick as possible.

Regarding the point that TBB is template library, I recall existence of STL which is also template library, but could be used in *.CPP file while it is not mentioned in *.H, so not required by project the library is used by.

If we undefined HAVE_TBB macro in some project manually, we will produce two versions of OSD_Parallel, one for internals of OCCT featured by high-end TBB and another one for project the OCCT serves for, which uses primitive implementation of parallel tools through OSD_Thread.

IMHO, the simplier and flexible OCCT is, the better user experience. So, having TBB libraries installed along the OCCT in couple with modified OSD_Parallel tools, users will have possibility to use TBB-featured tools without necessity of specification long and curly paths to its includes.

In addition, requested feature could be easily implemented using few interfaces and so-called wrappers.

I just leave it here, please have a look when you had time...

git

2017-07-27 18:01

administrator   ~0068808

Branch CR28931 has been created by oan.

SHA-1: a6d13483016198354fa621aa945cd323bd481e31


Detailed log of new commits:

Author: oan
Date: Thu Jul 27 17:21:03 2017 +0300

    0028931: Eliminate dependency from TBB in OSD_Parallel header
    
    Move implementation of parallel for and for each to CXX.
    Introduction of wrapper over iterators along with Functor interface.

abv

2017-07-31 09:25

manager   ~0068878

Oleg, I had no intent to make jokes, my statement was that template nature of the implementation prevents such change. Sorry I did not recognize your idea to use polymorphism instead of templates.

In general I like your idea, however the patch needs to be elaborated and tested, especially for performance. What I believe should improved are:

- instead of making operators like (), + etc. virtual, it is better to make virtual methods with normal names, and define those operators as inlines calling these methods;

- make better incapsulation for virtual interface of iterators, to avoid dynamic casts and explicit instantiation of wrappers in every call;

- revise / provide documentation, including upgrade guide (unless we can just keep the same interface and requirements as before).

Note that when you override virtual function in descendant class, you should use Standard_OVERRIDE specifier to avoid compiler warnings.

The major question now is -- do you need this for the project and in OCCT 7.2.0? If not, I suppose it can wait till the end of summer. If yes, please contact Kirill for further reviews.

oan

2017-07-31 11:29

developer   ~0068882

Dear Andrey,

thank you for the remarks, they are important, because branch CR28931 is a draft for now and shows feasibility only. Regarding the integration, I suppose there is no urgency for this patch, so target version was set to 7.2.1.

However, I would like to clarify some points through hints you have given:

>>instead of making operators like (), + etc. virtual...
IMHO, this will introduce unnecessary complexity to simple data structures like iterator-like wrappers where operators ==, !=, ++ etc. have well-defined behaviour. In addition, I have had a look on NCollection_StlIterator during prototyping and there were no such aliasing, so I used the similar approach.

>> to avoid dynamic casts and explicit instantiation of wrappers in every call.
Sorry, I afraid I have not got the idea right. Could you please provide some details regarding suggested approach, probably with examples.

>>revise / provide documentation, including upgrade guide
Sure, documentation should be updated.

>>you should use Standard_OVERRIDE specifier to avoid compiler warnings.
Yes, thanks, I have missed it because VS does not report such things.

BR,
Oleg.

git

2017-09-15 12:55

administrator   ~0070582

Branch CR28931_1 has been created by oan.

SHA-1: 85703dd85f6b3a39a8404d9f4e64b3c88bf3f8f2


Detailed log of new commits:

Author: oan
Date: Fri Sep 15 12:51:01 2017 +0300

    More suitable API for functors

Author: oan
Date: Thu Sep 14 17:59:48 2017 +0300

    Refactor parallel tools in order to fit old API

Author: oan
Date: Thu Jul 27 17:21:03 2017 +0300

    0028931: Eliminate dependency from TBB in OSD_Parallel header
    
    Move implementation of parallel for and for each to CXX.
    Introduction of wrapper over iterators along with Functor interface.

git

2017-09-15 17:27

administrator   ~0070600

Branch CR28931_1 has been updated forcibly by oan.

SHA-1: 271c93526f74bb5234f2cf863901c4d196d55f73

oan

2017-09-15 17:30

developer   ~0070601

Dear Andrey,

could you please have a look on updated version?
Latest modifications are made in order to keep new API of OSD_Parallel as close as possible to existing one.

git

2017-09-17 20:50

administrator   ~0070618

Branch CR28931_2 has been created by abv.

SHA-1: 6a653159cf28141ac66fdf9acad9ad2478d848c7


Detailed log of new commits:

Author: oan
Date: Fri Sep 15 17:22:30 2017 +0300

    0028931: Eliminate dependency from TBB in OSD_Parallel header
    
    Implementation of methods OSD_Parallel::For() and ForEach is moved to CXX.
    Runtime polymorphism (virtual methods) is used to hide implementation (TBB or threads-based).

git

2017-09-19 18:56

administrator   ~0070700

Branch CR28931_2 has been updated by oan.

SHA-1: 005072873cc623ace468859dd1a4b0d57031e5e4


Detailed log of new commits:

Author: oan
Date: Tue Sep 19 18:56:01 2017 +0300

    use auto_ptr instead of shared_ptr

Author: oan
Date: Tue Sep 19 18:51:29 2017 +0300

    Move TBB and OSD_Thread implementations to separate files

Author: oan
Date: Tue Sep 19 17:36:52 2017 +0300

    Move auxiliary classes Task and Range to CXX

git

2017-09-20 12:25

administrator   ~0070708

Branch CR28931_2 has been updated by oan.

SHA-1: 6c2d095d36de1e6f072a3aeb4e552b19ff358c7f


Detailed log of new commits:

Author: oan
Date: Wed Sep 20 12:25:10 2017 +0300

    Move framework-dependent functionality to PXX files
    
    Comments are provided for most of new code.
    Make auxiliary classes private.

oan

2017-09-20 12:29

developer   ~0070709

Please review

git

2017-09-28 15:23

administrator   ~0070936

Branch CR28931_2 has been updated by oan.

SHA-1: c60c50aaa1d1960a7e573be03257577693e9473d


Detailed log of new commits:

Author: oan
Date: Thu Sep 28 15:22:43 2017 +0300

    Warning on GCC

git

2017-09-28 16:23

administrator   ~0070940

Branch CR28931_2 has been updated forcibly by oan.

SHA-1: 5ac5e39a5fdb8f2d272e6da8ee6864912e4c72bb

git

2017-09-28 16:33

administrator   ~0070941

Branch CR28931_2 has been updated forcibly by oan.

SHA-1: 4dab6b1ec9b8a1ccea947788844e5879b320918b

git

2017-10-03 08:47

administrator   ~0071131

Branch CR28931_3 has been created by abv.

SHA-1: c0ffec1834e6d1240f13abff1eca41e6e0714c9d


Detailed log of new commits:

Author: oan
Date: Fri Sep 15 17:22:30 2017 +0300

    0028931: Eliminate dependency from TBB in OSD_Parallel header
    
    Implementation of methods OSD_Parallel::For() and ForEach() is moved to CXX files to avoid direct dependency of client code that uses OSD_Parallel on TBB headers, and necessity to link with TBB explicitly.
    Runtime polymorphism (virtual methods) is used to hide implementation (TBB or threads-based).

git

2017-10-03 09:38

administrator   ~0071132

Branch CR28931_3 has been updated forcibly by abv.

SHA-1: b928a951129903de0f5e7bf27fbd08f992a8b93a

git

2017-10-03 10:48

administrator   ~0071144

Branch CR28931_3 has been updated forcibly by abv.

SHA-1: ea000bbb941a8e0e3739e98a2b6050ac3ac7830f

git

2017-10-03 14:47

administrator   ~0071155

Branch CR28931_3 has been updated forcibly by abv.

SHA-1: dc8de0727098de6662eb6f0bf6a96cc43e24538e

abv

2017-10-03 14:49

manager   ~0071156

Reviewed with amendments, see branch CR28931_3. Please consider for integration (see testing results of that branch in Jenkins job CR28931_2-master-OAN).

bugmaster

2017-10-03 18:26

administrator   ~0071173

Last edited: 2017-10-05 13:58

 Combination -
OCCT branch : CR28931_3 SHA-1: dc8de0727098de6662eb6f0bf6a96cc43e24538e
Products branch : master
was compiled on Linux, MacOS and Windows platforms and tested on optimize mode.

http://jenkins-test-10.nnov.opencascade.com/view/CR28931_2-master-OAN/

Number of compiler warnings:
No new/fixed warnings

Regressions/Differences/Improvements:
No regressions/differences

CPU differences:
No differences that require special attention

Image differences :
No differences that require special attention

Memory differences :
No differences that require special attention

git

2017-10-14 12:19

administrator   ~0071444

Branch CR28931 has been deleted by kgv.

SHA-1: a6d13483016198354fa621aa945cd323bd481e31

git

2017-10-14 12:19

administrator   ~0071445

Branch CR28931_1 has been deleted by kgv.

SHA-1: 271c93526f74bb5234f2cf863901c4d196d55f73

git

2017-10-14 12:19

administrator   ~0071446

Branch CR28931_2 has been deleted by kgv.

SHA-1: 4dab6b1ec9b8a1ccea947788844e5879b320918b

git

2017-10-14 12:19

administrator   ~0071447

Branch CR28931_3 has been deleted by kgv.

SHA-1: dc8de0727098de6662eb6f0bf6a96cc43e24538e

Related Changesets

occt: master 00af0ebb

2017-09-15 14:22:30

oan


Committer: bugmaster Details Diff		 0028931: Eliminate dependency from TBB in OSD_Parallel header

Implementation of methods OSD_Parallel::For() and ForEach() is moved to CXX files to avoid direct dependency of client code that uses OSD_Parallel on TBB headers, and necessity to link with TBB explicitly.
Runtime polymorphism (virtual methods) is used to hide implementation (TBB or threads-based).		 Affected Issues
0028931
mod - src/BOPCol/BOPCol_Parallel.hxx Diff File
mod - src/OSD/FILES Diff File
mod - src/OSD/OSD_Parallel.cxx Diff File
mod - src/OSD/OSD_Parallel.hxx Diff File
add - src/OSD/OSD_Parallel_TBB.cxx Diff File
add - src/OSD/OSD_Parallel_Threads.cxx Diff File
mod - src/QABugs/QABugs_19.cxx Diff File
mod - src/Select3D/Select3D_SensitivePrimitiveArray.cxx Diff File
mod - src/StdPrs/StdPrs_WFShape.cxx Diff File

Issue History

Date Modified Username Field Change
2017-07-21 09:57 oan New Issue
2017-07-21 09:57 oan Assigned To => abv
2017-07-21 10:04 kgv Note Added: 0068525
2017-07-21 10:19 oan Note Added: 0068527
2017-07-21 10:55 kgv Relationship added related to 0028915
2017-07-26 14:41 abv Note Added: 0068725
2017-07-26 14:41 abv Assigned To abv => oan
2017-07-26 14:41 abv Status new => feedback
2017-07-27 18:01 oan Note Added: 0068807
2017-07-27 18:01 oan Assigned To oan => abv
2017-07-27 18:01 oan Status feedback => resolved
2017-07-27 18:01 oan Steps to Reproduce Updated
2017-07-27 18:01 git Note Added: 0068808
2017-07-27 18:03 oan Note Edited: 0068807
2017-07-31 09:25 abv Note Added: 0068878
2017-07-31 11:29 oan Note Added: 0068882
2017-07-31 11:29 oan Status resolved => feedback
2017-08-11 13:07 oan Relationship added related to 0026365
2017-09-15 12:55 git Note Added: 0070582
2017-09-15 17:27 git Note Added: 0070600
2017-09-15 17:30 oan Note Added: 0070601
2017-09-17 20:50 git Note Added: 0070618
2017-09-19 18:56 git Note Added: 0070700
2017-09-20 12:25 git Note Added: 0070708
2017-09-20 12:29 oan Note Added: 0070709
2017-09-20 12:29 oan Status feedback => resolved
2017-09-28 15:23 git Note Added: 0070936
2017-09-28 16:23 git Note Added: 0070940
2017-09-28 16:33 git Note Added: 0070941
2017-10-03 08:47 git Note Added: 0071131
2017-10-03 09:38 git Note Added: 0071132
2017-10-03 10:48 git Note Added: 0071144
2017-10-03 14:47 git Note Added: 0071155
2017-10-03 14:48 oan Relationship added parent of 0029177
2017-10-03 14:49 abv Note Added: 0071156
2017-10-03 14:49 abv Assigned To abv => bugmaster
2017-10-03 14:49 abv Status resolved => reviewed
2017-10-03 18:26 bugmaster Note Added: 0071173
2017-10-03 18:26 bugmaster Status reviewed => tested
2017-10-04 16:11 bugmaster Test case number => Not required
2017-10-05 13:58 bugmaster Note Edited: 0071173
2017-10-06 14:55 bugmaster Changeset attached => occt master 00af0ebb
2017-10-06 14:55 bugmaster Status tested => verified
2017-10-06 14:55 bugmaster Resolution open => fixed
2017-10-14 12:19 git Note Added: 0071444
2017-10-14 12:19 git Note Added: 0071445
2017-10-14 12:19 git Note Added: 0071446
2017-10-14 12:19 git Note Added: 0071447
2018-02-15 22:30 abv Target Version 7.3.0 => 7.4.0
2018-02-20 12:58 aiv Target Version 7.4.0 => 7.3.0
2018-06-29 21:15 aiv Fixed in Version => 7.3.0
2018-06-29 21:19 aiv Status verified => closed