Optimize methods of gp_* classes to avoid sqrt() function calling

Please proceed.

Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Tue Dec 30 17:45:53 2014 +0300

    0025605: It is necessary to optimize methods of gp_* classes to avoid sqrt() function calling

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Dear nbv,

> 0025605: It is necessary to optimize methods of gp_* classes to avoid sqrt() function calling
please consider re-formulating the bug name and description.
From current context it is unclear why this modification is actually necessary - significant performance issues of some algorithm?. Otherwise it would be better to omit redundant words in description and change Severity of the bug appropriately ("feature" or "integration request" in case of unknown impact).

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Dear Mikhail,

Please review CR25605 branch.

Some parts of the fix should be registered as separated issues.

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Dear Mikhail,

Please review the current state of CR25605 branch.

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Dear Mikhail,

Please review the current state of CR25605 branch.

src\gp\gp_Circ.lxx:

1) It's better to declare reference to avoid copying coordinates in line 71.
2) The sign '+' must be changed to '-' in line 73.
3) Why do not you call Abs here (as you did in other places):
75: if(aDist < 0.0)
76: aDist = -aDist;

src\gp\gp_Circ2d.lxx:

4) In computation of gp_Circ2d::SquareDistance there is extra calculation of absolute value now, which is done in Distance() method.

src\gp\gp_Dir.cxx:

5) In gp_Dir::Angle, you did not changed any logic in the method, so, please return its look to original one.
6) Comment in lines 117-118, please correct as follows:
    //After transformation, earlier normalized vector "coord" might become
    //not normalized. Therefore, we must normalize it again.

src\gp\gp_Dir.hxx:

7) gp_Dir::AngleWithRef: the new implementation requires one more calculation of cross product, and extra calculation of Abs (via call of Sign). Please leave the old one, but move calculation of Sinus nearer to the place of its usage.
8) Why the new method IsCoincide is added? I see no reason, so I propose to not add it.
9) No need to modify the description of IsParallel method.

src\gp\gp_Dir.lxx:

10) The changes in the methods IsEqual, IsOpposite, IsParallel are useless, especially taking of Abs of the positive number. So, please return them back.

src\gp\gp_Dir2d.cxx:
11) In gp_Dir2d::Angle, usage of Sign function is not justified, as it involves calculation of Abs, while the first argument to Sign is never negative in these cases. Summarizing, you did not changed any logic in the method, so, please return its look to original one.
12) The same as 6)

src\gp\gp_Dir2d.hxx:
13) Comment in the line 180 does not make description more clear, the old one is more precise.
14) the same as 8)
15) the same as 9), plus No need to modify the description of IsOpposite method.

src\gp\gp_Dir2d.lxx:
16) In line 141, use more natural expression: M_PI - Ang <= AngularTolerance.
17) In line 149, use more natural expression: Ang <= AngularTolerance || M_PI - Ang <= AngularTolerance.
18) Why you changed code to use Abs only in two places, while code like "if (Ang < 0) Ang = - Ang" is used more in this file? May be then it is not needed to make changes at all?

src\gp\gp_Lin.hxx:
19) Line 125: "there is an infinity solutions in 3D space": the preposition 'of' is missed between words 'infinity' and 'solutions'.

src\gp\gp_Mat2d.lxx:
20) In gp_Mat2d::Divide and Divided, 'val' is not used in Release mode, so please hide its computation in the argument of the macro Standard_ConstructionError_Raise_if.

src\gp\gp_Pln.hxx:
21) What is the reason of creation of the following constructor?
gp_Pln(const gp_Pnt& theP1, const gp_Pnt& theP2, const gp_Pnt& theP3);
It is not needed here, because this method of a plane creation ia available in the class gce_MakePln. According to user's guide, the package gce is intended for such kind of direct constructions:
"Direct Construction methods from gce, GC and GCE2d packages provide simplified algorithms to build elementary geometric entities such as lines, circles and curves. They complement the reference definitions provided by the gp, Geom and Geom2d packages."
22) So, it is not needed to create a new protected method Init.

src\gp\gp_Pnt.hxx:
23) Line 107: "Computes a Bary-center of the system two points": the preposition 'of' is missed between the words 'system' and two'.

src\gp\gp_Trsf.hxx:
24) Change in the line 54 (in comments) is not logically correct.

src\gp\gp_Trsf2d.hxx:
25) The same as 24)

src\gp\gp_Vec.hxx:
26) the same as 8)
27) the same as 15)
28) No need to change this file at all. Please revert.

src\gp\gp_Vec.lxx:
29) AngleWithRef method: fix makes regression in the sense that if theVref is null then it will return 0 instead of raising exception in debug mode. I propose to leave the old implementation. BTW, why did you move this method to another location?
30) The method IsOpposite: unjustified change, please revert.
31) The method IsParallel: unjustified change, please revert.
32) Unjustified change in the method Angle. It is better to have one source of truth. Here it is in the method gp_Dir::Angle.

