MantisBT
Mantis Bug Tracker Workflow

View Issue Details Jump to Notes ] Related Changesets ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0029014Community[OCCT] OCCT:Application Frameworkpublic2017-08-18 10:342017-10-09 12:14
ReporterBenjaminBihler 
Assigned Tobugmaster 
PrioritynormalSeverityminor 
StatusverifiedResolutionfixed 
PlatformOSOS Version
Product Version[OCCT] 7.1.0 
Target Version[OCCT] 7.2.1Fixed in Version 
Summary0029014: Managing Binary Format Versions Is Not Possible for Own TDF_Attributes
DescriptionI have tried to discuss the issue here: https://www.opencascade.com/content/document-versions-binocaf. [^] There is a document version hardcoded in OCCT which can be used for checking whether the versions of standard attributes are compatible.

But if an application using its own TDF_Attributes evolves, the versions of these attributes should also be managed. Perhaps they are incompatible or binary drivers of newer application versions store additional data. All application versions may use the same OCCT, therefore the document version stored by OCCT does not suffice for format version management.

Within Storage_HeaderData the name and version of the creating application are already stored, but these fields are always empty. Enhancing CDM_Application to return an application name and version, storing that within the Storage_HeaderData and propagating application name and version during retrieval like the document version is already propagated, would solve the problem. The binary drivers could then for example read additional data depending on the application and/or application version.
Steps To ReproduceNot required
TagsNo tags attached.
Test case numberNot required
Attached Files

- Relationships
related to 0029195newszy Open CASCADE OCAF - ensure thread safety for different documents 

-  Notes
(0069565)
git (administrator)
2017-08-18 12:00

Branch CR29014 has been created by BenjaminBihler.

SHA-1: 7418856cdcdd2ab2a11a02aa7428332c41b6fc13


Detailed log of new commits:

Author: Benjamin Bihler
Date: Fri Aug 18 09:40:19 2017 +0200

    0029014: Managing Binary Format Version Is Not Possible for Own TDF_Attributes
    
    CDM_Application has been extended to provide application name and version.
    
    Application name and version is stored by BinLDrivers_DocumentStorageDriver.
    
    BinLDrivers_DocumentStorageDriver propagates application name and version
    by passing it to BinMDataStd.
(0069597)
mpv (developer)
2017-08-18 16:55

Your solution deos not resolve the problem of version of specific TDF_Attribute (as it is mentioned in the header). Keeping storage-version in the file for each attribute is memory consuming. If it is necessary, why not to store it as a persistent field of your attribute?

In the commit proposed the application version is retrieved in BinMDataStd, which is related to one of the package of standard attributes in OCAF, which is not really related to the whole application.

Storage_HeaderData class is an inheritance from standard (ASCII) storage/retrevial schema, where it was supposed that this schema may be redefined. As you see, these methods are not really usable and accessible for this task.

You may take a look at issue 28691, where the supported document version may be changed externally, from the application. This info is accessible from any attribute or persistance driver, if needed. Unfortunately this is implemented only in Bin format, but approach may be the same, it is quite simple.
(0069675)
BenjaminBihler (developer)
2017-08-18 17:28

But isn't that exactly the advantage of the proposed solution? The storage version is NOT kept for each attribute, but it is only stored once in the file header data with which application version the document was created. Storing it in every attribute would be memory consuming.

My intention is like that: common applications can open documents created with older versions, but usually not with newer versions of the same application. I understand that my approach does not offer storing old document formats from new versions of the application.

On the other hand I do not understand why you consider Storage_HeaderData as not being usable for storing the creating application name and version - it is already used that way! Only the fields are always empty! And filling them with correct data enables simple downwards compatibility (binary drivers can take the application version into account when reading in data) and does not prevent compatibility into both directions. But for the latter one would have to store detailed version information into every attribute.

I have misused BinMDataStd, since then the application name and version are close to the document version which is somehow similar.

What would you say to the following proposal: I have to propagate the application name and version so the binary attribute drivers to have a benefit from it. They inherit from BinMDF_ADriver and from them there is no bridge to BinLDrivers_DocumentRetrievalDriver which knows the file header data. If I would introduce two static fields (application name and version) and public static getters into BinLDrivers_DocumentRetrievalDriver that are overwritten each time a file is read, then the binary drivers could get the application version from there. I would remove the changes from BinMDataStd then.

Do you still see any disadvantage?

Issue 28691 seems interesting, but it is actually much more than I have intended.

Thank you!
(0069677)
abv (manager)
2017-08-18 18:33

I agree that it is necessary to have possibility to access header data from within attribute drivers. However, proposed approach uses static variables (in BinMDataStd.cxx) -- this is not thread safe.

