View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0028636Open CASCADEOCCT:Codingpublic2017-04-06 11:192017-09-29 16:25
ReporternbvAssigned Tobugmaster  
PrioritynormalSeveritytweak 
Status closedResolutionfixed 
Product Version7.1.0 
Target Version7.2.0Fixed in Version7.2.0 
Summary0028636: Optimisation of gp_* classes in order to avoid unnecesary calling gp_Dir* constructors with normalisation
DescriptionMany constructors of gp_* classes contains fields as

gp_Dir v


and v is initialized by the value (1, 0). While such initialization, copy-constructor is used, which computes expression such as 

sqrt(1*1+0*0)


and spent much time.

The main idea of optimization is to avoid using constructors with normalization for trivial cases.

P.S.

This problem has been observed while fixing performance degradation in the issue #28211. Though this was very little hot spot (about 1-3%) it was decided to correct such obvious issue.
Steps To ReproduceNew test case is not required.
TagsNo tags attached.
Test case numberNot needed

Activities

git

2017-04-06 15:28

administrator   ~0064981

Branch CR28636 has been created by msv.

SHA-1: 75da7906240cfc6663a455a38912cb4885cc6898


Detailed log of new commits:

Author: msv
Date: Thu Apr 6 15:28:26 2017 +0300

    0028636: Optimisation of gp_* classes in order to avoid unnecesary calling gp_Dir* constructors with normalisation
    
    The improvement is concluded in replacement of calls to gp_Dir2d(1,0) with calls to gp_Dir2d(void), and calls to gp_Dir2d(0,1) to gp_Dir2d(gp::DY2d()). Thus we avoid sqrt() that is called from within gp_Dir2d(double,double). The same is with direction in 3D space.

msv

2017-04-06 15:29

developer   ~0064982

Dear Eugeny, please review the fix.

abv

2017-04-06 15:42

manager   ~0064984

Mikhail, please provide results of your performance measurements showing the positive effect of the change.

emv

2017-04-06 15:49

developer   ~0064988

Reviewed.

msv

2017-04-06 16:15

developer   ~0064994

Test case is to be created.

git

2017-04-07 19:44

administrator   ~0065056

Branch CR28636_1 has been created by msv.

SHA-1: 7c022c420d2bb05933c258b00b07fcfefde77b36


Detailed log of new commits:

Author: msv
Date: Thu Apr 6 15:28:26 2017 +0300

    0028636: Optimisation of gp_* classes in order to avoid unnecesary calling gp_Dir* constructors with normalisation
    
    The improvement is concluded in replacement of calls to gp_Dir2d(1,0) with calls to gp_Dir2d(void). Thus we avoid sqrt() that is called from within gp_Dir2d(double,double). The same is with direction in 3D space.
    
    Test cases have been created to check the improvement.

git

2017-04-07 20:01

administrator   ~0065057

Branch CR28636_1 has been updated forcibly by msv.

SHA-1: e57e1e343ba81e3975fb089716ebc61016988d89

msv

2017-04-07 20:08

developer   ~0065058

Last edited: 2017-04-10 10:13

 I have created synthetic test case to call default constructor in a loop. The code is the following:

  Standard_Integer nbRep = 1000000;
  Standard_Real dummy = 0.;
  for (; nbRep > 0; nbRep--)
  {
    for (Standard_Integer n = 100; n > 0; n--)
    {
      gp_Ax2d anAx1, anAx2;
      anAx2.Mirror(gp_Pnt2d(1., 1.));
      dummy += anAx1.Direction().XY() * anAx2.Direction().XY();
    }
  }


The following is comparison of CPU time spent in the fix vs master:

0.46875/1.203125

Dear Kirill, please review.

kgv

2017-04-07 20:25

developer   ~0065059

Last edited: 2017-04-09 21:35

 
-inline gp_Ax3::gp_Ax3() : vydir(0.,1.,0.), vxdir(1.,0.,0.)
+inline gp_Ax3::gp_Ax3()
+  :
+  vydir(0.,1.,0.)
+  // vxdir(1.,0.,0.) use default ctor of gp_Dir, as it creates the same dir(1,0,0)


Both vydir and vxdir are gp_Dir, but patch modifies initialization only of vxdir - why?
Does initialization of vydir(0.,1.,0.) somehow avoid normalization while vxdir(1.,0.,0.) does not?

Patch looks incomplete if it is intended to improve performance.
There might be many other options for making gp_Dir/gp_Dir2d constructors for standard directions without normalization:
- Extra bool flag to prevent normalization.
  Can be useful also in context when normalization is performed in advance (especially taking into account gp_Dir throws exception on division by zero).
- Constructor with constexpr looks like a good example what constexpr is designed for.
- Constructor taking enum with standard directions.
- Template constructor/method as alternative to constexpr.

git

2017-04-10 10:18

administrator   ~0065071

Branch CR28636_2 has been created by msv.

SHA-1: 9c1a3136a17e3647253a6ee625ed2279428c7031


Detailed log of new commits:

Author: msv
Date: Thu Apr 6 15:28:26 2017 +0300

    0028636: Optimisation of gp_* classes in order to avoid unnecesary calling gp_Dir* constructors with normalisation
    
    The improvement is concluded in replacement of calls to gp_Dir2d(1,0) with calls to gp_Dir2d(void). Thus we avoid sqrt() that is called from within gp_Dir2d(double,double). The same is with direction in 3D space.

