MantisBT
Mantis Bug Tracker Workflow

View Issue Details Jump to Notes ] Related Changesets ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0025746Open CASCADE[OCCT] OCCT:Foundation Classespublic2015-01-23 16:362015-05-14 16:28
Reporteragv 
Assigned Tobugmaster 
PriorityhighSeverityminor 
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version 
Target Version[OCCT] 6.9.0Fixed in Version[OCCT] 6.9.0 
Summary0025746: Excessive memory use in math_Matrix
DescriptionClass math_Matrix (very useful to deal with matrix algebra) allocates the space for numbers in encapsulated class math_DoubleTabOfReal. The latter one contains as data members:

Standard_Address AddrBuf[32];
Standard_Real Buf[512];

When the space necessary to hold raw addresses and/or matrix values exceeds these arrays then it is taken from the heap, otherwise the intenal arrays "AddrBuf" and "Buf" are used.

Thus, regardless of the actual size, each matrix occupies more than 4 kilobytes with most of this memory staying unused, this has a negative effect on performance. It is better to implement arrays with configurable sizes, like NCollection_Array1<Standard_Address> and NCollection_Array1<Standard_Real>. To maintain the best performance, the constructor of math_DoubleTab (and math_Matrix as well) can receive an allocator as additinal parameter.
Steps To ReproduceNot required
TagsNo tags attached.
Test case numberNot needed
Attached Fileshtml file icon diff.html (31,825 bytes) 2015-04-14 19:04

- Relationships
related to 0024044closedbugmaster Community Performance improvements: Foundation Classes (math) 
related to 0025794closedbugmaster Community Boolean operation crashes, when running from a new thread 

-  Notes
(0037546)
abv (manager)
2015-02-14 22:58

Alexander, I deem this design is exactly aimed to improve performance, since for small matrices (which is often the case) stack will be used, thus no dynamic memory allocation will be made. I believe if we switch to using dynamic memory in all cases (your advice to use NCollection_Array1) will harm performance. What we can do, IMHO, is to decrease the size of local buffer: in most cases the matrix is 3x3 or 4x4.

The question is what will be the best compromise between size of local buffer and performance. In this regard, I do not see why 4KB per matrix instance is too much: in my understanding, in most cases algorithms should use just a few matrices at a time, thus this should not be too much. Can you explain better your particular use case where you have found a problem?
(0037548)
agv (developer)
2015-02-15 02:28

The problem is not in terms of crash or slow processing, it is something like memory leak: in most cases you would not notice it but somehow the available memory becomes considerably smaller than expected, due to the memory fragmentation. We should not forget that math_Matrix uses heap memory when it is part of some complex class that is managed by Handle.

This is a bad design when it requires to define a compromise size vs. performance. Because in such basic classes we should not make assumptions about the memory model and possible number of allocations. A correct design should always allow the best performance whether an algorithm uses a few matrices or a few million matrices.

For that all logic relevant to the optimization of memory allocations/deallocations should be provided by a relevant allocator that is chosen by developer according to the particular situation. Classes like math_Matrix should receive an allocator to request exactly the amount of memory that is needed. It does not matter if the needed size is only 4 numbers, such a small size can be efficiently handled by certain kinds of allocator. Also, this will eliminate the need to manage the local buffer size with all comparisons: the math class should only call Allocate() in its constructor and Free() in its destructor. The code will become shorter and clearer.

I should correct my initial suggestion about NCollection_Array1. This array does not use allocator, so it will be better to modify the actual class math_DoubleTab in such way that it would always call myAllocator->Allocate() for the calculated sizes of internal arrays.
(0038215)
msv (developer)
2015-03-09 19:39

