|Anonymous | Login||2017-08-20 07:11 MSK|
|My View | View Issues | Change Log | Roadmap|
|View Issue Details|
|ID||Project||Category||View Status||Date Submitted||Last Update|
|0028931||Open CASCADE||[OCCT] OCCT:Foundation Classes||public||2017-07-21 09:57||2017-08-11 13:07|
|Target Version||[OCCT] 7.2.1||Fixed in Version|
|Summary||0028931: Eliminate dependency from TBB in OSD_Parallel header|
|Description||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.|
|Steps To Reproduce||None|
|Tags||No tags attached.|
|Test case number|
> including TBB headers.
Also including TBB libraries.
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.
|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.|
edited on: 2017-07-27 18:03
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...
Branch CR28931 has been created by oan.
Detailed log of new commits:
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.
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.
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.
|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|