Perhaps the more consistent but still easy approach (for reading) would be to store handle to header section (variable aHeaderData in BinLDrivers_DocumentRetrievalDriver::Read()) together with relocation table (myRelocTable) so that it is available to all drivers. For that, type BinObjMgt_RRelocationTable can be defined as a class inheriting map (in its current definition) with adding the field for header.
(0069688)
BenjaminBihler (developer)
2017-08-21 08:31

This proposal sounds very nice to me. mpv, do you have any objections? Should I try to implement that in the issue branch?

(The remark about the thread safety actually applies also to the document version during retrieval, doesn't it?)
(0069690)
abv (manager)
2017-08-21 09:18

Benjamin, you are perfectly right, management of document version is not thread safe in the current code... It would be nice to correct that, too, to try to be thread-safe in reading. Let's see opinion of Mikhail
(0069699)
mpv (developer)
2017-08-21 11:00

I agree, this could be a good solultion. So, you may try, Benjamin.

Don't forget to nullify this handler in "Clear" method of the new RelocationTable.

Also it is worth to look at "PropagateDocumentVersion" methods necessity due to these modifications: perhaps they may be deleted in all drivers-packages (as vell as SetDocumentVersion/DocumentVersion in all drivers).
(0069705)
git (administrator)
2017-08-21 13:09

Branch CR29014 has been updated by BenjaminBihler.

SHA-1: 2f7bba515211066f25ca2a674086c5f920e09166


Detailed log of new commits:

Author: Benjamin Bihler
Date: Mon Aug 21 12:06:02 2017 +0200

    0029014: Managing Binary Format Versions Is Not Possible for Own TDF_Attributes
    
    Undone storing application name and version as static fields in BinMDataStd
    which is bad style and not thread-safe.

Author: Benjamin Bihler
Date: Mon Aug 21 11:58:00 2017 +0200

    0029014: Managing Binary Format Versions Is Not Possible for Own TDF_Attributes
    
    Made BinObjMgt_RRelocationTable store a handle to the header data of the file
    begin read in to make it accessible by binary attribute drivers.

(0069707)
BenjaminBihler (developer)
2017-08-21 13:13

Mikhail, I have done the changes and tested them here. They work as expected.

I did not get why the handle should be nullified in the Clear method of the relocation table. A relocation table belongs to a document and clearing the table cannot make the relocation table belong to another document. Therefore why should the handle ever be nullified? So I haven't done that.

I also haven't touched PropagateDocumentVersion, because it probably will have a deeper impact...

Looking forward to your comments...
(0069735)
mpv (developer)
2017-08-22 11:29

1. As you see in BinLDrivers_DocumentRetrievalDriver.hxx, myRelocTable belongs to the DocumentRetrievalDriver, not document. Such drivers are stored in the application and reused for each document opening. This is why "Clear" method is called for relocation table in the start and finish of reading process. Keeping handler to myHeaderData in BinObjMgt_RRelocationTable will prevent from freeing this object in memory after reading. So, I guess, myRelocTable.SetHeaderData(aHeaderData) must be called later, after myRelocTable is cleared, but before it is used in ReadSubTree.

2. Please move definition of methods from CDM_Application.hxx and BinObjMgt_RRelocationTable.hxx headers to .cxx or to .lxx files (but I guess for such methods there is no big necessity to make them inline, they are called not-often, really).

3. Please, don't use "tab" for lines-indent in CDM_Application.hxx and BinObjMgt_RRelocationTable.hxx. They should be replaced by two-spaces (as you did in other files).
(0069743)
BenjaminBihler (developer)
2017-08-22 14:10

You have convinced me. ;)

Should I hide the Clear() method or override it (make the method in NCollection_DataMap virtual)? For the code that I am adapting it is not absolutely necessary to make it virtual, since BinLDrivers_DocumentRetrievalDriver stores the relocation table as instance and not as pointer. Still it might be considered better practice...
(0069745)
git (administrator)
2017-08-22 14:49

Branch CR29014 has been updated by BenjaminBihler.

SHA-1: 0da2faefbc974f7d807cf53e9637d50f770775dd


Detailed log of new commits:

Author: Benjamin Bihler
Date: Tue Aug 22 13:34:16 2017 +0200

    0029014: Managing Binary Format Versions Is Not Possible for Own TDF_Attributes
    
    Moved method implementations to .cxx files.
    
    Replaced tabs by spaces.
    
    Clearing a BinObjMgt_RRelocationTable now nullifies the reference to the
    file header data and BinLDrivers_DocumentRetrievalDriver therefore sets
    the reference after the relocation table has been cleared before reading
    in the document subtree.

(0069766)
mpv (developer)
2017-08-23 10:14

