View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0026949CommunityOCCT:Modeling Datapublic2015-12-03 19:122016-04-20 15:50
ReporterIstvan Csanady Assigned Tobugmaster  
PrioritynormalSeverityminor 
Status closedResolutionfixed 
Target Version7.0.0Fixed in Version7.0.0 
Summary0026949: Geom(2d)Adaptor_Curve/Surface should not do down casts in evaluation
DescriptionWhen a (2d) BSpline curve is evaluated a lot of times, constantly downcasting myCurve to Handle(Geom_BSplineCurve) cna be time consuming. For example on iOS I noticed that it can take up to the 30% of the CPU time during meshing and extrema calculation. This can be easily prevented, by saveing a pointer to the BSpline object in the load method, and the modification is simple and straightforward. Patch is attached.
Steps To ReproduceN/A
TagsNo tags attached.
Test case numberNot needed

Attached Files

  • patch.diff (38,128 bytes)
  • diff-2015-12-01T2116.log (25,883 bytes)

Activities

Istvan Csanady

2015-12-03 19:12

developer  

patch.diff (38,128 bytes)

msv

2015-12-04 18:16

developer   ~0048760

I think this patch is disputable.

1. Why do you take care of eliminating of down casts of only b-spline geometry type. There are other down cast operations in these classes concerning other geometry types.
2. If we insert in adaptor class one handle for each geometry type then adaptor will contain too many fields unjustified.

Possible solution to the 2nd problem could be to use union of handles.

Anyway, we should test how this patch influence performance on other platforms.

Istvan Csanady

2015-12-04 18:24

developer   ~0048761

1. Because BSpline downcasts are called in D0/D1 etc. calls, thus if an algorithm evaluates a curve a lot (like in extrema calculation), it can cause a significant performance penalty. Other downcasts are just used in methods that are not called that often in any algorithm. But it can be considered apply the same to Bezier curves as well.

2. That's right, but since BSplines and Bezier curves are handled as special cases, this might can be reasonable. Or having a reference in the BSpline cache to the BSpline representation, and using that pointer in the case of BSplines.

git

2015-12-06 22:54

administrator   ~0048764

Branch CR26949 has been created by abv.

SHA-1: 6add904ec8c0e883f774d5191cbcd01b4ecc9464


Detailed log of new commits:

Author: Istvan Csanady
Date: Thu Dec 3 17:13:08 2015 +0100

    0026949: Geom(2d)Adaptor_Curve/Surface should not do down casts in evaluation
    
    Prevent downcasting in curve evaluation in GeomAdaptor classes

abv

2015-12-06 22:59

manager   ~0048765

Istvan, I have pushed your commit to branch CR26949, and confirm that it yields noticeable gain of performance (-4% of CPU time by all tests, VC++ 10 x64), thus thank you for spotting and fixing this issue! Note that patch is corrected -- some unnecessary down casts were still preaent, and absence of nullification of new field was causing errors.

abv

2015-12-06 22:59

manager   ~0048766

No remarks, please test

msv

2015-12-07 09:40

developer  

diff-2015-12-01T2116.log (25,883 bytes)

msv

2015-12-07 09:52

developer   ~0048770

I have deleted my previous message, as I tested not correct version of the fix.

Istvan Csanady

2015-12-07 10:49

developer   ~0048772

Shouldn't we do the same for Bezier geometries?

msv

2015-12-07 10:53

developer   ~0048773

I think yes, but it is needed to elaborate a solution with using the same memory for alternative handles (union).

Istvan Csanady

2015-12-07 11:02

developer   ~0048774

I am not sure it would compile, because Handles have non-trivial destructors.

Try the following:

union HandleUnion
{
    Handle(Geom2d_Curve) c1;
    Handle(Geom2d_Curve) c2;
};

HandleUnion h;

Clang fails to compile this with:

/Users/icsanady/Desktop/Shapr3D/src/Kernel/src/Rendering.mm:65:17: Call to implicitly-deleted default constructor of 'HandleUnion'
Default constructor of 'HandleUnion' is implicitly deleted because variant field 'c1' has a non-trivial default constructor

msv

2015-12-07 11:24

developer   ~0048775

I see. Thank you Istvan.

abv

2015-12-07 12:28

manager   ~0048782

Regarding other types of curves and surfaces, I believe this can make sense, but needs to be justified by tests. The important lesson of this issue is that DownCast() can really be a bottle neck, thus we need to pay attention to this, and put some effort to identify other cases when this can be optimized.

Besides, Istvan, if you have detailed results of profiler, can you indicate what was the main source of the performance loss -- DownCast() itself (type check) or creation / destruction of the new handle (i.e. incrementing and decrementing of reference counter)?

In the latter case, we can try general optimization - make DownCast() to return pointer instead of Handle(). This would allow avoiding creation of Handle in some cases. (Actually I have tried this some time ago, but it did not compile straight due to some places where code was based on DownCast() returning handle, e.g. by applying .IsNull() directly to its result).

Another point is that with current level of C++ we should be able to rewrite some constructs to remove extra type checks. For instance, instead of

