MantisBT
Mantis Bug Tracker Workflow

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0030773Open CASCADE[OCCT] OCCT:Application Frameworkpublic2019-06-06 18:102019-08-21 09:39
Reportermpv 
Assigned Tokgv 
PrioritynormalSeverityfeature 
StatusresolvedResolutionopen 
PlatformOSOS Version
Product Version[OCCT] 7.3.0 
Target Version[OCCT] 7.5.0Fixed in Version 
Summary0030773: Application Framework - To allow to inherit existing attributes to reuse persistence mechanisms
DescriptionCurrently most of the medium and big size-applications that use OCAF data structures covers them by higher-level interfaces. They allow involving application-specific names, relations between data, some simple logic over the basis OCAF components.

For an example: there may be “Color” class that covers the array of integer attribute (RGB). It manages OCAF data by reference to attribute of to the label. It may:
• Provide application-specific access to the data: methods ”blue”, “setBlue”, “isWhite”, etc.
• Support references between objects. Like, this label may contain not array attribute, but reference to other array or object or pointer to external, not OCAF object. Method “blue” and others may correctly manage this.
• To cash some data based on persistent values to avoid too often computation. Like “intensity” by RGB.

Disadvantage of such approach is that the interface classes hardly can be persistent. Normally interface is created on the fly by demand and cannot live longer than a transaction. This approach does not allow the cashing, keeping some objects in fields and controlling the data modification.

If objects are more persistent, there appeared a problem with update of these objects on undo/redo, abort and load/save operations. Modification of OCAF data may be done outside of related objects, so it breaks the object-oriented approach and an idea of encapsulation.

Currently here appeared a problem that it is required to create interface-objects by some data of TDF_Attribute. Low level OCAF entities (labels and/or attributes) in many cases become arguments of methods for creation, searching or iteration of high-level interfaces. It is necessary anyway to create some kind of factory that creates interface-objects (at least on load of the document) by the presented OCAF structure. Another approach may be to inherit the OCAF attributes and somehow control the data change (AfterUndo and other similar methods), but this requires complicated development to implement persistent mechanism for them (in general, at least for 2 standard formats supported) and other things. With the current way of implementation it is simple code-duplication.

In this improvement it is proposed to allow developer to inherit existing attributes if the same persistent fileds are used. All methods that allow controlling the data model changes or getting some callbacks may be overridden in successor.
Development of persistence mechanism for such attributes is minimal. In the example above for “Color” class it will be enough to inherit “TDataStd_IntegerArray”, so, functionality of “TDataStd_IntegerArray” will be used to store/restore all the needed data. Minimal requirement is to redefine NewEmpty method (ID and GetID to have different GUID, other methods for more complicated behavior).

To re-create such attributes on load of the file, there must be a special factory. It is proposed to add it to the TDF_Attribute.hxx file, so, no additional includes will be required to make a derived attribute. A special macro will register a new derived attributes in the factory (it may be callled instead of standard Handle macro definition: IMPLEMENT_STANDARD_RTTIEXT).

The main benefit for OCCT is that derived attributes contain only interfaces and new GUIDs (and dynamic type) and require no classes in persistent drivers (like TDataStd_Comment may inherit TDataStd_Name and the following drivers become obsolete: BinMDataStd_CommentDriver, XmlMDataStd_CommentDriver, part of StdLPersistent_Value).

This improvement should not change both present formats Bin and XML documents. THe obsolete Standard scheme is not changed at all.
Steps To ReproduceNot required
TagsNo tags attached.
Test case numberNot required
Attached Files

- Relationships

-  Notes
(0084935)
git (administrator)
2019-06-06 18:24

Branch CR30773 has been created by mpv.

SHA-1: 3468f571674dbc9e876bad577980f70d0465c126


Detailed log of new commits:

Author: mpv
Date: Thu Jun 6 18:23:34 2019 +0300

    30773: To allow to inherit existing attributes to reuse persistence mechanisms
    
    Added possibility to inherit attributes, supported by Deriver Attributes factory in TDF_Attribute, macros and DerivedDriver instance in the Bin and XML data storage.
    Removed drivers in Bin and XML schemes for derived attributes.
(0084947)
git (administrator)
2019-06-07 16:28

