Modeling Algorithms - GeomPlate_BuildPlateSurface has no progress information and is not abortable

Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

Author: Author
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 Author, 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 [archived branch] has been updated by Author.

[revision removed]


Detailed log of new commits:

Author: Author
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 Commenter 2 when combining to one commit.

Thank you.

Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

Author: Author
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 [archived branch] has been updated by Participant.

[revision removed]


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 : [archived branch] SHA - [revision removed]
Products branch : [archived branch] SHA - [revision removed]
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 [archived branch] has been deleted by Commenter 4.

[revision removed]

Branch [archived branch] has been deleted by Commenter 4.

[revision removed]

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.