if (aCurve->IsKind(STANDARD_TYPE(Geom_Circle))
{
  Handle(Geom_Circle) aCircle = Handle(Geom_Circle)::DownCast (aCurve);
  ...
}

we can write

if (Handle(Geom_Circle) aCircle = Handle(Geom_Circle)::DownCast (aCurve))
{
  ...
}

I have not tested this with current version of OCCT, but should work...

Istvan Csanady

2015-12-07 13:17

developer   ~0048788

The top item was DynamicCast, and dynamic_cast from the libc++ ABI.

Another option we can try is to use static_cast, but that would require to implement a StaticCast method in Standard_Handle. And geometry classes could have a Type field that could return the curve/surface type, so a static_cast could be applied on handles like:

if (curve->Type() == GeomAbs_BSplineCurve)
{
   auto bspline = Handle(Geom_BSplineCurve)::StaticCast(curve);
}

mkv

2015-12-07 18:12

tester   ~0048817

Dear BugMaster,
Branch CR26949 from occt git-repository (and master from products git-repository) was compiled on Linux, MacOS and Windows platforms and tested on Release mode.
SHA-1: 6add904ec8c0e883f774d5191cbcd01b4ecc9464

Number of compiler warnings:

occt component :
Linux: 0 (0 on master)
Windows: 0 (0 on master)
MacOS : 135 (135 on master)

products component :
Linux: 37 (37 on master)
Windows: 0 (0 on master)

Regressions/Differences/Improvements:
No regressions/differences

Testing cases:
Not needed

Testing on Linux:
occt component :
Total MEMORY difference: 93125747 / 91969488 [+1.26%]
Total CPU difference: 19589.11999999977 / 19861.86999999963 [-1.37%]
products component :
Total MEMORY difference: 26210778 / 26022482 [+0.72%]
Total CPU difference: 7567.039999999997 / 7421.12000000001 [+1.97%]

Testing on Windows:
occt component :
Total MEMORY difference: 56183956 / 56183094 [+0.00%]
Total CPU difference: 17735.51968849893 / 18342.45717909876 [-3.31%]
products component :
Total MEMORY difference: 16502137 / 16515468 [-0.08%]
Total CPU difference: 5642.244167999962 / 5665.6131177999705 [-0.41%]

There are no differences in images found by testdiff.

mkv

2015-12-07 18:12

tester   ~0048818

Dear BugMaster,
Branch CR26949 is TESTED.

git

2016-04-17 14:01

administrator   ~0053034

Branch CR26949 has been deleted by kgv.

SHA-1: 6add904ec8c0e883f774d5191cbcd01b4ecc9464

Related Changesets

occt: master 3b25c0e8

2015-12-03 16:13:08

Istvan Csanady


Committer: bugmaster Details Diff		 0026949: Geom(2d)Adaptor_Curve/Surface should not do down casts in evaluation

Prevent downcasting in curve evaluation in GeomAdaptor classes		 Affected Issues
0026949
mod - src/Geom2dAdaptor/Geom2dAdaptor_Curve.cxx Diff File
mod - src/Geom2dAdaptor/Geom2dAdaptor_Curve.hxx Diff File
mod - src/GeomAdaptor/GeomAdaptor_Curve.cxx Diff File
mod - src/GeomAdaptor/GeomAdaptor_Curve.hxx Diff File
mod - src/GeomAdaptor/GeomAdaptor_Surface.cxx Diff File
mod - src/GeomAdaptor/GeomAdaptor_Surface.hxx Diff File

Issue History

Date Modified Username Field Change
2015-12-03 19:12 Istvan Csanady New Issue
2015-12-03 19:12 Istvan Csanady Assigned To => msv
2015-12-03 19:12 Istvan Csanady File Added: patch.diff
2015-12-04 18:16 msv Note Added: 0048760
2015-12-04 18:24 Istvan Csanady Note Added: 0048761
2015-12-06 22:54 git Note Added: 0048764
2015-12-06 22:59 abv Note Added: 0048765
2015-12-06 22:59 abv Status new => resolved
2015-12-06 22:59 abv Steps to Reproduce Updated
2015-12-06 22:59 abv Note Added: 0048766
2015-12-06 22:59 abv Assigned To msv => bugmaster
2015-12-06 22:59 abv Status resolved => reviewed
2015-12-07 09:40 msv File Added: diff-2015-12-01T2116.log
2015-12-07 09:52 msv Note Added: 0048770
2015-12-07 10:18 mkv Assigned To bugmaster => mkv
2015-12-07 10:49 Istvan Csanady Note Added: 0048772
2015-12-07 10:53 msv Note Added: 0048773
2015-12-07 11:02 Istvan Csanady Note Added: 0048774
2015-12-07 11:24 msv Note Added: 0048775
2015-12-07 12:28 abv Note Added: 0048782
2015-12-07 13:17 Istvan Csanady Note Added: 0048788
2015-12-07 18:12 mkv Note Added: 0048817
2015-12-07 18:12 mkv Note Added: 0048818
2015-12-07 18:12 mkv Assigned To mkv => bugmaster
2015-12-07 18:12 mkv Status reviewed => tested
2015-12-07 18:12 mkv Test case number => Not needed
2015-12-11 11:38 bugmaster Changeset attached => occt master 3b25c0e8
2015-12-11 11:38 bugmaster Status tested => verified
2015-12-11 11:38 bugmaster Resolution open => fixed
2016-04-17 14:01 git Note Added: 0053034
2016-04-20 15:42 aiv Fixed in Version => 7.0.0
2016-04-20 15:50 aiv Status verified => closed