MantisBT
Mantis Bug Tracker Workflow

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0028931Open CASCADE[OCCT] OCCT:Foundation Classespublic2017-07-21 09:572017-08-11 13:07
Reporteroan 
Assigned Toabv 
PrioritynormalSeverityfeature 
StatusfeedbackResolutionopen 
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 number
Attached Files

- Relationships
related to 0028915verifiedkgv Open CASCADE Configuration, Font_BRepFont - do not include FreeType headers within OCCT headers 
related to 0026365newabv Open CASCADE Optimization of work of OSD_Parallel class members 

-  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.

- 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


Copyright © 2000 - 2017 MantisBT Team
Powered by Mantis Bugtracker