Branch CR30773 has been updated by mpv.

SHA-1: 2540bde130ea87e5a83646fd8c9752066e58c88a


Detailed log of new commits:

Author: mpv
Date: Fri Jun 7 16:28:07 2019 +0300

    30773: To allow to inherit existing attributes to reuse persistence mechanisms
    
    # Correction of compilation warnings

(0084981)
git (administrator)
2019-06-11 09:49

Branch CR30773_2 has been created by mpv.

SHA-1: 7f21b051d965bea56dfeea4da88cbb08766454d7


Detailed log of new commits:

Author: mpv
Date: Tue Jun 11 09:46:16 2019 +0300

    30773: Application Framework - To allow to inherit existing attributes to reuse persistence mechanisms
(0085025)
mpv (developer)
2019-06-13 10:20

Solution is in CR30773_2 branch: many drivers in Bin and Xml persistence schemes are removed due to duplication of functionality of drivers, or fully empty drivers methods.

No regressions are detected:
http://occt-tests/CR30773_2-CR30773_2-MPV-Products/Debian80-64/diff_summary.html [^]
http://occt-tests/CR30773_2-CR30773_2-MPV-OCCT/Windows-64-VC14/diff_summary.html [^]

Please, review.
(0085050)
szy (developer)
2019-06-14 12:46

Remarks.
1.IntegerDriver (Bin & Xml) modification
if (!ok) {
      theSource.SetPosition(aPos);
      anAtt->SetID(TDataStd_Integer::GetID());
      ok = Standard_True;
    } else ...
It means that expected predefined attribute (before TDataStd_Integer).
Removing line anAtt->SetID(...) is dangerous as it leads to not initialized field.
It should be processed taking into account as parent as derived types.

2.It is not desirable to use static maps (DERIVED & DERIVED_TYPES) as it blocks possibility to use it in multi-threading application.
It would be better to use another solution with dynamic creation. Let's discuss it.
(0085051)
szy (developer)
2019-06-14 12:47

See my comments.
(0085055)
git (administrator)
2019-06-14 15:39

Branch CR30773_2 has been updated by mpv.

SHA-1: 1056ba2343c05ec33814341474943fe6f188032b


Detailed log of new commits:

Author: mpv
Date: Fri Jun 14 15:39:03 2019 +0300

    30773: Application Framework - To allow to inherit existing attributes to reuse persistence mechanisms
    
    Protection for the multi-threaded open/save of documents with different resources.

(0085093)
mpv (developer)
2019-06-17 17:14

It is discussed that the first note is not needed to implement: GUID is defined in the constructor of the attributes.

The second note is fixed in branch CR30773_2 : access to the map is covered by a mutex.

http://vm-jenkins-test-12.nnov.opencascade.com:8080/job/CR30773_2-CR30773_2-MPV-OCCT-Windows-64-VC14-opt-test-compare/4/ [^]

http://vm-jenkins-test-12.nnov.opencascade.com:8080/job/CR30773_2-CR30773_2-MPV-OCCT-Debian80-64-opt-test-compare/7/ [^]

Please, review.
(0085138)
szy (developer)
2019-06-19 10:37

Reviewed.
(0085151)
bugmaster (administrator)
2019-06-19 20:12

Combination -
OCCT branch : CR30773_2
master SHA - 1056ba2343c05ec33814341474943fe6f188032b
d67d4b811012eef8913d3c535c29654d0acf3c4c
Products branch : CR30773_2 SHA - 13155ea25080dfb52eff0da21e2a40bed0df94a2
was compiled on Linux, MacOS and Windows platforms and tested in optimize mode.

Number of compiler warnings:
No new/fixed warnings

Regressions/Differences/Improvements:
No regressions/differences

CPU differences:
Debian80-64:
OCCT
Total CPU difference: 16189.080000000034 / 16165.440000000055 [+0.15%]
Products
Total CPU difference: 10479.060000000041 / 10470.220000000043 [+0.08%]
Windows-64-VC14:
OCCT
Total CPU difference: 17615.328125 / 17617.515625 [-0.01%]
Products
Total CPU difference: 12065.65625 / 12110.78125 [-0.37%]


Image differences :
No differences that require special attention

