View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0025605Open CASCADEOCCT:Modeling Algorithmspublic2014-12-12 17:542019-10-02 17:54
ReporternbvAssigned Tomsv 
PrioritynormalSeverityintegration request 
Status closedResolutionopen 
Summary0025605: Optimize methods of gp_* classes to avoid sqrt() function calling
DescriptionIn accordance with conclusion, made in bug 0025124, using Magnitude() method is not efficient (it is better to use SquareMagnitude() method).
Steps To ReproduceNot required.
TagsNo tags attached.
Test case numberNot needed

Relationships

related to 0026746 closedbugmaster Method gp_Torus::Coefficients(...) returns incorrect value. 
related to 0026747 closedbugmaster Some constructors of gp_Parab2d class contain redundant parameters 
related to 0026750 closedbugmaster Method IsNormal(...) for gp_Vec2d returns FALSE if the angle between two vectors is equal to -90 degree (-M_PI/2 radian) 

Activities

msv

2014-12-15 12:26

developer   ~0035404

Please proceed.

git

2014-12-30 17:50

administrator   ~0035912

Branch CR25605 has been created by nbv.

SHA-1: 7e809659d9e2bb0d0ae1b71d2465262f16978b14


Detailed log of new commits:

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

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

git

2015-02-19 12:27

administrator   ~0037686

Branch CR25605 has been updated forcibly by nbv.

SHA-1: 8c806b1384130558d79716347bcbadec36e84f11

git

2015-03-02 11:15

administrator   ~0037982

Branch CR25605 has been updated forcibly by nbv.

SHA-1: 35f07c033069e5476054eaea589a343af3ad9d32

kgv

2015-03-02 12:56

developer   ~0037991

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).

git

2015-03-11 09:41

administrator   ~0038237

Branch CR25605 has been updated forcibly by nbv.

SHA-1: 704deea9cc2aeb1b1964db89350c6ce49cd871d1

git

2015-07-27 16:13

administrator   ~0043594

Branch CR25605 has been updated forcibly by nbv.

SHA-1: 16d2825afb063888bac01b83c01f7b84b5862a02

git

2015-09-30 13:58

administrator   ~0046319

Branch CR25605 has been updated forcibly by nbv.

SHA-1: 38bf86abe833b312fc9b520d1469d2831d665ea2

git

2015-10-02 09:49

administrator   ~0046388

Branch CR25605 has been updated forcibly by nbv.

SHA-1: f080e199394c7691ca504ee12a6c0eeb390e3c28

git

2015-10-02 11:57

administrator   ~0046391

Branch CR25605 has been updated forcibly by nbv.

SHA-1: 32027616f82b08f7e76f89a0e60eec1363f0d196

nbv

2015-10-02 11:58

developer   ~0046392

Dear Mikhail,

Please review CR25605 branch.

nbv

2015-10-02 15:23

developer   ~0046398

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

git

2015-10-02 18:05

administrator   ~0046423

Branch CR25605 has been updated forcibly by nbv.

SHA-1: dbec9b3b82d643a0ac49c98c337d9bf2b6dc0dec

nbv

2015-10-02 18:07

developer   ~0046424

Dear Mikhail,

Please review the current state of CR25605 branch.

git

2015-10-05 13:49

administrator   ~0046454

Branch CR25605 has been updated forcibly by nbv.

SHA-1: 57cf9cdfa267bdad5eaf9edf1ec99323aa5be852

nbv

2015-10-05 13:51

developer   ~0046455

Dear Mikhail,

Please review the current state of CR25605 branch.

msv

2015-10-19 17:54

developer   ~0047202

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);

git

2015-12-22 09:46

administrator   ~0049411

Branch CR25605_1 has been created by nbv.

SHA-1: 3e5e3a48197f3964f272c2b7437b220ccb9d6441


Detailed log of new commits:

Author: nbv
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.

nbv

2015-12-22 09:47

developer   ~0049412

Dear Mikhail,

Please review the current state of CR25605_1 branch.

msv

2015-12-22 12:57

