MantisBT
Mantis Bug Tracker Workflow

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0025605Open CASCADE[OCCT] OCCT:Modeling Algorithmspublic2014-12-12 17:542018-07-07 23:14
Reporternbv 
Assigned Tomsv 
PrioritynormalSeverityintegration request 
StatusfeedbackResolutionopen 
PlatformOSOS Version
Product Version 
Target Version[OCCT] UnscheduledFixed in Version 
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
Attached Files

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

-  Notes
(0035404)
msv (developer)
2014-12-15 12:26

Please proceed.
(0035912)
git (administrator)
2014-12-30 17:50

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
(0037686)
git (administrator)
2015-02-19 12:27

Branch CR25605 has been updated forcibly by nbv.

SHA-1: 8c806b1384130558d79716347bcbadec36e84f11
(0037982)
git (administrator)
2015-03-02 11:15

Branch CR25605 has been updated forcibly by nbv.

SHA-1: 35f07c033069e5476054eaea589a343af3ad9d32
(0037991)
kgv (developer)
2015-03-02 12:56

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).
(0038237)
git (administrator)
2015-03-11 09:41

Branch CR25605 has been updated forcibly by nbv.

SHA-1: 704deea9cc2aeb1b1964db89350c6ce49cd871d1
(0043594)
git (administrator)
2015-07-27 16:13

Branch CR25605 has been updated forcibly by nbv.

SHA-1: 16d2825afb063888bac01b83c01f7b84b5862a02
(0046319)
git (administrator)
2015-09-30 13:58

Branch CR25605 has been updated forcibly by nbv.

SHA-1: 38bf86abe833b312fc9b520d1469d2831d665ea2
(0046388)
git (administrator)
2015-10-02 09:49

Branch CR25605 has been updated forcibly by nbv.

SHA-1: f080e199394c7691ca504ee12a6c0eeb390e3c28
(0046391)
git (administrator)
2015-10-02 11:57

Branch CR25605 has been updated forcibly by nbv.

SHA-1: 32027616f82b08f7e76f89a0e60eec1363f0d196
(0046392)
nbv (developer)
2015-10-02 11:58

Dear Mikhail,

Please review CR25605 branch.
(0046398)
nbv (developer)
2015-10-02 15:23

Some parts of the fix should be registered as separated issues.
(0046423)
git (administrator)
2015-10-02 18:05

Branch CR25605 has been updated forcibly by nbv.

SHA-1: dbec9b3b82d643a0ac49c98c337d9bf2b6dc0dec
(0046424)
nbv (developer)
2015-10-02 18:07

Dear Mikhail,

Please review the current state of CR25605 branch.
(0046454)
git (administrator)
2015-10-05 13:49

Branch CR25605 has been updated forcibly by nbv.

SHA-1: 57cf9cdfa267bdad5eaf9edf1ec99323aa5be852
(0046455)
nbv (developer)
2015-10-05 13:51

Dear Mikhail,

Please review the current state of CR25605 branch.
(0047202)
msv (developer)
2015-10-19 17:54

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);
(0049411)
git (administrator)
2015-12-22 09:46

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.
(0049412)
nbv (developer)
2015-12-22 09:47

Dear Mikhail,

Please review the current state of CR25605_1 branch.
(0049423)
msv (developer)
2015-12-22 12:57

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)
(0049425)
git (administrator)
2015-12-22 13:05

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.

(0049439)
git (administrator)
2015-12-22 13:47

Branch CR25605_1 has been updated forcibly by nbv.

SHA-1: 2cd0c286ea5f78d77c9262f41682f6e126d3df55
(0049440)
nbv (developer)
2015-12-22 13:47

Dear Mikhail,

Please review the current state of CR25605_1 branch.
(0049491)
msv (developer)
2015-12-23 11:20

Consider changing gp_Vec2d::GetNormal to return counter-clock-wise oriented normal.
(0049599)
git (administrator)
2015-12-25 15:25

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.
(0049600)
nbv (developer)
2015-12-25 15:26

Dear Mikhail,

Please review CR25605_2 branch.
(0049620)
msv (developer)
2015-12-28 09:37

Reviewed.
(0049650)
git (administrator)
2015-12-28 17:26

Branch CR25605_2 has been updated forcibly by apv.

SHA-1: 5f07c9798a5c807fe6e4cf6708ed267cc51185ce
(0049651)
apv (tester)
2015-12-28 17:28

Branch CR25605_2 has been rebased on the current master
(0049662)
apv (tester)
2015-12-29 12:38

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%]
(0049663)
apv (tester)
2015-12-29 12:38

Dear nbv,

Branch CR25605_2 has been rejected due to:
- regressions/differences/improvements
(0049741)
abv (manager)
2016-01-08 21:01

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

- 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 View Revisions
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 View Revisions
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 View Revisions
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 Note Added: 0049610
2015-12-25 23:38 msv Assigned To msv => nbv
2015-12-25 23:38 msv Status resolved => assigned
2015-12-28 09:36 msv Note Deleted: 0049610
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


Copyright © 2000 - 2018 MantisBT Team
Powered by Mantis Bugtracker