msv

2017-04-10 10:20

developer   ~0065073

Please review again.

kgv

2017-04-10 10:23

developer   ~0065074

Please test the patch.

apv

2017-04-11 12:53

tester   ~0065127

Dear BugMaster,

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

Number of compiler warnings:
occt component:
   Linux: 0 (0 on master)
   Windows: 0 (0 on master)
   MasOS: 0 (0 on master)
products component:
   Linux: 64
   Windows: 0
   MacOS: 1198

Regressions/Differences:
Not detected

Testing cases:
Not needed

Testing on Linux:
Total MEMORY difference: 91799412 / 91395470 [+0.44%]
Total CPU difference: 19636.610000000222 / 19613.34000000024 [+0.12%]

Testing on Windows:
Total MEMORY difference: 57855660 / 57855806 [-0.00%]
Total CPU difference: 18299.525703898533 / 18063.40259029849 [+1.31%]

git

2017-05-12 11:36

administrator   ~0065956

Branch CR28636 has been deleted by kgv.

SHA-1: 75da7906240cfc6663a455a38912cb4885cc6898

git

2017-05-12 11:36

administrator   ~0065957

Branch CR28636_1 has been deleted by kgv.

SHA-1: e57e1e343ba81e3975fb089716ebc61016988d89

git

2017-05-12 11:36

administrator   ~0065958

Branch CR28636_2 has been deleted by kgv.

SHA-1: 9c1a3136a17e3647253a6ee625ed2279428c7031

Related Changesets

occt: master 0bd575a7

2017-04-06 12:28:26

msv


Committer: bugmaster Details Diff		 0028636: Optimisation of gp_* classes in order to avoid unnecesary calling gp_Dir* constructors with normalisation

The improvement is concluded in replacement of calls to gp_Dir2d(1,0) with calls to gp_Dir2d(void).
Thus we avoid sqrt() that is called from within gp_Dir2d(double,double). The same is with direction in 3D space.		 Affected Issues
0028636
mod - src/gp/gp_Ax2.lxx Diff File
mod - src/gp/gp_Ax22d.lxx Diff File
mod - src/gp/gp_Ax2d.lxx Diff File
mod - src/gp/gp_Ax3.lxx Diff File

Issue History

Date Modified Username Field Change
2017-04-06 11:19 nbv New Issue
2017-04-06 11:19 nbv Assigned To => msv
2017-04-06 11:20 nbv Description Updated
2017-04-06 12:32 msv Status new => assigned
2017-04-06 12:32 msv Product Version => 7.1.0
2017-04-06 15:28 git Note Added: 0064981
2017-04-06 15:29 msv Note Added: 0064982
2017-04-06 15:29 msv Assigned To msv => emv
2017-04-06 15:29 msv Status assigned => resolved
2017-04-06 15:42 abv Note Added: 0064984
2017-04-06 15:49 emv Note Added: 0064988
2017-04-06 15:49 emv Assigned To emv => bugmaster
2017-04-06 15:49 emv Status resolved => reviewed
2017-04-06 16:14 mkv Assigned To bugmaster => apv
2017-04-06 16:15 msv Note Added: 0064994
2017-04-06 16:15 msv Assigned To apv => msv
2017-04-06 16:15 msv Status reviewed => assigned
2017-04-07 19:44 git Note Added: 0065056
2017-04-07 20:01 git Note Added: 0065057
2017-04-07 20:08 msv Note Added: 0065058
2017-04-07 20:08 msv Assigned To msv => kgv
2017-04-07 20:08 msv Status assigned => resolved
2017-04-07 20:25 kgv Note Added: 0065059
2017-04-07 20:25 kgv Assigned To kgv => msv
2017-04-07 20:25 kgv Status resolved => assigned
2017-04-07 20:27 kgv Note Edited: 0065059
2017-04-07 20:31 kgv Note Edited: 0065059
2017-04-09 21:35 kgv Note Edited: 0065059
2017-04-10 10:06 msv Description Updated
2017-04-10 10:13 msv Note Edited: 0065058
2017-04-10 10:18 git Note Added: 0065071
2017-04-10 10:20 msv Note Added: 0065073
2017-04-10 10:20 msv Assigned To msv => kgv
2017-04-10 10:20 msv Status assigned => resolved
2017-04-10 10:23 kgv Note Added: 0065074
2017-04-10 10:23 kgv Assigned To kgv => bugmaster
2017-04-10 10:23 kgv Severity minor => tweak
2017-04-10 10:23 kgv Status resolved => reviewed
2017-04-10 10:48 apv Assigned To bugmaster => apv
2017-04-11 12:06 apv Test case number => Not needed
2017-04-11 12:53 apv Note Added: 0065127
2017-04-11 12:53 apv Assigned To apv => bugmaster
2017-04-11 12:53 apv Status reviewed => tested
2017-04-14 14:28 bugmaster Changeset attached => occt master 0bd575a7
2017-04-14 14:28 bugmaster Status tested => verified
2017-04-14 14:28 bugmaster Resolution open => fixed
2017-05-12 11:36 git Note Added: 0065956
2017-05-12 11:36 git Note Added: 0065957
2017-05-12 11:36 git Note Added: 0065958
2017-09-29 16:19 aiv Fixed in Version => 7.2.0
2017-09-29 16:25 aiv Status verified => closed