Geom(2d)Adaptor_Curve/Surface should not do down casts in evaluation

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.

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.

Branch [archived branch] has been created by Participant.

[revision removed]


Detailed log of new commits:

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

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.

No remarks, please test

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

Shouldn't we do the same for Bezier geometries?

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

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:

[user path removed]/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

I see. Thank you Istvan.

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

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

Dear Commenter 1,
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.
[revision removed]

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.

Dear Commenter 1,
Branch CR26949 is TESTED.

Branch [archived branch] has been deleted by Participant.

[revision removed]