I agree to do not touch commonly used DataMap and create an additional method in RRelocationTable class. This should just free the myHeadrData handle and be called additionally in BinLDrivers_DocumentRetrievalDriver::Clear.
(0069817)
BenjaminBihler (developer)
2017-08-24 10:57

This is yet another possibility. Still I would propose to leave it like it is:

- The Clear() method of BinObjMgt_RRelocationTable hides the Clear() method of NCollection_DataMap, but calls it at the end.
- Since only BinLDrivers_DocumentRetrievalDriver calls the relocation table's Clear() method and there the static type of the object is enough (it is not accessed through a pointer), there is no disadvantage in leaving the code like it is.
- To me it seems as if clearing a relocation table should always be done at the same time as nullifying the header data handle (why should anyone do one thing without the other?). In this case the Clear() method should take care of it.
- Actually the cleanest solution would be to make the Clear() method in NCollection_DataMap virtual. If relocation tables are to be stored and cleared in more general ways and places, this is IMHO the only correct way. But as long as this is not the case, I would leave the code like it is right now.

Do you agree? If yes, then the issue is resolved.
(0069865)
mpv (developer)
2017-08-25 09:35

For me it is ok. Please, test.
(0070119)
git (administrator)
2017-08-30 19:35

Branch CR29014 has been updated forcibly by inv.

SHA-1: f9609eb83f5705f5f6e7a8c9751a757a402cb9d1
(0070120)
bugmaster (administrator)
2017-08-31 10:34

Combination -
OCCT branch : CR29014 - SHA-1: f9609eb83f5705f5f6e7a8c9751a757a402cb9d1
Products branch : master
was compiled on Linux, MacOS and Windows platforms and tested on optimize mode.
http://jenkins-test-10.nnov.opencascade.com/view/CR29014-master-INV/ [^]

Number of compiler warnings:

OCCT :
Linux: 0 (0 on master)
Windows: 0 (0 on master)
MacOS : 0 (0 on master)

Products :
Linux: 4 (4 on master)
Windows: 0 (0 on master)
MacOS : 0 (0 on master)

Regressions/Differences/Improvements:
No regressions/differences

CPU differences:

Windows:
OCCT
Total CPU difference: 17330.010689098726 / 17295.75286949859 [+0.20%]
Producst
Total CPU difference: 7688.758886599963 / 7720.302288799964 [-0.41%]

Linux:
OCCT
Total CPU difference: 19451.89000000023 / 19456.560000000376 [-0.02%]
Products
Total CPU difference: 7703.40000000009 / 7725.120000000072 [-0.28%]

Image differences :
No differences that require special attention

Memory differences :
No differences that require special attention
(0071012)
git (administrator)
2017-09-29 17:11

Branch CR29014 has been deleted by kgv.

SHA-1: f9609eb83f5705f5f6e7a8c9751a757a402cb9d1

- Related Changesets
occt: master fe93e33a
Timestamp: 2017-08-18 07:40:19
Author: BenjaminBihler
Committer: bugmaster
Details ] Diff ]
0029014: Managing Binary Format Version Is Not Possible for Own TDF_Attributes

CDM_Application has been extended to provide application name and version.

Application name and version is stored by BinLDrivers_DocumentStorageDriver.

BinLDrivers_DocumentStorageDriver propagates application name and version
by passing it to BinMDataStd.

Made BinObjMgt_RRelocationTable store a handle to the header data of the file
begin read in to make it accessible by binary attribute drivers.

Undone storing application name and version as static fields in BinMDataStd
which is bad style and not thread-safe.

Moved method implementations to .cxx files.

Clearing a BinObjMgt_RRelocationTable now nullifies the reference to the
file header data and BinLDrivers_DocumentRetrievalDriver therefore sets
the reference after the relocation table has been cleared before reading
in the document subtree.
mod - src/BinLDrivers/BinLDrivers_DocumentRetrievalDriver.cxx Diff ] File ]
mod - src/BinLDrivers/BinLDrivers_DocumentStorageDriver.cxx Diff ] File ]
add - src/BinObjMgt/BinObjMgt_RRelocationTable.cxx Diff ] File ]
mod - src/BinObjMgt/BinObjMgt_RRelocationTable.hxx Diff ] File ]
mod - src/BinObjMgt/FILES Diff ] File ]
mod - src/CDM/CDM_Application.cxx Diff ] File ]
mod - src/CDM/CDM_Application.hxx Diff ] File ]
occt: master fe21f796
Timestamp: 2017-08-18 07:40:19
Author: BenjaminBihler
Committer: bugmaster
Details ] Diff ]
0029014: Managing Binary Format Version Is Not Possible for Own TDF_Attributes

CDM_Application has been extended to provide application name and version.