I have found the bug 0024044, in the scope of which allocation on stack has been introduced.
Also, there is a topic on the forum (http://dev.opencascade.org/index.php?q=node/1067 [^]), where the crashes on iOS can be explained by this behavior, as the author tells the stack of a new thread on iOS is only 512K.
(0039524)
msv (developer)
2015-04-09 18:09

It is proposed to reduce the size of the field
Buf : Real[512];
to 16 items.
Also it is needed to get rid of indirection connected with storage of addresses of array rows:
AddrBuf : Address[32];
For that, throw away the field AddrBuf, and make necessary changes in the code.
(0039535)
Roman Lygin (updater)
2015-04-09 20:01

Folks,
Please make sure you gather performance data before and after the change, so that you can make data-based decision for a trade-off between performance and footprint.
Thank you,
Roman
(0039716)
git (administrator)
2015-04-14 18:46

Branch CR25746 has been created by dbv.

SHA-1: 56e113780b12d231316405d8aa67ac045036d75a


Detailed log of new commits:

Author: dbv
Date: Tue Apr 14 18:46:12 2015 +0300

    math_DoubleTab now statically allocates only 16 items for Buf
    Removed indirection table from math_DoubleTab
(0039718)
git (administrator)
2015-04-14 18:57

Branch CR25746 has been updated forcibly by dbv.

SHA-1: 88120f68b73cb7c12b38e52cf07ce73ec9e234ef
(0039720)
dbv (developer)
2015-04-14 19:03

(patched / IR-2015-04-09)
Total MEMORY difference: 80244408 / 80285317 [-0.05%]
Total CPU difference: 26907.083680197204 / 26606.93775619701 [+1.13%]

CPU v3d vertex_edge: 28.766584400000003 / 14.539293199999998 [+97.85%]
CPU v3d vertex_face: 27.970979299999996 / 14.024489900000003 [+99.44%]
CPU v3d vertex_solid: 28.033379699999998 / 14.180490899999995 [+97.69%]
CPU v3d vertex_wire: 27.736977800000012 / 13.962089499999998 [+98.66%]
CPU v3d wire: 13.182084499999998 / 7.2696466000000015 [+81.33%]
CPU v3d wire_solid K4: 0.7176046 / 0.156001 [+360.00%]
CPU v3d wire_solid: 27.2377746 / 13.993289699999995 [+94.65%]

As you can see there is a little difference globaly, but some tests deserve more detailed consideration.

Full diff attached.

Dear msv,

could you please review patch in occt branch CR25746
(0039751)
msv (developer)
2015-04-15 11:39

Remarks:

src\math\math_DoubleTab.cxx
- line 95: it is better to use directly the field Addr as the address to free instead of computation.
(0039768)
msv (developer)
2015-04-15 12:31

One more remark concerns patch preparation rules. Please, study the topic http://dev.opencascade.org/doc/overview/html/occt_dev_guides__contribution_workflow.html#occt_contribution_workflow_2_4 [^]

In particular, the following rule:

The first line of the first commit message should contain the Summary of the issue (starting with its ID followed by colon, e.g. "0022943: Bug TDataXtd_PatternStd"). The consequent lines should contain a description of the changes made. If more than one commit has been made, the commit messages should contain description of the changes made.
(0039772)
git (administrator)
2015-04-15 12:51

Branch CR25746 has been updated forcibly by dbv.

SHA-1: 22165fce5743a6db8211130dc2e6d1fd1349d24d
(0039775)
dbv (developer)
2015-04-15 13:01

Patch has been updated according to remarks.

Tests mentioned in note http://tracker.dev.opencascade.org/view.php?id=25746#c39720 [^] does not use math_DoubleTab (as it turned out after the investigation by msv).

Dear msv, could you please review latest patch in occt branch CR25746: http://git.dev.opencascade.org/gitweb/?p=occt.git;a=commitdiff;h=CR25746 [^]
(0039776)
msv (developer)
2015-04-15 13:18

Reviewed.
(0039785)
git (administrator)
2015-04-15 15:13

Branch CR25746 has been updated forcibly by mkv.

SHA-1: 970c85c0a719abd1dd2eebd1d57254be15b12671
(0039833)
mkv (tester)
2015-04-16 12:06

Dear BugMaster,
Branch CR25746 was rebased on current master of occt git-repository.
(0039834)
mkv (tester)
2015-04-16 12:07

Dear BugMaster,
Branch CR25746 from occt git-repository (and master from products git-repository) was compiled on Linux, MacOS and Windows platforms and tested on Release mode.
SHA-1: 970c85c0a719abd1dd2eebd1d57254be15b12671

Number of compiler warnings:

occt component :
Linux: 18 (18 on master)
Windows: 0 (0 on master)

products component :
Linux: 4 (4 on master)
Windows: 0 (0 on master)

Regressions/Differences:
No regressions/differences

Testing cases:
Not needed

Testing on Linux:
occt component :
Total MEMORY difference: 93969520 / 94236289 [-0.28%]
Total CPU difference: 54618.00999999947 / 56192.6999999997 [-2.80%]
products component :
Total MEMORY difference: 23623592 / 23635163 [-0.05%]
Total CPU difference: 17956.03999999999 / 19024.899999999965 [-5.62%]

Testing on Windows:
occt component :
Total MEMORY difference: 57126932 / 57127628 [-0.00%]
Total CPU difference: 15944.550207999004 / 15976.577213298899 [-0.20%]
products component :
Total MEMORY difference: 15470554 / 15472368 [-0.01%]
Total CPU difference: 6362.157582799963 / 6216.093846499964 [+2.35%]

There are no differences in images found by testdiff.
(0040990)
git (administrator)
2015-05-14 16:28

Branch CR25746 has been deleted by inv.

SHA-1: 970c85c0a719abd1dd2eebd1d57254be15b12671

- Related Changesets
occt: master d2d0f002
Timestamp: 2015-04-14 15:46:12
Author: dbv
Committer: bugmaster
Details ] Diff ]
0025746: Excessive memory use in math_Matrix

