MantisBT - Open CASCADE
View Issue Details
0028636Open CASCADE[OCCT] OCCT:Codingpublic2017-04-06 11:192017-09-29 16:25
nbv 
bugmaster 
normaltweak 
closedfixed 
[OCCT] 7.1.0 
[OCCT] 7.2.0[OCCT] 7.2.0 
Not needed
0028636: Optimisation of gp_* classes in order to avoid unnecesary calling gp_Dir* constructors with normalisation
Many 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.
New test case is not required.
No tags attached.
Issue History
2017-04-06 11:19nbvNew Issue
2017-04-06 11:19nbvAssigned To => msv
2017-04-06 11:20nbvDescription Updatedbug_revision_view_page.php?rev_id=16361#r16361
2017-04-06 11:21nbvRelationship addedrelated to 0028211
2017-04-06 12:32msvStatusnew => assigned
2017-04-06 12:32msvProduct Version => 7.1.0
2017-04-06 15:28gitNote Added: 0064981
2017-04-06 15:29msvNote Added: 0064982
2017-04-06 15:29msvAssigned Tomsv => emv
2017-04-06 15:29msvStatusassigned => resolved
2017-04-06 15:42abvNote Added: 0064984
2017-04-06 15:49emvNote Added: 0064988
2017-04-06 15:49emvAssigned Toemv => bugmaster
2017-04-06 15:49emvStatusresolved => reviewed
2017-04-06 16:14mkvAssigned Tobugmaster => apv
2017-04-06 16:15msvNote Added: 0064994
2017-04-06 16:15msvAssigned Toapv => msv
2017-04-06 16:15msvStatusreviewed => assigned
2017-04-07 19:44gitNote Added: 0065056
2017-04-07 20:01gitNote Added: 0065057
2017-04-07 20:08msvNote Added: 0065058
2017-04-07 20:08msvAssigned Tomsv => kgv
2017-04-07 20:08msvStatusassigned => resolved
2017-04-07 20:25kgvNote Added: 0065059
2017-04-07 20:25kgvAssigned Tokgv => msv
2017-04-07 20:25kgvStatusresolved => assigned
2017-04-07 20:27kgvNote Edited: 0065059bug_revision_view_page.php?bugnote_id=65059#r16376
2017-04-07 20:31kgvNote Edited: 0065059bug_revision_view_page.php?bugnote_id=65059#r16377
2017-04-09 21:35kgvNote Edited: 0065059bug_revision_view_page.php?bugnote_id=65059#r16386
2017-04-10 10:06msvDescription Updatedbug_revision_view_page.php?rev_id=16387#r16387
2017-04-10 10:13msvNote Edited: 0065058bug_revision_view_page.php?bugnote_id=65058#r16389
2017-04-10 10:18gitNote Added: 0065071
2017-04-10 10:20msvNote Added: 0065073
2017-04-10 10:20msvAssigned Tomsv => kgv
2017-04-10 10:20msvStatusassigned => resolved
2017-04-10 10:23kgvNote Added: 0065074
2017-04-10 10:23kgvAssigned Tokgv => bugmaster
2017-04-10 10:23kgvSeverityminor => tweak
2017-04-10 10:23kgvStatusresolved => reviewed
2017-04-10 10:48apvAssigned Tobugmaster => apv
2017-04-11 12:06apvTest case number => Not needed
2017-04-11 12:53apvNote Added: 0065127
2017-04-11 12:53apvAssigned Toapv => bugmaster
2017-04-11 12:53apvStatusreviewed => tested
2017-04-14 14:28bugmasterChangeset attached => occt master 0bd575a7
2017-04-14 14:28bugmasterStatustested => verified
2017-04-14 14:28bugmasterResolutionopen => fixed
2017-05-12 11:36gitNote Added: 0065956
2017-05-12 11:36gitNote Added: 0065957
2017-05-12 11:36gitNote Added: 0065958
2017-09-29 16:19aivFixed in Version => 7.2.0
2017-09-29 16:25aivStatusverified => closed

Notes
(0064981)
git   
2017-04-06 15:28   
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.
(0064982)
msv   
2017-04-06 15:29   
Dear Eugeny, please review the fix.
(0064984)
abv   
2017-04-06 15:42   
Mikhail, please provide results of your performance measurements showing the positive effect of the change.
(0064988)
emv   
2017-04-06 15:49   
Reviewed.
(0064994)
msv   
2017-04-06 16:15   
Test case is to be created.
(0065056)
git   
2017-04-07 19:44   
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.
(0065057)
git   
2017-04-07 20:01   
Branch CR28636_1 has been updated forcibly by msv.

SHA-1: e57e1e343ba81e3975fb089716ebc61016988d89
(0065058)
msv   
2017-04-07 20:08   
(edited on: 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.

(0065059)
kgv   
2017-04-07 20:25   
(edited on: 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.

(0065071)
git   
2017-04-10 10:18   
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.
(0065073)
msv   
2017-04-10 10:20   
Please review again.
(0065074)
kgv   
2017-04-10 10:23   
Please test the patch.
(0065127)
apv   
2017-04-11 12:53   
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%]
(0065956)
git   
2017-05-12 11:36   
Branch CR28636 has been deleted by kgv.

SHA-1: 75da7906240cfc6663a455a38912cb4885cc6898
(0065957)
git   
2017-05-12 11:36   
Branch CR28636_1 has been deleted by kgv.

SHA-1: e57e1e343ba81e3975fb089716ebc61016988d89
(0065958)
git   
2017-05-12 11:36   
Branch CR28636_2 has been deleted by kgv.

SHA-1: 9c1a3136a17e3647253a6ee625ed2279428c7031