developer   ~0049423

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)

git

2015-12-22 13:05

administrator   ~0049425

Branch CR25605_1 has been updated by nbv.

SHA-1: fc4640287afa25065ee65456641462a75324cddd


Detailed log of new commits:

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

    Changes in accordance with the last remark.

git

2015-12-22 13:47

administrator   ~0049439

Branch CR25605_1 has been updated forcibly by nbv.

SHA-1: 2cd0c286ea5f78d77c9262f41682f6e126d3df55

nbv

2015-12-22 13:47

developer   ~0049440

Dear Mikhail,

Please review the current state of CR25605_1 branch.

msv

2015-12-23 11:20

developer   ~0049491

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

git

2015-12-25 15:25

administrator   ~0049599

Branch CR25605_2 has been created by nbv.

SHA-1: 61e2f8dd1167b70531f95f786a22f2a2d0f83186


Detailed log of new commits:

Author: nbv
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.

nbv

2015-12-25 15:26

developer   ~0049600

Dear Mikhail,

Please review CR25605_2 branch.

msv

2015-12-28 09:37

developer   ~0049620

Reviewed.

git

2015-12-28 17:26

administrator   ~0049650

Branch CR25605_2 has been updated forcibly by apv.

SHA-1: 5f07c9798a5c807fe6e4cf6708ed267cc51185ce

apv

2015-12-28 17:28

tester   ~0049651

Branch CR25605_2 has been rebased on the current master

apv

2015-12-29 12:38

tester   ~0049662

Dear BugMaster,

Branch CR25605_2 from occt git-repository (and master from products git-repository) was compiled on Linux, MacOS and Windows platforms and tested.
SHA-1: 5f07c9798a5c807fe6e4cf6708ed267cc51185ce

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%]

apv

2015-12-29 12:38

tester   ~0049663

Dear nbv,

Branch CR25605_2 has been rejected due to:
- regressions/differences/improvements

abv

2016-01-08 21:01

manager   ~0049741

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...

git

2019-10-02 17:54

administrator   ~0087710

Branch CR25605 has been deleted by kgv.

SHA-1: 57cf9cdfa267bdad5eaf9edf1ec99323aa5be852

git

2019-10-02 17:54

administrator   ~0087711

Branch CR25605_1 has been deleted by kgv.

SHA-1: 2cd0c286ea5f78d77c9262f41682f6e126d3df55

git

2019-10-02 17:54

administrator   ~0087712

Branch CR25605_2 has been deleted by kgv.

SHA-1: 5f07c9798a5c807fe6e4cf6708ed267cc51185ce

Issue History

