MantisBT - Open CASCADE
View Issue Details
0028931Open CASCADE[OCCT] OCCT:Foundation Classespublic2017-07-21 09:572018-06-29 21:19
oan 
bugmaster 
normalfeature 
closedfixed 
 
[OCCT] 7.3.0[OCCT] 7.3.0 
Not required
0028931: Eliminate dependency from TBB in OSD_Parallel header
It 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.
None
No tags attached.
related to 0028915closed kgv Open CASCADE Configuration, Font_BRepFont - do not include FreeType headers within OCCT headers 
parent of 0029177verified apn Open CASCADE Foundation Classes - Adapt BVH package for use of OSD_Parallel 
related to 0026365assigned kgv Open CASCADE Optimization of work of OSD_Parallel class members for GeomLib_CheckCurveOnSurface 
Issue History
2017-07-21 09:57oanNew Issue
2017-07-21 09:57oanAssigned To => abv
2017-07-21 09:58oanRelationship addedrelated to 0025115
2017-07-21 10:04kgvNote Added: 0068525
2017-07-21 10:19oanNote Added: 0068527
2017-07-21 10:55kgvRelationship addedrelated to 0028915
2017-07-26 14:41abvNote Added: 0068725
2017-07-26 14:41abvAssigned Toabv => oan
2017-07-26 14:41abvStatusnew => feedback
2017-07-27 18:01oanNote Added: 0068807
2017-07-27 18:01oanAssigned Tooan => abv
2017-07-27 18:01oanStatusfeedback => resolved
2017-07-27 18:01oanSteps to Reproduce Updatedbug_revision_view_page.php?rev_id=17208#r17208
2017-07-27 18:01gitNote Added: 0068808
2017-07-27 18:03oanNote Edited: 0068807bug_revision_view_page.php?bugnote_id=68807#r17210
2017-07-31 09:25abvNote Added: 0068878
2017-07-31 11:29oanNote Added: 0068882
2017-07-31 11:29oanStatusresolved => feedback
2017-08-11 13:07oanRelationship addedrelated to 0026365
2017-09-15 12:55gitNote Added: 0070582
2017-09-15 17:27gitNote Added: 0070600
2017-09-15 17:30oanNote Added: 0070601
2017-09-17 20:50gitNote Added: 0070618
2017-09-19 18:56gitNote Added: 0070700
2017-09-20 12:25gitNote Added: 0070708
2017-09-20 12:29oanNote Added: 0070709
2017-09-20 12:29oanStatusfeedback => resolved
2017-09-28 15:23gitNote Added: 0070936
2017-09-28 16:23gitNote Added: 0070940
2017-09-28 16:33gitNote Added: 0070941
2017-10-03 08:47gitNote Added: 0071131
2017-10-03 09:38gitNote Added: 0071132
2017-10-03 10:48gitNote Added: 0071144
2017-10-03 14:47gitNote Added: 0071155
2017-10-03 14:48oanRelationship addedparent of 0029177
2017-10-03 14:49abvNote Added: 0071156
2017-10-03 14:49abvAssigned Toabv => bugmaster
2017-10-03 14:49abvStatusresolved => reviewed
2017-10-03 18:26bugmasterNote Added: 0071173
2017-10-03 18:26bugmasterStatusreviewed => tested
2017-10-04 16:11bugmasterTest case number => Not required
2017-10-05 13:58bugmasterNote Edited: 0071173bug_revision_view_page.php?bugnote_id=71173#r17833
2017-10-06 14:55bugmasterChangeset attached => occt master 00af0ebb
2017-10-06 14:55bugmasterStatustested => verified
2017-10-06 14:55bugmasterResolutionopen => fixed
2017-10-14 12:19gitNote Added: 0071444
2017-10-14 12:19gitNote Added: 0071445
2017-10-14 12:19gitNote Added: 0071446
2017-10-14 12:19gitNote Added: 0071447
2018-02-15 22:30abvTarget Version7.3.0 => 7.4.0
2018-02-20 12:58aivTarget Version7.4.0 => 7.3.0
2018-06-29 21:15aivFixed in Version => 7.3.0
2018-06-29 21:19aivStatusverified => closed

Notes
(0068525)
kgv   
2017-07-21 10:04   
> including TBB headers.
Also including TBB libraries.
(0068527)
oan   
2017-07-21 10:19   
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.
(0068725)
abv   
2017-07-26 14:41   
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.
(0068807)
oan   
2017-07-27 18:01   
(edited on: 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...

(0068808)
git   
2017-07-27 18:01   
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.
(0068878)
abv   
2017-07-31 09:25   
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.
(0068882)
oan   
2017-07-31 11:29   
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.
(0070582)
git   
2017-09-15 12:55   
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.
(0070600)
git   
2017-09-15 17:27   
Branch CR28931_1 has been updated forcibly by oan.

SHA-1: 271c93526f74bb5234f2cf863901c4d196d55f73
(0070601)
oan   
2017-09-15 17:30   
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.
(0070618)
git   
2017-09-17 20:50   
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).
(0070700)
git   
2017-09-19 18:56   
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

(0070708)
git   
2017-09-20 12:25   
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.

(0070709)
oan   
2017-09-20 12:29   
Please review
(0070936)
git   
2017-09-28 15:23   
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

(0070940)
git   
2017-09-28 16:23   
Branch CR28931_2 has been updated forcibly by oan.

SHA-1: 5ac5e39a5fdb8f2d272e6da8ee6864912e4c72bb
(0070941)
git   
2017-09-28 16:33   
Branch CR28931_2 has been updated forcibly by oan.

SHA-1: 4dab6b1ec9b8a1ccea947788844e5879b320918b
(0071131)
git   
2017-10-03 08:47   
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).
(0071132)
git   
2017-10-03 09:38   
Branch CR28931_3 has been updated forcibly by abv.

SHA-1: b928a951129903de0f5e7bf27fbd08f992a8b93a
(0071144)
git   
2017-10-03 10:48   
Branch CR28931_3 has been updated forcibly by abv.

SHA-1: ea000bbb941a8e0e3739e98a2b6050ac3ac7830f
(0071155)
git   
2017-10-03 14:47   
Branch CR28931_3 has been updated forcibly by abv.

SHA-1: dc8de0727098de6662eb6f0bf6a96cc43e24538e
(0071156)
abv   
2017-10-03 14:49   
Reviewed with amendments, see branch CR28931_3. Please consider for integration (see testing results of that branch in Jenkins job CR28931_2-master-OAN).
(0071173)
bugmaster   
2017-10-03 18:26   
(edited on: 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

(0071444)
git   
2017-10-14 12:19   
Branch CR28931 has been deleted by kgv.

SHA-1: a6d13483016198354fa621aa945cd323bd481e31
(0071445)
git   
2017-10-14 12:19   
Branch CR28931_1 has been deleted by kgv.

SHA-1: 271c93526f74bb5234f2cf863901c4d196d55f73
(0071446)
git   
2017-10-14 12:19   
Branch CR28931_2 has been deleted by kgv.

SHA-1: 4dab6b1ec9b8a1ccea947788844e5879b320918b
(0071447)
git   
2017-10-14 12:19   
Branch CR28931_3 has been deleted by kgv.

SHA-1: dc8de0727098de6662eb6f0bf6a96cc43e24538e