MantisBT
Mantis Bug Tracker Workflow

View Issue Details Jump to Notes ] Related Changesets ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0028931Open CASCADE[OCCT] OCCT:Foundation Classespublic2017-07-21 09:572017-10-14 12:19
Reporteroan 
Assigned Tobugmaster 
PrioritynormalSeverityfeature 
StatusverifiedResolutionfixed 
PlatformOSOS Version
Product Version 
Target Version[OCCT] 7.2.1Fixed in Version 
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
Attached Files

- Relationships
related to 0028915closedkgv Open CASCADE Configuration, Font_BRepFont - do not include FreeType headers within OCCT headers 
parent of 0029177newabv Open CASCADE Adapt BVH package for use of OSD_Parallel 
related to 0026365newabv Open CASCADE Optimization of work of OSD_Parallel class members 
Not all the children of this issue are yet resolved or closed.

-  Notes
(0068525)
kgv (developer)
2017-07-21 10:04

> including TBB headers.
Also including TBB libraries.
(0068527)
oan (developer)
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 (manager)
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 (developer)
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 (administrator)
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 (manager)
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 (developer)
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 (administrator)
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 (administrator)
2017-09-15 17:27

Branch CR28931_1 has been updated forcibly by oan.

SHA-1: 271c93526f74bb5234f2cf863901c4d196d55f73
(0070601)
oan (developer)
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 (administrator)
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 (administrator)
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 (administrator)
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 (developer)
2017-09-20 12:29

Please review
(0070936)
git (administrator)
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 (administrator)
2017-09-28 16:23

Branch CR28931_2 has been updated forcibly by oan.

SHA-1: 5ac5e39a5fdb8f2d272e6da8ee6864912e4c72bb
(0070941)
git (administrator)
2017-09-28 16:33

Branch CR28931_2 has been updated forcibly by oan.

SHA-1: 4dab6b1ec9b8a1ccea947788844e5879b320918b
(0071131)
git (administrator)
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 (administrator)
2017-10-03 09:38

Branch CR28931_3 has been updated forcibly by abv.

SHA-1: b928a951129903de0f5e7bf27fbd08f992a8b93a
(0071144)
git (administrator)
2017-10-03 10:48

Branch CR28931_3 has been updated forcibly by abv.

SHA-1: ea000bbb941a8e0e3739e98a2b6050ac3ac7830f
(0071155)
git (administrator)
2017-10-03 14:47

Branch CR28931_3 has been updated forcibly by abv.

SHA-1: dc8de0727098de6662eb6f0bf6a96cc43e24538e
(0071156)
abv (manager)
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 (administrator)
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 (administrator)
2017-10-14 12:19

Branch CR28931 has been deleted by kgv.

SHA-1: a6d13483016198354fa621aa945cd323bd481e31
(0071445)
git (administrator)
2017-10-14 12:19

Branch CR28931_1 has been deleted by kgv.

SHA-1: 271c93526f74bb5234f2cf863901c4d196d55f73
(0071446)
git (administrator)
2017-10-14 12:19

Branch CR28931_2 has been deleted by kgv.

SHA-1: 4dab6b1ec9b8a1ccea947788844e5879b320918b
(0071447)
git (administrator)
2017-10-14 12:19

Branch CR28931_3 has been deleted by kgv.

SHA-1: dc8de0727098de6662eb6f0bf6a96cc43e24538e

- Related Changesets
occt: master 00af0ebb
Timestamp: 2017-09-15 14:22:30
Author: 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).
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 09:58 oan Relationship added related to 0025115
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 View Revisions
2017-07-27 18:01 git Note Added: 0068808
2017-07-27 18:03 oan Note Edited: 0068807 View Revisions
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 View Revisions
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


Copyright © 2000 - 2017 MantisBT Team
Powered by Mantis Bugtracker