MantisBT
Mantis Bug Tracker Workflow

View Issue Details Jump to Notes ] Related Changesets ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0024755Community[OCCT] OCCT:Application Frameworkpublic2014-03-23 11:592017-09-15 15:40
ReporterRoman Lygin 
Assigned Tompv 
PrioritynormalSeverityminor 
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version[OCCT] 6.7.0 
Target Version[OCCT] 6.8.0Fixed in Version[OCCT] 6.8.0 
Summary0024755: TDF_Label::AddAttribute() reverses the order of added attributes
DescriptionTDF_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 ReproduceSee the reproducer.
TagsNo tags attached.
Test case numberbugs caf(015) bug24755
Attached Filescxx file icon tdf_label-addattribute-test.cxx (562 bytes) 2014-03-23 11:59

- Relationships
parent of 0028946closedbugmaster Open CASCADE Exception on Undo 
related to 0029116verifiedbugmaster Community [Regression] OCAF attributes insertion order is violated again 

-  Notes
(0028433)
Roman Lygin (developer)
2014-03-23 12:50

The fix has been pushed into the repository
(0028459)
szy (administrator)
2014-03-25 12:21

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
(0028460)
Roman Lygin (developer)
2014-03-25 12:55

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.
(0028470)
szy (administrator)
2014-03-25 16:07

Ok, accepted.
Reviewed, to be tested.
szy
(0028493)
mkv (tester)
2014-03-26 16:28

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.
(0028498)
Roman Lygin (developer)
2014-03-26 17:49

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.
(0028618)
szy (administrator)
2014-04-02 12:16
edited on: 2014-04-02 14:26

1)case bug361 ==> it's not reproduced
2)case bug24047 ==> Fix integrated in branch CR24755 (file XDEDRAW.cxx).
Review it, please.
szy

(0028632)
mkv (tester)
2014-04-02 15:44

Dear szy,
could you please rebase branch CR24755, there are conflict files.
(0028638)
mkv (tester)
2014-04-02 16:42

Done.
(0028648)
mkv (tester)
2014-04-03 12:25

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.
(0028649)
mkv (tester)
2014-04-03 12:26

Dear szy,

Regression ( bugs/caf(015)/bug361 ) reproduced only on Linux platform.
(0028673)
szy (administrator)
2014-04-04 11:36

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.
(0028680)
mkv (tester)
2014-04-04 14:06

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
(0028686)
szy (administrator)
2014-04-04 16:12

Added additional fix for case 'bug361'
Test it, please.
(0028687)
szy (administrator)
2014-04-04 16:13

Fixed.
(0028688)
szy (administrator)
2014-04-04 16:13

Test it, please.
(0028725)
apn (administrator)
2014-04-08 10:48

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.
(0033135)
Roman Lygin (developer)
2014-10-15 15:13

Confirmed as closed in 6.8.0 beta.
(0070592)
mpv (developer)
2017-09-15 15:37

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.

- Related Changesets
occt: master d01cc61d
Timestamp: 2014-04-10 13:50:49
Author: szy
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.
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 ]

- Issue History
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 mkv Note Added: 0028493
2014-03-26 16:29 mkv Test case number => bugs caf(015) bug24755
2014-03-26 16:29 mkv Assigned To mkv => szy
2014-03-26 16:29 mkv 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 View Revisions
2014-04-02 15:44 mkv Note Added: 0028632
2014-04-02 15:45 mkv Assigned To mkv => szy
2014-04-02 15:45 mkv Status reviewed => feedback
2014-04-02 16:42 mkv Note Added: 0028638
2014-04-02 16:42 mkv Assigned To szy => mkv
2014-04-02 16:42 mkv Status feedback => resolved
2014-04-02 16:43 mkv Status resolved => reviewed
2014-04-03 12:25 mkv Note Added: 0028648
2014-04-03 12:26 mkv Note Added: 0028649
2014-04-03 12:26 mkv Assigned To mkv => szy
2014-04-03 12:26 mkv 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 mkv Note Added: 0028680
2014-04-04 14:08 mkv Assigned To mkv => szy
2014-04-04 14:08 mkv 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 abv Target Version => 6.8.0
2014-04-04 17:57 mkv 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 user533 Fixed in Version => 6.8.0
2014-11-11 12:59 user533 Status verified => closed
2017-09-15 12:12 Roman Lygin Relationship added related to 0029116
2017-09-15 15:26 abv Relationship added parent of 0028946
2017-09-15 15:37 mpv Assigned To apn => mpv
2017-09-15 15:37 mpv Note Added: 0070592
2017-09-15 15:37 mpv Status closed => feedback
2017-09-15 15:37 mpv Resolution fixed => reopened
2017-09-15 15:37 mpv Status feedback => assigned
2017-09-15 15:40 bugmaster Status assigned => closed
2017-09-15 15:40 bugmaster Resolution reopened => fixed


Copyright © 2000 - 2018 MantisBT Team
Powered by Mantis Bugtracker