Date Modified Username Field Change
2014-12-12 17:54 nbv New Issue
2014-12-12 17:54 nbv Assigned To => msv
2014-12-15 12:12 nbv Description Updated
2014-12-15 12:17 nbv Summary It is necessary to optimize methods of gp_Vec and gp_Vec2d classes => It is necessary to optimize methods of gp_* classes to avoid sqrt() function calling
2014-12-15 12:26 msv Note Added: 0035404
2014-12-15 12:26 msv Assigned To msv => nbv
2014-12-15 12:26 msv Status new => assigned
2014-12-30 17:50 git Note Added: 0035912
2015-02-19 12:27 git Note Added: 0037686
2015-03-02 11:15 git Note Added: 0037982
2015-03-02 12:56 kgv Note Added: 0037991
2015-03-11 09:41 git Note Added: 0038237
2015-04-14 10:01 msv Severity minor => integration request
2015-04-14 10:01 msv Target Version 6.9.0 => 7.1.0
2015-04-14 10:01 msv Summary It is necessary to optimize methods of gp_* classes to avoid sqrt() function calling => Optimize methods of gp_* classes to avoid sqrt() function calling
2015-07-27 16:13 git Note Added: 0043594
2015-09-30 13:58 git Note Added: 0046319
2015-10-02 09:49 git Note Added: 0046388
2015-10-02 11:57 git Note Added: 0046391
2015-10-02 11:58 nbv Note Added: 0046392
2015-10-02 11:58 nbv Assigned To nbv => msv
2015-10-02 11:58 nbv Status assigned => resolved
2015-10-02 11:58 nbv Steps to Reproduce Updated
2015-10-02 15:21 nbv Assigned To msv => nbv
2015-10-02 15:21 nbv Status resolved => assigned
2015-10-02 15:23 nbv Note Added: 0046398
2015-10-02 17:43 nbv Relationship added related to 0026746
2015-10-02 17:43 nbv Relationship added related to 0026747
2015-10-02 18:05 git Note Added: 0046423
2015-10-02 18:07 nbv Note Added: 0046424
2015-10-02 18:07 nbv Assigned To nbv => msv
2015-10-02 18:07 nbv Status assigned => resolved
2015-10-02 18:07 nbv Steps to Reproduce Updated
2015-10-05 09:28 nbv Assigned To msv => nbv
2015-10-05 09:28 nbv Status resolved => assigned
2015-10-05 11:45 nbv Relationship added related to 0026750
2015-10-05 13:49 git Note Added: 0046454
2015-10-05 13:51 nbv Note Added: 0046455
2015-10-05 13:51 nbv Assigned To nbv => msv
2015-10-05 13:51 nbv Status assigned => resolved
2015-10-19 17:54 msv Note Added: 0047202
2015-10-19 17:54 msv Assigned To msv => nbv
2015-10-19 17:54 msv Status resolved => assigned
2015-12-22 09:46 git Note Added: 0049411
2015-12-22 09:47 nbv Note Added: 0049412
2015-12-22 09:47 nbv Assigned To nbv => msv
2015-12-22 09:47 nbv Status assigned => resolved
2015-12-22 12:57 msv Note Added: 0049423
2015-12-22 12:57 msv Assigned To msv => nbv
2015-12-22 12:57 msv Status resolved => assigned
2015-12-22 13:05 git Note Added: 0049425
2015-12-22 13:47 git Note Added: 0049439
2015-12-22 13:47 nbv Note Added: 0049440
2015-12-22 13:47 nbv Assigned To nbv => msv
2015-12-22 13:47 nbv Status assigned => resolved
2015-12-23 11:20 msv Note Added: 0049491
2015-12-23 11:20 msv Assigned To msv => nbv
2015-12-23 11:20 msv Status resolved => assigned
2015-12-25 15:25 git Note Added: 0049599
2015-12-25 15:26 nbv Note Added: 0049600
2015-12-25 15:26 nbv Assigned To nbv => msv
2015-12-25 15:26 nbv Status assigned => resolved
2015-12-25 23:38 msv Assigned To msv => nbv
2015-12-25 23:38 msv Status resolved => assigned
2015-12-28 09:37 msv Assigned To nbv => msv
2015-12-28 09:37 msv Status assigned => resolved
2015-12-28 09:37 msv Note Added: 0049620
2015-12-28 09:37 msv Assigned To msv => bugmaster
2015-12-28 09:37 msv Status resolved => reviewed
2015-12-28 17:26 git Note Added: 0049650
2015-12-28 17:28 apv Note Added: 0049651
2015-12-28 17:28 apv Test case number => Not needed
2015-12-28 17:28 apv Assigned To bugmaster => apv
2015-12-29 12:38 apv Note Added: 0049662
2015-12-29 12:38 apv Assigned To apv => nbv
2015-12-29 12:38 apv Status reviewed => assigned
2015-12-29 12:38 apv Note Added: 0049663
2016-01-08 21:01 abv Note Added: 0049741
2016-01-08 21:02 abv Target Version 7.1.0 => Unscheduled
2018-07-07 23:14 abv Assigned To nbv => msv
2018-07-07 23:14 abv Status assigned => feedback
2019-09-02 18:41 abv Status feedback => closed
2019-09-02 18:41 abv Target Version Unscheduled =>
2019-10-02 17:54 git Note Added: 0087710
2019-10-02 17:54 git Note Added: 0087711
2019-10-02 17:54 git Note Added: 0087712