math_DoubleTab now statically allocates only 16 items for Buf
Removed indirection table from math_DoubleTab
mod - src/math/math_DoubleTab.cdl Diff ] File ]
mod - src/math/math_DoubleTab.cxx Diff ] File ]
mod - src/math/math_DoubleTab.lxx Diff ] File ]

- Issue History
Date Modified Username Field Change
2015-01-23 16:36 agv New Issue
2015-01-23 16:36 agv Assigned To => abv
2015-02-14 22:58 abv Note Added: 0037546
2015-02-14 22:58 abv Assigned To abv => agv
2015-02-14 22:58 abv Status new => feedback
2015-02-15 02:28 agv Note Added: 0037548
2015-03-04 11:27 agv Assigned To agv => msv
2015-03-04 11:27 agv Status feedback => assigned
2015-03-09 19:36 msv Relationship added related to 0024044
2015-03-09 19:39 msv Note Added: 0038215
2015-03-14 20:53 abv Relationship added related to 0025794
2015-04-09 18:06 msv Assigned To msv => dbv
2015-04-09 18:09 msv Note Added: 0039524
2015-04-09 20:01 Roman Lygin Note Added: 0039535
2015-04-14 18:46 git Note Added: 0039716
2015-04-14 18:57 git Note Added: 0039718
2015-04-14 19:03 dbv Note Added: 0039720
2015-04-14 19:03 dbv Assigned To dbv => msv
2015-04-14 19:03 dbv Status assigned => resolved
2015-04-14 19:03 dbv Steps to Reproduce Updated View Revisions
2015-04-14 19:04 dbv File Added: diff.html
2015-04-14 22:18 kgv Relationship added related to 0023419
2015-04-15 11:39 msv Note Added: 0039751
2015-04-15 11:39 msv Assigned To msv => dbv
2015-04-15 11:39 msv Status resolved => assigned
2015-04-15 11:39 msv Priority normal => high
2015-04-15 12:31 msv Note Added: 0039768
2015-04-15 12:51 git Note Added: 0039772
2015-04-15 13:01 dbv Note Added: 0039775
2015-04-15 13:01 dbv Assigned To dbv => msv
2015-04-15 13:01 dbv Status assigned => resolved
2015-04-15 13:18 msv Note Added: 0039776
2015-04-15 13:18 msv Assigned To msv => bugmaster
2015-04-15 13:18 msv Status resolved => reviewed
2015-04-15 14:09 mkv Assigned To bugmaster => mkv
2015-04-15 15:13 git Note Added: 0039785
2015-04-16 12:06 mkv Note Added: 0039833
2015-04-16 12:07 mkv Note Added: 0039834
2015-04-16 12:07 mkv Assigned To mkv => bugmaster
2015-04-16 12:07 mkv Status reviewed => tested
2015-04-16 12:08 mkv Test case number => Not needed
2015-04-17 15:40 bugmaster Changeset attached => occt master d2d0f002
2015-04-17 15:40 bugmaster Status tested => verified
2015-04-17 15:40 bugmaster Resolution open => fixed
2015-05-14 15:28 aiv Status verified => closed
2015-05-14 15:31 aiv Fixed in Version => 6.9.0
2015-05-14 16:28 git Note Added: 0040990


Copyright © 2000 - 2019 MantisBT Team
Powered by Mantis Bugtracker