View Issue Details

IDProjectCategoryView StatusLast Update
0029014CommunityOCCT:Application Frameworkpublic2018-06-29 21:19
ReporterBenjaminBihler Assigned Tobugmaster  
PrioritynormalSeverityminor 
Status closedResolutionfixed 
Product Version7.1.0 
Target Version7.3.0Fixed in Version7.3.0 
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

Relationships

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

Activities

git

2017-08-18 12:00

administrator   ~0069565

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.

mpv

2017-08-18 16:55

developer   ~0069597

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.

BenjaminBihler

2017-08-18 17:28

developer   ~0069675

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!

abv

2017-08-18 18:33

manager   ~0069677

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.

BenjaminBihler

2017-08-21 08:31

developer   ~0069688

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?)

abv

2017-08-21 09:18

manager   ~0069690

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

mpv

2017-08-21 11:00

developer   ~0069699

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

git

2017-08-21 13:09

administrator   ~0069705

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.

BenjaminBihler

2017-08-21 13:13

developer   ~0069707

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

mpv

2017-08-22 11:29

developer   ~0069735

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

BenjaminBihler

2017-08-22 14:10

developer   ~0069743

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

git

2017-08-22 14:49

administrator   ~0069745

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.

mpv

2017-08-23 10:14

developer   ~0069766

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.

BenjaminBihler

2017-08-24 10:57

developer   ~0069817

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.

mpv

2017-08-25 09:35

developer   ~0069865

For me it is ok. Please, test.

git

2017-08-30 19:35

administrator   ~0070119

Branch CR29014 has been updated forcibly by inv.

SHA-1: f9609eb83f5705f5f6e7a8c9751a757a402cb9d1

bugmaster

2017-08-31 10:34

administrator   ~0070120

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

git

2017-09-29 17:11

administrator   ~0071012

Branch CR29014 has been deleted by kgv.

SHA-1: f9609eb83f5705f5f6e7a8c9751a757a402cb9d1

BenjaminBihler

2018-02-09 11:09

developer   ~0073899

May I ask what it means that the target version is 7.3.0? Will the change be removed from the current master?

mpv

2018-02-13 14:57

developer   ~0073955

This patch is still in the master, but it is not added to the version 7.2.1, containing some specific bug-fixes only.

Related Changesets

occt: master fe21f796

2017-08-18 07:40:19

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.
Affected Issues
0029014
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
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.3.0
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.3.0 => 7.2.0
2017-09-22 12:00 bugmaster Target Version 7.2.0 => 7.3.0
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
2018-02-08 21:58 abv Changeset removed occt master fe93e33a =>
2018-02-08 22:03 abv Target Version 7.3.0 => 7.4.0
2018-02-09 11:09 BenjaminBihler Note Added: 0073899
2018-02-13 14:57 mpv Note Added: 0073955
2018-02-20 12:58 aiv Target Version 7.4.0 => 7.3.0
2018-06-29 21:15 aiv Fixed in Version => 7.3.0
2018-06-29 21:19 aiv Status verified => closed