View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0025746 | Open CASCADE | OCCT:Foundation Classes | public | 2015-01-23 16:36 | 2015-05-14 16:28 |
| Reporter | Assigned To | bugmaster | |||
| Priority | high | Severity | minor | ||
| Status | closed | Resolution | fixed | ||
| Target Version | 6.9.0 | Fixed in Version | 6.9.0 | ||
| Summary | 0025746: Excessive memory use in math_Matrix | ||||
| Description | Class 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 Reproduce | Not required | ||||
| Tags | No tags attached. | ||||
| Test case number | Not needed | ||||
|
|
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? |
|
|
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. |
|
|
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. |
|
|
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. |
|
|
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 |
|
|
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 |
|
|
Branch CR25746 has been updated forcibly by dbv. SHA-1: 88120f68b73cb7c12b38e52cf07ce73ec9e234ef |
|
|
(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 |
|
2015-04-14 19:04 developer |
diff.html (31,825 bytes) |
|
|
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. |
|
|
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. |
|
|
Branch CR25746 has been updated forcibly by dbv. SHA-1: 22165fce5743a6db8211130dc2e6d1fd1349d24d |
|
|
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 |
|
|
Reviewed. |
|
|
Branch CR25746 has been updated forcibly by mkv. SHA-1: 970c85c0a719abd1dd2eebd1d57254be15b12671 |
|
|
Dear BugMaster, Branch CR25746 was rebased on current master of occt git-repository. |
|
|
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. |
|
|
Branch CR25746 has been deleted by inv. SHA-1: 970c85c0a719abd1dd2eebd1d57254be15b12671 |
|
occt: master d2d0f002 2015-04-14 15:46:12
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 |
Affected Issues 0025746 |
|
| mod - src/math/math_DoubleTab.cdl | Diff File | ||
| mod - src/math/math_DoubleTab.cxx | Diff File | ||
| mod - src/math/math_DoubleTab.lxx | Diff File | ||
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2015-01-23 16:36 |
|
New Issue | |
| 2015-01-23 16:36 |
|
Assigned To | => abv |
| 2015-02-14 22:58 |
|
Note Added: 0037546 | |
| 2015-02-14 22:58 |
|
Assigned To | abv => agv |
| 2015-02-14 22:58 |
|
Status | new => feedback |
| 2015-02-15 02:28 |
|
Note Added: 0037548 | |
| 2015-03-04 11:27 |
|
Assigned To | agv => msv |
| 2015-03-04 11:27 |
|
Status | feedback => assigned |
| 2015-03-09 19:36 |
|
Relationship added | related to 0024044 |
| 2015-03-09 19:39 |
|
Note Added: 0038215 | |
| 2015-03-14 20:53 |
|
Relationship added | related to 0025794 |
| 2015-04-09 18:06 |
|
Assigned To | msv => dbv |
| 2015-04-09 18:09 |
|
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 |
|
Note Added: 0039720 | |
| 2015-04-14 19:03 |
|
Assigned To | dbv => msv |
| 2015-04-14 19:03 |
|
Status | assigned => resolved |
| 2015-04-14 19:03 |
|
Steps to Reproduce Updated | |
| 2015-04-14 19:04 |
|
File Added: diff.html | |
| 2015-04-15 11:39 |
|
Note Added: 0039751 | |
| 2015-04-15 11:39 |
|
Assigned To | msv => dbv |
| 2015-04-15 11:39 |
|
Status | resolved => assigned |
| 2015-04-15 11:39 |
|
Priority | normal => high |
| 2015-04-15 12:31 |
|
Note Added: 0039768 | |
| 2015-04-15 12:51 | git | Note Added: 0039772 | |
| 2015-04-15 13:01 |
|
Note Added: 0039775 | |
| 2015-04-15 13:01 |
|
Assigned To | dbv => msv |
| 2015-04-15 13:01 |
|
Status | assigned => resolved |
| 2015-04-15 13:18 |
|
Note Added: 0039776 | |
| 2015-04-15 13:18 |
|
Assigned To | msv => bugmaster |
| 2015-04-15 13:18 |
|
Status | resolved => reviewed |
| 2015-04-15 14:09 |
|
Assigned To | bugmaster => mkv |
| 2015-04-15 15:13 | git | Note Added: 0039785 | |
| 2015-04-16 12:06 |
|
Note Added: 0039833 | |
| 2015-04-16 12:07 |
|
Note Added: 0039834 | |
| 2015-04-16 12:07 |
|
Assigned To | mkv => bugmaster |
| 2015-04-16 12:07 |
|
Status | reviewed => tested |
| 2015-04-16 12:08 |
|
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 |
|
Status | verified => closed |
| 2015-05-14 15:31 |
|
Fixed in Version | => 6.9.0 |
| 2015-05-14 16:28 | git | Note Added: 0040990 |
