View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0029719 | Community | OCCT:Modeling Algorithms | public | 2018-04-23 17:49 | 2019-11-25 09:53 |
| Reporter | drazmyslovich | Assigned To | Roman Lygin | ||
| Priority | normal | Severity | tweak | ||
| Status | closed | Resolution | fixed | ||
| Platform | Windows | OS | VC++ 2010 | ||
| Product Version | 7.1.0 | ||||
| Target Version | 7.4.0 | Fixed in Version | 7.4.0 | ||
| Summary | 0029719: Modeling Algorithms - GeomPlate_BuildPlateSurface has no progress information and is not abortable | ||||
| Description | For the complex cases GeomPlate_BuildPlateSurface takes a lot of time. At the same time it's not abortable and has no progress information. It makes the usage of GeomPlate_BuildPlateSurface in the end-user application is hardly possible - the user thinks, that the program hangs. | ||||
| Steps To Reproduce | N/A | ||||
| Tags | No tags attached. | ||||
| Test case number | Not needed | ||||
|
|
Branch CR29719 has been created by drazmyslovich. SHA-1: 0d4af647dffcfbd65aac91bc58b9c9b2132ab7da Detailed log of new commits: Author: drazmyslovich Date: Mon Apr 23 16:52:43 2018 +0200 0029719: Enhance GeomPlate_BuildPlateSurface with the progress information and the user break response |
|
|
Dear msv! We added the progress indicator as an optional parameter for GeomPlate_BuildPlateSurface and propagated to Plate_Plate class. Unfortunately, we needed to substitute the standard LU decomposition from the math package with a local algorithm, which supports the progress information. We were unsure, if the standard math package is allowed to use the end-user concept of Message_ProgressIndicator, therefore we decided to duplicate the function for the local usage. Thank you. |
|
|
Dear drazmyslovich, thank you for the contribution! Before passing the patch on the next stage, could you please consider the following remarks: src/Plate/Plate_Plate.hxx - replace "#include <Message_ProgressIndicator.hxx>" with declaration "class Message_ProgressIndicator;" src/GeomPlate/GeomPlate_BuildPlateSurface.cxx - Please instrument the other calls to SolveTI in lines 703, 1730. For the last, add progress indicator parameter to the method ComputeSurfInit(). - Please make the equal indentation of lines. For that, use "8 spaces per tab character" setting in your editor. src/Plate/Plate_Plate.cxx - The package math is located in the toolkit TKMath. The package Message is located in the toolkit TKernel. TKernel has the lowest level. So, it is OK to pass the instance of progress indicator to math classes. So, please avoid code duplication and add progress indicator in math where it is needed. - I wonder if it is also needed to use progress indicator in the methods math_Gauss::Solve() and Invert(). Don't they take much time? - The Gauss algo in the lines 561, 664, 930 is to be called with progress indicator as well. - When you return on user break you should set OK to false, otherwise IsDone() will return true. |
|
|
Branch CR29719 has been updated by drazmyslovich. SHA-1: 6033b43fad6638f90f6c9348534fe555a6485547 Detailed log of new commits: Author: drazmyslovich Date: Tue Apr 24 08:53:07 2018 +0200 0029719: Code review modifications for the progress information enhancements of GeomPlate_BuildPlateSurface |
|
|
Dear msv, I modified the code according to your recommendations. According to our observations the most critical part of math_Gauss is LU decomposition. All other algorithms are quite fast. Please, review the latest state of the branch. Thank you. |
|
|
More remarks: src/math/math_Recipes.cxx - Line 188: why max parameter is set to n+1. It should be the number of iterations, i.e. n. - Line 200: this is extra statement. src/math/math_Gauss.hxx - Line 60: Please wrap the line to make it shorter. src/Plate/Plate_Plate.hxx - Line 87: Please wrap the line to make it shorter. src/GeomPlate/GeomPlate_BuildPlateSurface.cxx - Lines 663-668, 698-726 have inconsistent indentation. Please form one commit in the new branch CR29719_1 using the following rule for commit message: 1. The first line must be equal to the summary field of the bug. 2. The second line must be empty. 3. The next lines present the description of the fix if it is needed. 4. The other commits (additional) may contain only lines commented with '#' character. Such lines will be ignored by bugmaster when combining to one commit. Thank you. |
|
|
Branch CR29719_1 has been created by drazmyslovich. SHA-1: ee968ef614da984086c5835ce0290f26261fe1b6 Detailed log of new commits: Author: drazmyslovich Date: Mon Apr 23 16:52:43 2018 +0200 0029719: Modeling Algorithms - GeomPlate_BuildPlateSurface has no progress information and is not abortable The Message_ProgressIndicator handle is added as a parameter to the function LU_Decompose and the the member functions of math_Gauss, Plate_Plate and GeomPlate_BuildPlateSurface classes. |
|
|
Please, check the new branch CR29719_1 |
|
|
Branch CR29719_1 has been updated by msv. SHA-1: e6c3e277cc198894f9e8f97a596041b51b461418 Detailed log of new commits: Author: msv Date: Tue Apr 24 15:06:08 2018 +0300 #correct indents |
|
|
I have made minor corrections of intentation, and put the patch onto certification testing. Jenkins job is CR29719-master-msv. |
|
|
Reviewed. |
|
|
Combination - OCCT branch : CR29719_1 SHA - e6c3e277cc198894f9e8f97a596041b51b461418 Products branch : master SHA - 4667f5aca4a456ae13db44bc7210e8cac1ab89f0 was compiled on Linux, MacOS and Windows platforms and tested in optimize mode. Number of compiler warnings: No new/fixed warnings Regressions/Differences/Improvements: No regressions/differences CPU differences: Debian70-64: OCCT Total CPU difference: 18234.80999999992 / 18243.73999999987 [-0.05%] Products Total CPU difference: 7509.7400000000525 / 7501.010000000052 [+0.12%] Windows-64-VC10: OCCT Total CPU difference: 18226.345634798417 / 18049.78370299853 [+0.98%] Products Total CPU difference: 8275.509847799878 / 8220.566295599881 [+0.67%] Image differences : No differences that require special attention Memory differences : No differences that require special attention |
|
|
Branch CR29719 has been deleted by kgv. SHA-1: 6033b43fad6638f90f6c9348534fe555a6485547 |
|
|
Branch CR29719_1 has been deleted by kgv. SHA-1: e6c3e277cc198894f9e8f97a596041b51b461418 |
|
|
Adding progress indicator into the lowest level math function should not have been accepted. This will have immediate strong performance penalties as soon as any intrusive progress indicator is used. Costs of virtual call and checking the status of some GUI widget will add significant overhead to simple loops over matrix rows/columns. The regression tests did not show performance drop apparently due to using some null/empty progress indicator in the test framework. Please roll back the modifications from the math kernel. All performance indicators-related stuff may only be used in higher level algorithms (GeomPlate in this case). Thank you. |
|
|
Roman, can you prove that this change actually leads to performance downgrade in some real-world situation (or at least confirm that you actually experience that in your appliaction)? Indeed we shall assume that progress indicator shall be implemented in reasonable way to avoid stopping in method Show() - otherwise any its usage will lead to performance issues. If the problem is real, please create separate issue for it. |
|
occt: master 9f785738 2018-04-23 14:52:43 Committer: bugmaster Details Diff |
0029719: Modeling Algorithms - GeomPlate_BuildPlateSurface has no progress information and is not abortable The Message_ProgressIndicator handle is added as a parameter to the function LU_Decompose and the the member functions of math_Gauss, Plate_Plate and GeomPlate_BuildPlateSurface classes. |
Affected Issues 0029719 |
|
| mod - src/GeomPlate/GeomPlate_BuildPlateSurface.cxx | Diff File | ||
| mod - src/GeomPlate/GeomPlate_BuildPlateSurface.hxx | Diff File | ||
| mod - src/math/math_Gauss.cxx | Diff File | ||
| mod - src/math/math_Gauss.hxx | Diff File | ||
| mod - src/math/math_Recipes.cxx | Diff File | ||
| mod - src/math/math_Recipes.hxx | Diff File | ||
| mod - src/Plate/Plate_Plate.cxx | Diff File | ||
| mod - src/Plate/Plate_Plate.hxx | Diff File | ||
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2018-04-23 17:49 | drazmyslovich | New Issue | |
| 2018-04-23 17:49 | drazmyslovich | Assigned To | => msv |
| 2018-04-23 17:52 | git | Note Added: 0075617 | |
| 2018-04-23 17:58 | drazmyslovich | Note Added: 0075618 | |
| 2018-04-23 17:58 | drazmyslovich | Status | new => resolved |
| 2018-04-23 19:44 |
|
Note Added: 0075622 | |
| 2018-04-23 19:44 |
|
Assigned To | msv => drazmyslovich |
| 2018-04-23 19:44 |
|
Status | resolved => assigned |
| 2018-04-24 09:53 | git | Note Added: 0075632 | |
| 2018-04-24 09:55 | drazmyslovich | Note Added: 0075633 | |
| 2018-04-24 09:55 | drazmyslovich | Assigned To | drazmyslovich => msv |
| 2018-04-24 09:55 | drazmyslovich | Status | assigned => resolved |
| 2018-04-24 09:57 | kgv | Summary | GeomPlate_BuildPlateSurface has no progress information and is not abortable => Modeling Algorithms - GeomPlate_BuildPlateSurface has no progress information and is not abortable |
| 2018-04-24 12:55 |
|
Note Added: 0075647 | |
| 2018-04-24 12:56 |
|
Assigned To | msv => drazmyslovich |
| 2018-04-24 12:56 |
|
Status | resolved => assigned |
| 2018-04-24 13:30 | git | Note Added: 0075648 | |
| 2018-04-24 13:31 | drazmyslovich | Note Added: 0075649 | |
| 2018-04-24 13:31 | drazmyslovich | Assigned To | drazmyslovich => msv |
| 2018-04-24 13:31 | drazmyslovich | Status | assigned => resolved |
| 2018-04-24 15:07 | git | Note Added: 0075652 | |
| 2018-04-24 15:12 |
|
Note Added: 0075653 | |
| 2018-04-25 11:54 |
|
Note Added: 0075674 | |
| 2018-04-25 11:54 |
|
Assigned To | msv => bugmaster |
| 2018-04-25 11:54 |
|
Status | resolved => reviewed |
| 2018-05-23 18:17 | bugmaster | Test case number | => Not needed |
| 2018-05-23 18:31 | bugmaster | Note Added: 0076197 | |
| 2018-05-23 18:31 | bugmaster | Status | reviewed => tested |
| 2018-06-14 18:20 | bugmaster | Changeset attached | => occt master 9f785738 |
| 2018-06-14 18:20 | bugmaster | Status | tested => verified |
| 2018-06-14 18:20 | bugmaster | Resolution | open => fixed |
| 2018-06-23 13:56 | git | Note Added: 0076933 | |
| 2018-06-23 13:56 | git | Note Added: 0076934 | |
| 2019-11-09 12:42 | Roman Lygin | Assigned To | bugmaster => msv |
| 2019-11-09 12:42 | Roman Lygin | Note Added: 0088822 | |
| 2019-11-09 12:42 | Roman Lygin | Status | closed => feedback |
| 2019-11-09 12:42 | Roman Lygin | Resolution | fixed => reopened |
| 2019-11-12 10:43 |
|
Assigned To | msv => Roman Lygin |
| 2019-11-12 10:45 |
|
Note Added: 0088901 | |
| 2019-11-13 10:01 |
|
Note Edited: 0088901 | |
| 2019-11-23 10:20 |
|
Status | feedback => closed |
| 2019-11-23 10:20 |
|
Resolution | reopened => fixed |
| 2019-11-25 09:53 |
|
Relationship added | parent of 0031190 |