Application name and version is stored by BinLDrivers_DocumentStorageDriver.

BinLDrivers_DocumentStorageDriver propagates application name and version
by passing it to BinMDataStd.

Made BinObjMgt_RRelocationTable store a handle to the header data of the file
begin read in to make it accessible by binary attribute drivers.

Undone storing application name and version as static fields in BinMDataStd
which is bad style and not thread-safe.

Moved method implementations to .cxx files.

Clearing a BinObjMgt_RRelocationTable now nullifies the reference to the
file header data and BinLDrivers_DocumentRetrievalDriver therefore sets
the reference after the relocation table has been cleared before reading
in the document subtree.
mod - src/BinLDrivers/BinLDrivers_DocumentRetrievalDriver.cxx Diff ] File ]
mod - src/BinLDrivers/BinLDrivers_DocumentStorageDriver.cxx Diff ] File ]
add - src/BinObjMgt/BinObjMgt_RRelocationTable.cxx Diff ] File ]
mod - src/BinObjMgt/BinObjMgt_RRelocationTable.hxx Diff ] File ]
mod - src/BinObjMgt/FILES Diff ] File ]
mod - src/CDM/CDM_Application.cxx Diff ] File ]
mod - src/CDM/CDM_Application.hxx Diff ] File ]

- Issue History
Date Modified Username Field Change
2017-08-18 10:34 BenjaminBihler New Issue
2017-08-18 10:34 BenjaminBihler Assigned To => BenjaminBihler
2017-08-18 12:00 git Note Added: 0069565
2017-08-18 12:01 BenjaminBihler Assigned To BenjaminBihler => mpv
2017-08-18 12:01 BenjaminBihler Status new => resolved
2017-08-18 12:01 BenjaminBihler Steps to Reproduce Updated View Revisions
2017-08-18 16:44 mpv Relationship added related to 0028691
2017-08-18 16:55 mpv Note Added: 0069597
2017-08-18 16:55 mpv Assigned To mpv => BenjaminBihler
2017-08-18 16:55 mpv Status resolved => feedback
2017-08-18 17:28 BenjaminBihler Note Added: 0069675
2017-08-18 17:33 BenjaminBihler Assigned To BenjaminBihler => mpv
2017-08-18 18:33 abv Note Added: 0069677
2017-08-21 08:31 BenjaminBihler Note Added: 0069688
2017-08-21 09:18 abv Note Added: 0069690
2017-08-21 11:00 mpv Note Added: 0069699
2017-08-21 11:00 mpv Assigned To mpv => BenjaminBihler
2017-08-21 11:00 mpv Status feedback => assigned
2017-08-21 13:09 git Note Added: 0069705
2017-08-21 13:13 BenjaminBihler Note Added: 0069707
2017-08-21 13:14 BenjaminBihler Assigned To BenjaminBihler => mpv
2017-08-21 13:14 BenjaminBihler Status assigned => resolved
2017-08-22 11:29 mpv Note Added: 0069735
2017-08-22 11:29 mpv Assigned To mpv => BenjaminBihler
2017-08-22 11:29 mpv Status resolved => assigned
2017-08-22 14:10 BenjaminBihler Note Added: 0069743
2017-08-22 14:49 git Note Added: 0069745
2017-08-23 10:14 mpv Note Added: 0069766
2017-08-24 10:57 BenjaminBihler Note Added: 0069817
2017-08-24 17:49 abv Assigned To BenjaminBihler => mpv
2017-08-24 17:49 abv Status assigned => feedback
2017-08-25 09:35 mpv Note Added: 0069865
2017-08-25 09:35 mpv Assigned To mpv => bugmaster
2017-08-25 09:35 mpv Status feedback => resolved
2017-08-28 10:01 mpv Target Version 7.2.0 => 7.2.1
2017-08-29 18:43 abv Status resolved => reviewed
2017-08-30 19:35 git Note Added: 0070119
2017-08-31 10:34 bugmaster Note Added: 0070120
2017-08-31 10:34 bugmaster Status reviewed => tested
2017-08-31 10:35 bugmaster Test case number => Not required
2017-09-21 19:33 bugmaster Changeset attached => occt master fe93e33a
2017-09-21 19:33 bugmaster Status tested => verified
2017-09-21 19:33 bugmaster Resolution open => fixed
2017-09-21 19:49 apn Target Version 7.2.1 => 7.2.0
2017-09-22 12:00 bugmaster Target Version 7.2.0 => 7.2.1
2017-09-29 13:26 bugmaster Changeset attached => occt master fe21f796
2017-09-29 17:11 git Note Added: 0071012
2017-10-09 12:14 abv Relationship added related to 0029195


Copyright © 2000 - 2017 MantisBT Team
Powered by Mantis Bugtracker