View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0024755 | Community | OCCT:Application Framework | public | 2014-03-23 11:59 | 2017-09-15 15:40 |
| Reporter | Roman Lygin | Assigned To | |||
| Priority | normal | Severity | minor | ||
| Status | closed | Resolution | fixed | ||
| Product Version | 6.7.0 | ||||
| Target Version | 6.8.0 | Fixed in Version | 6.8.0 | ||
| Summary | 0024755: TDF_Label::AddAttribute() reverses the order of added attributes | ||||
| Description | TDF_Label::AddAttribute() prepends (instead of append) the attribute into the list of attributes. This leads to a reversed order when iterating, saving and restoring the file. Appending enables determinism (document contents are reproduced) and facilitates debugging. Note that theoretically there can be minor performance hit, as finding the last attribute via a linked list of attributes is more expensive than just inserting into its beginning. However, for short attribute lists the difference is negligible and for large documents the extra cost is negligible comparing to payload of creating the document content itself. | ||||
| Steps To Reproduce | See the reproducer. | ||||
| Tags | No tags attached. | ||||
| Test case number | bugs caf(015) bug24755 | ||||
|
|
tdf_label-addattribute-test.cxx (562 bytes) |
|
|
The fix has been pushed into the repository |
|
|
It is not very clear the real reason of the improvement. Could you give more arguments. The current message: a)"Appending enables determinism (document contents are reproduced) and facilitates debugging." seems not very convincing. Determinism exists as in current state (back order) as in the proposed one (preorder). b)"facilitates debugging"? It is questionable. I have opposite impression that the current order (the last is the first) facilitates debugging. I.e. it is more fast to find the last appended attribute as you already see it. Very probably it depends on situation and subjective habits. Could you provide some example when the back order of the attributes on Label leads to any drawback? Thanks |
|
|
Let me offer more details. 1. In the current (prepend) implementation you reverse the order each time you add attributes, and open and save a document. Consider the following scenario: - you create a document in memory and call AddAttribute() with A1, A2, ..., An. - in reality, you receive the reversed order in memory: An, An-1, ..., A1. If you attach a doc browser, or otherwise try to explore the document, this confuses you and you don't understand why it is so. - then you save this document to a file on a disk (e.g. XML) - you see the revered order An, ..., A1. - you retrieve the document back, expecting that you will see the same order as you saw in XML and in the doc brower. Instead you get reversing of a reversed order - A1, ..., An. This confuses you again, and you start loosing track of what is happening - the same document is being explored differently without any (evil) actions on your side. - now you save the document again, and in the XML file you see A1, ..., An. Crazy! So the same document created/saved/retrieved/saved produces different results. This is not deterministic and confusing. 2. On debugability: Imagine you need to debug how adding a complex attribute affects the structure of your large document (say dozens of MB's) which is saved on your disk. For instance, you might receive it from your customer. You open the document, add an attribute, save the document back and want to 'diff' the XML files expecting to see only real differences. Instead you see a myriad of changes - all the XML nodes got changed - at least the attributes' order and their id's. So you waste your time and accuracy of your debug analysis, as you try to locate where the real change is. This is based on real experience and I hope this provides sufficient rationale of the proposed change. You might want to add TDF_Label::PrependAttribute() for any potential use case where current behavior might be important, but I do not see strong reason for that. So this should be your call. |
|
|
Ok, accepted. Reviewed, to be tested. szy |
|
|
Dear BugMaster, Branch CR24755 (and products from GIT master) was compiled on Linux and Windows platforms and tested. SHA-1: b59940a09a48893c05c4be530d202ab33229a114 Number of compiler warnings: occt component : Linux: 29 (29 on master) Windows: 0 (0 on master) products component : Linux: 12 (12 on master) Windows: 2 (2 on master) Regressions/Differences: http://occt-tests/CR24755-master-occt/Debian60-64/summary.html http://occt-tests/CR24755-master-occt/Windows-32-VC9/summary.html bugs caf(015) bug361, bug24047 Testing cases: http://occt-tests/CR24755-master-occt/Debian60-64/bugs/caf/bug24755.html bugs caf(015) bug24755: OK Testing on Linux: Total MEMORY difference: 391462820 / 391452264 Total CPU difference: 50769.80999999987 / 51358.129999999655 Testing on Windows: Total MEMORY difference: 395458372 / 395804064 Total CPU difference: 32027.09375 / 35037.53125 There are no differences in images found by testdiff. |
|
|
bug361 does pass in my environment. The C++ code looks pretty straightforward as well. Not sure what may trigger an error. 0024047 is an infamous bug related to the order of destroying labels, and intensively discussed in the past. I let Sergey (SZY) to decide how that should be addressed - perhaps redesigning the logic of TPrsStd attributes to make them non-sensitive to the way the attributes are stored and/or how the document gets destroyed. |
|
|
1)case bug361 ==> it's not reproduced 2)case bug24047 ==> Fix integrated in branch CR24755 (file XDEDRAW.cxx). Review it, please. szy |
|
|
Dear szy, could you please rebase branch CR24755, there are conflict files. |
|
|
Done. |
|
|
Dear BugMaster, Branch CR24755 (and products from GIT master) was compiled on Linux and Windows platforms and tested. SHA-1: 80044205de447a0ad8480e8c9eb7fd5c3f4d58c5 Number of compiler warnings: occt component : Linux: 27 (27 on master) Windows: 0 (0 on master) products component : Linux: 12 (12 on master) Windows: 2 (2 on master) Regressions/Differences: http://occt-tests/CR24755-master-occt/Debian60-64/bugs/caf/bug361.html bugs caf(015) bug361 Testing cases: http://occt-tests/CR24755-master-occt/Debian60-64/bugs/caf/bug24755.html http://occt-tests/CR24755-master-occt/Windows-32-VC9/bugs/caf/bug24755.html bugs caf(015) bug24755: OK Testing on Linux: Total MEMORY difference: 393479608 / 393216140 Total CPU difference: 52314.58000000013 / 57189.0499999999 Testing on Windows: Total MEMORY difference: 422505024 / 414274688 Total CPU difference: 37486.34375 / 42798.984375 There are no differences in images found by testdiff. |
|
|
Dear szy, Regression ( bugs/caf(015)/bug361 ) reproduced only on Linux platform. |
|
|
The opened Document is not closed by the <bug361>. I suggest to add at the end of the script OCAF command: Close D and test the case once again. |
|
|
Dear szy, There is the same problem: http://occt-tests/CR24755-master-occt/Debian60-64/bugs/caf/bug361.html pload QAcommands 1 NewDocument D MDTV-Standard document D created 0:1 OCC361 D 0 Close D pure virtual method called terminate called without an active exception |
|
|
Added additional fix for case 'bug361' Test it, please. |
|
|
Fixed. |
|
|
Test it, please. |
|
|
Dear BugMaster, Branch CR24755 (and products from GIT master) was compiled on Linux and Windows platforms and tested. SHA-1: 39b050e152618e1dc0254f580a0353781c9f0ed5 Number of compiler warnings: occt component : Linux: 21 (21 on master) Windows: 0 (0 on master) products component : Linux: 12 (12 on master) Windows: 2 (2 on master) Regressions/Differences: No regressions/differences Testing cases: bugs caf bug24755 - OK http://occt-tests/CR24755-master-occt/Debian60-64/bugs/caf/bug24755.html http://occt-tests/CR24755-master-occt/Windows-32-VC9/bugs/caf/bug24755.html Testing on Linux: Total MEMORY difference: 351077656 / 352026040 Total CPU difference: 56576.029999999795 / 50789.29000000014 Testing on Windows: Total MEMORY difference: 380380576 / 381211776 Total CPU difference: 34861.171875 / 36678.890625 There are no differences in images found by testdiff. |
|
|
Confirmed as closed in 6.8.0 beta. |
|
|
Ok, after some discussions it is decided to think again about this problem. May be the order of attributes may be reverted during open or save of the document to keep the document's content same each time. Perhaps performance become not so bad. |
|
occt: master d01cc61d 2014-04-10 13:50:49 Committer: apn Details Diff |
0024755: TDF_Label::AddAttribute() reverses the order of added attributes Test case and new draw command for issue CR24755 Fix of regression linked with test case bug24047. Fix of bug361 regression on Linux. |
Affected Issues 0024755 |
|
| mod - src/QABugs/QABugs_1.cxx | Diff File | ||
| mod - src/QABugs/QABugs_19.cxx | Diff File | ||
| mod - src/TDF/TDF_Label.cxx | Diff File | ||
| mod - src/XDEDRAW/XDEDRAW.cxx | Diff File | ||
| add - tests/bugs/caf/bug24755 | Diff File | ||
| mod - tests/bugs/caf/bug361 | Diff File | ||
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2014-03-23 11:59 | Roman Lygin | New Issue | |
| 2014-03-23 11:59 | Roman Lygin | Assigned To | => szy |
| 2014-03-23 11:59 | Roman Lygin | File Added: tdf_label-addattribute-test.cxx | |
| 2014-03-23 12:50 | Roman Lygin | Note Added: 0028433 | |
| 2014-03-23 12:50 | Roman Lygin | Status | new => resolved |
| 2014-03-25 12:21 | szy | Note Added: 0028459 | |
| 2014-03-25 12:21 | szy | Assigned To | szy => Roman Lygin |
| 2014-03-25 12:21 | szy | Status | resolved => feedback |
| 2014-03-25 12:55 | Roman Lygin | Note Added: 0028460 | |
| 2014-03-25 12:55 | Roman Lygin | Assigned To | Roman Lygin => szy |
| 2014-03-25 12:55 | Roman Lygin | Status | feedback => resolved |
| 2014-03-25 16:07 | szy | Note Added: 0028470 | |
| 2014-03-25 16:07 | szy | Assigned To | szy => mkv |
| 2014-03-25 16:07 | szy | Status | resolved => reviewed |
| 2014-03-26 16:28 |
|
Note Added: 0028493 | |
| 2014-03-26 16:29 |
|
Test case number | => bugs caf(015) bug24755 |
| 2014-03-26 16:29 |
|
Assigned To | mkv => szy |
| 2014-03-26 16:29 |
|
Status | reviewed => assigned |
| 2014-03-26 17:49 | Roman Lygin | Note Added: 0028498 | |
| 2014-04-02 12:16 | szy | Note Added: 0028618 | |
| 2014-04-02 12:16 | szy | Assigned To | szy => vro |
| 2014-04-02 12:16 | szy | Status | assigned => resolved |
| 2014-04-02 12:23 | vro | Assigned To | vro => mkv |
| 2014-04-02 12:23 | vro | Status | resolved => reviewed |
| 2014-04-02 14:26 | szy | Note Edited: 0028618 | |
| 2014-04-02 15:44 |
|
Note Added: 0028632 | |
| 2014-04-02 15:45 |
|
Assigned To | mkv => szy |
| 2014-04-02 15:45 |
|
Status | reviewed => feedback |
| 2014-04-02 16:42 |
|
Note Added: 0028638 | |
| 2014-04-02 16:42 |
|
Assigned To | szy => mkv |
| 2014-04-02 16:42 |
|
Status | feedback => resolved |
| 2014-04-02 16:43 |
|
Status | resolved => reviewed |
| 2014-04-03 12:25 |
|
Note Added: 0028648 | |
| 2014-04-03 12:26 |
|
Note Added: 0028649 | |
| 2014-04-03 12:26 |
|
Assigned To | mkv => szy |
| 2014-04-03 12:26 |
|
Status | reviewed => assigned |
| 2014-04-04 11:36 | szy | Note Added: 0028673 | |
| 2014-04-04 11:36 | szy | Status | assigned => resolved |
| 2014-04-04 11:37 | szy | Assigned To | szy => mkv |
| 2014-04-04 11:37 | szy | Status | resolved => reviewed |
| 2014-04-04 14:06 |
|
Note Added: 0028680 | |
| 2014-04-04 14:08 |
|
Assigned To | mkv => szy |
| 2014-04-04 14:08 |
|
Status | reviewed => assigned |
| 2014-04-04 16:12 | szy | Note Added: 0028686 | |
| 2014-04-04 16:13 | szy | Note Added: 0028687 | |
| 2014-04-04 16:13 | szy | Status | assigned => resolved |
| 2014-04-04 16:13 | szy | Note Added: 0028688 | |
| 2014-04-04 16:13 | szy | Assigned To | szy => mkv |
| 2014-04-04 16:13 | szy | Status | resolved => reviewed |
| 2014-04-04 16:26 |
|
Target Version | => 6.8.0 |
| 2014-04-04 17:57 |
|
Assigned To | mkv => apn |
| 2014-04-08 10:48 | apn | Note Added: 0028725 | |
| 2014-04-08 10:48 | apn | Assigned To | apn => bugmaster |
| 2014-04-08 10:48 | apn | Status | reviewed => tested |
| 2014-04-11 14:43 | apn | Changeset attached | => occt master d01cc61d |
| 2014-04-11 14:43 | apn | Assigned To | bugmaster => apn |
| 2014-04-11 14:43 | apn | Status | tested => verified |
| 2014-04-11 14:43 | apn | Resolution | open => fixed |
| 2014-10-15 15:13 | Roman Lygin | Note Added: 0033135 | |
| 2014-11-11 12:44 |
|
Fixed in Version | => 6.8.0 |
| 2014-11-11 12:59 |
|
Status | verified => closed |
| 2017-09-15 12:12 | Roman Lygin | Relationship added | related to 0029116 |
| 2017-09-15 15:26 |
|
Relationship added | parent of 0028946 |
| 2017-09-15 15:37 |
|
Assigned To | apn => mpv |
| 2017-09-15 15:37 |
|
Note Added: 0070592 | |
| 2017-09-15 15:37 |
|
Status | closed => feedback |
| 2017-09-15 15:37 |
|
Resolution | fixed => reopened |
| 2017-09-15 15:37 |
|
Status | feedback => assigned |
| 2017-09-15 15:40 | bugmaster | Status | assigned => closed |
| 2017-09-15 15:40 | bugmaster | Resolution | reopened => fixed |
