View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0029014 | Community | OCCT:Application Framework | public | 2017-08-18 10:34 | 2018-06-29 21:19 |
| Reporter | BenjaminBihler | Assigned To | bugmaster | ||
| Priority | normal | Severity | minor | ||
| Status | closed | Resolution | fixed | ||
| Product Version | 7.1.0 | ||||
| Target Version | 7.3.0 | Fixed in Version | 7.3.0 | ||
| Summary | 0029014: Managing Binary Format Versions Is Not Possible for Own TDF_Attributes | ||||
| Description | I 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 Reproduce | Not required | ||||
| Tags | No tags attached. | ||||
| Test case number | Not required | ||||
| related to | 0029195 | closed | Open CASCADE | OCAF - ensure thread safety for different documents |
|
|
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. |
|
|
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. |
|
|
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! |
|
|
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. |
|
|
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?) |
|
|
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 |
|
|
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). |
|
|
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. |
|
|
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... |
|
|
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). |
|
|
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... |
|
|
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. |
|
|
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. |
|
|
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. |
|
|
For me it is ok. Please, test. |
|
|
Branch CR29014 has been updated forcibly by inv. SHA-1: f9609eb83f5705f5f6e7a8c9751a757a402cb9d1 |
|
|
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 |
|
|
Branch CR29014 has been deleted by kgv. SHA-1: f9609eb83f5705f5f6e7a8c9751a757a402cb9d1 |
|
|
May I ask what it means that the target version is 7.3.0? Will the change be removed from the current master? |
|
|
This patch is still in the master, but it is not added to the version 7.2.1, containing some specific bug-fixes only. |
|
occt: master fe21f796 2017-08-18 07:40:19 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 | ||
| 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 |
|
Note Added: 0069597 | |
| 2017-08-18 16:55 |
|
Assigned To | mpv => BenjaminBihler |
| 2017-08-18 16:55 |
|
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 |
|
Note Added: 0069677 | |
| 2017-08-21 08:31 | BenjaminBihler | Note Added: 0069688 | |
| 2017-08-21 09:18 |
|
Note Added: 0069690 | |
| 2017-08-21 11:00 |
|
Note Added: 0069699 | |
| 2017-08-21 11:00 |
|
Assigned To | mpv => BenjaminBihler |
| 2017-08-21 11:00 |
|
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 |
|
Note Added: 0069735 | |
| 2017-08-22 11:29 |
|
Assigned To | mpv => BenjaminBihler |
| 2017-08-22 11:29 |
|
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 |
|
Note Added: 0069766 | |
| 2017-08-24 10:57 | BenjaminBihler | Note Added: 0069817 | |
| 2017-08-24 17:49 |
|
Assigned To | BenjaminBihler => mpv |
| 2017-08-24 17:49 |
|
Status | assigned => feedback |
| 2017-08-25 09:35 |
|
Note Added: 0069865 | |
| 2017-08-25 09:35 |
|
Assigned To | mpv => bugmaster |
| 2017-08-25 09:35 |
|
Status | feedback => resolved |
| 2017-08-28 10:01 |
|
Target Version | 7.2.0 => 7.3.0 |
| 2017-08-29 18:43 |
|
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 |
|
Relationship added | related to 0029195 |
| 2018-02-08 21:58 |
|
Changeset removed | occt master fe93e33a => |
| 2018-02-08 22:03 |
|
Target Version | 7.3.0 => 7.4.0 |
| 2018-02-09 11:09 | BenjaminBihler | Note Added: 0073899 | |
| 2018-02-13 14:57 |
|
Note Added: 0073955 | |
| 2018-02-20 12:58 |
|
Target Version | 7.4.0 => 7.3.0 |
| 2018-06-29 21:15 |
|
Fixed in Version | => 7.3.0 |
| 2018-06-29 21:19 |
|
Status | verified => closed |