Memory differences :
No differences that require special attention
(0085179)
git (administrator)
2019-06-20 15:02

Branch CR30773_2 has been updated by mpv.

SHA-1: 25cdd1ed800fbef952d5f5d7f2d2e75ffea2c3b8


Detailed log of new commits:

Author: mpv
Date: Thu Jun 20 15:01:15 2019 +0300

    30773: Application Framework - To allow to inherit existing attributes to reuse persistence mechanisms
    
    # fix of the Ubuntu-1604-64-opt warnings

(0085182)
kgv (developer)
2019-06-20 15:51
edited on: 2019-06-20 15:53

30773: Application Framework - To allow to inherit existing attributes to reuse persistence mechanisms


The patch description which is supposed to be within commit description is missing.

+  Standard_EXPORT void AddDerivedDriver (Handle(TDF_Attribute) theInstance);
...
+  BinMDF_DerivedDriver(const Handle(TDF_Attribute) theDerivative, const Handle(BinMDF_ADriver) theBaseDriver);

...
+  Standard_EXPORT bool RegisterAttribute(Handle(TDF_Attribute) theDerived,

const Handle(TDF_Attribute)&
const Handle(BinMDF_ADriver)&

--- /dev/null
+++ b/src/BinMDF/BinMDF_DerivedDriver.lxx
--- /dev/null
+++ b/src/XmlMDF/XmlMDF_DerivedDriver.lxx

Please avoid creation of new .lxx files for inlined functions having small size.
Also prefer inlining at declaration for compact functions.

--- a/src/BinMDataStd/BinMDataStd_IntegerDriver.cxx
+++ b/src/BinMDataStd/BinMDataStd_IntegerDriver.cxx
@@ -65,7 +65,6 @@ Standard_Boolean BinMDataStd_IntegerDriver::Paste
        ok = theSource >> aGuid;        
        if (!ok) {
          theSource.SetPosition(aPos);    
-         anAtt->SetID(TDataStd_Integer::GetID());

Is it related to the patch?

+    if (DERIVED.IsBound (theType))
+      return DERIVED (theType);

Seek()/Find().

+//! Defines implementation of Handle method and registers the derived attribute
+#define IMPLEMENT_DERIVED_ATTRIBUTE(Class, Base) \
+  IMPLEMENT_STANDARD_RTTIEXT(Class, Base) \
+  static bool TDF_DERIVED_CONTAINER(TDF_DerivedAttribute::RegisterAttribute(new Class()));

This mechanism will lead to problems on using OCCT built as static libraries.

+  //! Returns the current message driver of this driver
+  const Handle(Message_Messenger)& MessageDriver() const;

I suppose Standard_EXPORT is missing.

+inline Handle(Standard_Type) XmlMDF_ADriverTable::AddDerivedDriver (Standard_CString theDerivedType)


What inline is supposed to mean here?

+class XmlMDF_DerivedDriver : public XmlMDF_ADriver {

newline before {

(0085514)
git (administrator)
2019-07-09 16:16

Branch CR30773_3 has been created by mpv.

SHA-1: c638611f5d6065172d911de2857ee98a48d641a1


Detailed log of new commits:

Author: mpv
Date: Tue Jul 9 16:15:08 2019 +0300

    0030773: Application Framework - To allow to inherit existing attributes to reuse persistence mechanisms
    
    Added possibility to inherit existing attributes if the same persistent fields are used. All methods that allow controlling the data model changes or getting some callbacks may be overridden in successor. They may have same GUIDs as a base class or new ones.
    
    Special macros IMPLEMENT_DERIVED_ATTRIBUTE and IMPLEMENT_DERIVED_ATTRIBUTE_WITH_TYPE must be used instead of standard Handle macro definition IMPLEMENT_STANDARD_RTTIEXT to register new derived attributes.
    
    Using this improvement several existing attributes from TDataStd, TDataXtd and XCAFDoc packages become inherited from other base attribute-classes. XML and Bin drivers of these attributes are removed.
    
    This improvement does not change both present formats Bin and XML documents. The obsolete Standard scheme is not changed at all.
(0085529)
mpv (developer)
2019-07-09 18:25

  Dear KGV,

All remarks are fixed except the following:

    + Standard_EXPORT bool RegisterAttribute(Handle(TDF_Attribute) theDerived,
It should not receive references to "Handle" because it is called with attributes, created "on the fly".

    - anAtt->SetID(TDataStd_Integer::GetID());
    Is it related to the patch?
Yes. For now correct GUID is assigned in constructor of TDataStd_Integer and this line prevents from creation of derived attributes with different GUIDs (it is already discussed with SZY in comments 85050 and 85093)

    + static bool TDF_DERIVED_CONTAINER(TDF_DerivedAttribute::RegisterAttribute(new Class()));
    This mechanism will lead to problems on using OCCT built as static libraries.
In TDF_Attribute.cxx all global variables are covered by static methods now. So, there should not be problem with order of global variables initialization in static mode of link.


Solution is in CR30773_3 branch

No regressions are detected:
http://vm-jenkins-test-12.nnov.opencascade.com:8080/job/CR30773_3-CR30773_3-MPV-OCCT-Debian80-64-opt-test-compare/1/ [^]
http://vm-jenkins-test-12.nnov.opencascade.com:8080/job/CR30773_3-CR30773_3-MPV-OCCT-Windows-64-VC14-opt-test-compare/1/ [^]

Please, review.
(0085758)
git (administrator)
2019-07-18 13:07

Branch CR30773_3 has been updated by kgv.

SHA-1: 72a52f3bab11af12b10fd39661b81a7897377df2


Detailed log of new commits:

Author: kgv
Date: Thu Jul 18 13:05:23 2019 +0300

    # remarks

(0085763)
git (administrator)
2019-07-18 14:01

Branch CR30773_3 has been updated forcibly by kgv.

SHA-1: 25e0507a1c5d93e24cf6cddc0380862b12422478
(0085779)
kgv (developer)
2019-07-19 09:44
edited on: 2019-07-19 09:46

I have pushed some corrections to CR30773_3.
Apart from this, the problem of usage of static library still not handled by the patch.

The problem has been already indicated by GCC warnings which you have just suppressed:
+#if (defined(__GNUC__))
+  // gcc emits -Wunused-variable but TDF_DERIVED_CONTAINER variable is declared for RegisterAttribute 
call only
+  #pragma GCC diagnostic push
+  #pragma GCC diagnostic ignored "-Wunused-variable"
+#endif
+
+//! Defines implementation of Handle method and registers the derived attribute
+#define IMPLEMENT_DERIVED_ATTRIBUTE(Class, Base) \
+  IMPLEMENT_STANDARD_RTTIEXT(Class, Base) \
+  static bool TDF_DERIVED_CONTAINER(TDF_DerivedAttribute::RegisterAttribute(new Class()));

Defining a global variable which is never used explicitly in code allows linker to completely remove their construction from static library when using default linkage options.

In addition, unconditional initialization of these maps while loading DLL is also undesired, because its slows down application startup even in case, when OCAF is not used / not always used by application. Moreover, though, such kind of self-initialization looks very compact, it complicates understanding of where these structures are actually used.

Therefore, the preferred solution is implementing a dedicated initializer(s) which are explicitly called before they will be used, where each derived attribute is explicitly registered.

Example of this initialization style are global variables defined via Interface_Static interface - see STEPCAFControl_Controller::Init(), STEPControl_Controller::Init(), IGESControl_Controller::Init() calls.

(0085965)
git (administrator)
2019-08-01 13:37

Branch CR30773_3 has been updated by mpv.

SHA-1: 94cf40cfb67de7c3e1988c0f9e16f0785da06713


Detailed log of new commits:

Author: mpv
Date: Thu Aug 1 13:35:02 2019 +0300

    # reaction to remarks

(0085967)
mpv (developer)
2019-08-01 13:46

Ok, I have tried to satisfy the last request:

- Global variable is now used by NewEmpty method, so, no warnings, no remove possibility by constructor

- Slowing down the application on launch is minimized: no attribute-constructors are called, no map is filled. Just list, filled by simple pointers, few bytes per attribute.

- The needed maps are filled by "Initialize" method, like in Interface_Static. It is called by the first demand: on document open or save.

- Filling (resize) of maps is normally performed only once per application run-time. So, probability to keep invalid Handle referenced to destroyed map is minimized, near to 0 (this is to comment "not thread-safe when resizing the map" in TDF_DerivedAttribute.cxx)
(0086379)
mpv (developer)
2019-08-21 09:39

No regressions now:
http://vm-jenkins-test-12.nnov.opencascade.com:8080/job/CR30773_3-CR30773_3-MPV-OCCT-Windows-64-VC14-opt-test-compare/3/ [^]
http://vm-jenkins-test-12.nnov.opencascade.com:8080/job/CR30773_3-CR30773_3-MPV-OCCT-Debian80-64-opt-test-compare/3/ [^]
http://vm-jenkins-test-12.nnov.opencascade.com:8080/job/CR30773_3-CR30773_3-MPV-Products-Windows-64-VC14-opt-test-compare/3/ [^]
http://vm-jenkins-test-12.nnov.opencascade.com:8080/job/CR30773_3-CR30773_3-MPV-Products-Debian80-64-opt-test-compare/3/ [^]

- Issue History
Date Modified Username Field Change
2019-06-06 18:10 mpv New Issue
2019-06-06 18:10 mpv Assigned To => mpv
2019-06-06 18:24 git Note Added: 0084935
2019-06-07 13:44 kgv Summary To allow to inherit existing attributes to reuse persistence mechanisms => Application Framework - To allow to inherit existing attributes to reuse persistence mechanisms
2019-06-07 16:28 git Note Added: 0084947
2019-06-11 09:49 git Note Added: 0084981
2019-06-13 10:20 mpv Note Added: 0085025
2019-06-13 10:20 mpv Assigned To mpv => szy
2019-06-13 10:20 mpv Status new => resolved
2019-06-13 10:20 mpv Steps to Reproduce Updated View Revisions
2019-06-14 12:46 szy Note Added: 0085050
2019-06-14 12:47 szy Note Added: 0085051
2019-06-14 12:47 szy Assigned To szy => mpv
2019-06-14 12:47 szy Status resolved => feedback
2019-06-14 15:39 git Note Added: 0085055
2019-06-17 17:14 mpv Note Added: 0085093
2019-06-17 17:14 mpv Assigned To mpv => szy
2019-06-17 17:14 mpv Status feedback => resolved
2019-06-19 10:37 szy Note Added: 0085138
2019-06-19 10:37 szy Assigned To szy => bugmaster
2019-06-19 10:37 szy Status resolved => reviewed
2019-06-19 20:11 bugmaster Test case number => Not required
2019-06-19 20:12 bugmaster Note Added: 0085151
2019-06-19 20:12 bugmaster Status reviewed => tested
2019-06-20 15:02 git Note Added: 0085179
2019-06-20 15:51 kgv Note Added: 0085182
2019-06-20 15:51 kgv Assigned To bugmaster => mpv
2019-06-20 15:51 kgv Status tested => assigned
2019-06-20 15:51 kgv Target Version => 7.5.0
2019-06-20 15:53 kgv Note Edited: 0085182 View Revisions
2019-07-09 16:16 git Note Added: 0085514
2019-07-09 18:25 mpv Note Added: 0085529
2019-07-09 18:25 mpv Assigned To mpv => kgv
2019-07-09 18:25 mpv Status assigned => resolved
2019-07-18 13:07 git Note Added: 0085758
2019-07-18 14:01 git Note Added: 0085763
2019-07-19 09:44 kgv Note Added: 0085779
2019-07-19 09:45 kgv Assigned To kgv => mpv
2019-07-19 09:45 kgv Status resolved => assigned
2019-07-19 09:45 kgv Note Edited: 0085779 View Revisions
2019-07-19 09:46 kgv Note Edited: 0085779 View Revisions
2019-07-19 09:46 kgv Note Edited: 0085779 View Revisions
2019-08-01 13:37 git Note Added: 0085965
2019-08-01 13:46 mpv Note Added: 0085967
2019-08-01 13:46 mpv Assigned To mpv => kgv
2019-08-01 13:46 mpv Status assigned => resolved
2019-08-21 09:39 mpv Note Added: 0086379


Copyright © 2000 - 2019 MantisBT Team
Powered by Mantis Bugtracker