View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0024755CommunityOCCT:Application Frameworkpublic2014-03-23 11:592017-09-15 15:40
ReporterRoman Lygin Assigned Tompv 
PrioritynormalSeverityminor 
Status closedResolutionfixed 
Product Version6.7.0 
Target Version6.8.0Fixed in Version6.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 Files

  • tdf_label-addattribute-test.cxx (562 bytes)

Relationships

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

Activities

Roman Lygin

2014-03-23 11:59

developer  

tdf_label-addattribute-test.cxx (562 bytes)

Roman Lygin

2014-03-23 12:50

developer   ~0028433

The fix has been pushed into the repository

szy

2014-03-25 12:21

manager   ~0028459

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

Roman Lygin

2014-03-25 12:55

developer   ~0028460

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.

szy

2014-03-25 16:07

manager   ~0028470

Ok, accepted.
Reviewed, to be tested.
szy

mkv

2014-03-26 16:28

tester   ~0028493

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.

Roman Lygin

2014-03-26 17:49

developer   ~0028498

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.

szy

2014-04-02 12:16

manager   ~0028618

Last edited: 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

mkv

2014-04-02 15:44

tester   ~0028632

Dear szy,
could you please rebase branch CR24755, there are conflict files.

mkv

2014-04-02 16:42

tester   ~0028638

Done.

mkv

2014-04-03 12:25

tester   ~0028648

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.

mkv

2014-04-03 12:26

tester   ~0028649

Dear szy,

Regression ( bugs/caf(015)/bug361 ) reproduced only on Linux platform.

szy

2014-04-04 11:36

manager   ~0028673

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.

mkv

2014-04-04 14:06

tester   ~0028680

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

szy

2014-04-04 16:12

manager   ~0028686

Added additional fix for case 'bug361'
Test it, please.

szy

2014-04-04 16:13

manager   ~0028687

Fixed.

szy

2014-04-04 16:13

manager   ~0028688

Test it, please.

apn

2014-04-08 10:48

administrator   ~0028725

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.

Roman Lygin

2014-10-15 15:13

developer   ~0033135

Confirmed as closed in 6.8.0 beta.

mpv

2017-09-15 15:37

developer   ~0070592

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

2014-04-10 13:50:49

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

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
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 aiv Fixed in Version => 6.8.0
2014-11-11 12:59 aiv 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