src\gp\gp_Vec2d.cxx:
33) Lines 90, 94: usage of Sign is not justified here, as its first argument is never negative.

src\gp\gp_Vec2d.hxx:
34) the same as 8)
35) the same as 15)

src\gp\gp_Vec2d.lxx:
36) In line 88, use more natural expression: M_PI - Ang <= AngularTolerance.
37) In line 96, use more natural expression: Ang <= AngularTolerance || M_PI - Ang <= AngularTolerance.

src\gp\gp_XY.lxx:
38) Line 91: it's better to write 'Abs(Crossed(Right))'.
39) Line 95: it's better to write 'Zresult = Crossed(Right);

Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Mon Dec 21 18:25:39 2015 +0300

    0025605: Optimize methods of gp_* classes to avoid sqrt() function calling
    
    Some methods have been optimized and commented.

Dear Mikhail,

Please review the current state of CR25605_1 branch.

Remarks:

src\gp\gp_Dir.hxx

1) Please remove Standard_EXPORT from declaration of AngleWithRef in gp_Dir.hxx.

src\gp\gp_Dir2d.lxx

2) lines 149-153: please make one logical expression and one return statement. If you want to insert a comment you can do it without adding extra compilable constructions.

src\gp\gp_Lin.lxx

3) Line 69: use gp_XYZ::CrossSquareMagnitude. For the most effectiveness, use gp_XYZ instead of gp_Vec for temporary object aCoord.
4) Line 82: it's incorrect to assign gp_Dir to reference of gp_Vec. It's better to use gp_XYZ, and pos.Direction().XYZ(), and consequently all vector arithmetic with gp_XYZ instead of gp_Vec.

src\gp\gp_Lin2d.lxx

5) Line 72: It's better to use gp_XY to avoid construction of temporary gp_Vec2d in the line 73.
6) Line 74: replace with Abs, as you make this in other places.

src\gp\gp_Vec.cxx
7) Restore the logic of the method IsEqual, when (aMyCond XOR aOCond).

src\gp\gp_Vec2d.cxx
8) the same as 7)

src\gp\gp_Vec2d.lxx
9) Lines 93-97: same as 2)

Branch [archived branch] has been updated by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Tue Dec 22 13:04:19 2015 +0300

    Changes in accordance with the last remark.

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Dear Mikhail,

Please review the current state of CR25605_1 branch.

Consider changing gp_Vec2d::GetNormal to return counter-clock-wise oriented normal.

Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Fri Dec 25 15:24:20 2015 +0300

    0025605: Optimize methods of gp_* classes to avoid sqrt() function calling
    
    Some methods have been optimized and commented.
    
    The direction of the vector returned by gp_Vec2d::GetNormal() method has been changed. This value is brought to conformity with gp_Lin2d classes.

Dear Mikhail,

Please review CR25605_2 branch.

Reviewed.

Branch [archived branch] has been updated forcibly by Participant.

[revision removed]

Branch [archived branch] has been rebased on the current master

Dear Commenter 1,

Branch CR25605_2 from occt git-repository (and master from products git-repository) was compiled on Linux, MacOS and Windows platforms and tested.
[revision removed]

Number of compiler warnings:
occt component:
   Linux: 0 (0 on master)
   Windows: 0 (0 on master)
   MasOS: 134 (134 on master)
products component:
   Linux: 37 (37 on master)
   Windows: 0 (0 on master)

Regressions/Differences:
http://occt-tests/CR25605-2-master-occt-64/Debian70-64/summary.html
blend bfuseblend B5
http://occt-tests/CR25605-2-master-products-64/Debian70-64/summary.html
http://occt-tests/CR25605-2-master-products-64/Windows-64-VC10/summary.html
cr standard A2, A3, B1, C4, C7, D4
parasolid doc_3 C3 (Linux only)

Testing on Linux:
Total MEMORY difference: 89962052 / 89881494 [+0.09%]
Total CPU difference: 19203.24000000012 / 19263.68000000009 [-0.31%]

Testing on Windows:
Total MEMORY difference: 57258311 / 57526605 [-0.47%]
Total CPU difference: 18134.929048798895 / 18866.152536099093 [-3.88%]

Dear nbv,

Branch [archived branch] has been rejected due to:
- regressions/differences/improvements

I believe this activity should be stopped as it apparemtly has no sense.

There were no reply on question of Kirill, thus let me repeat: have you measured effect of these changes on performance? Can you prove that it improves performance and not makes it worse?

Besides, the change looks quite suspicious also from functional point of view. For instance, gp_Circ::SquareDistance() seems to calculate now quite different quantity... It was the only file I looked at...

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

[revision removed]

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

[revision removed]

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

[revision removed]