MantisBT - Community
View Issue Details
0026949Community[OCCT] OCCT:Modeling Datapublic2015-12-03 19:122016-04-20 15:50
Istvan Csanady 
bugmaster 
normalminor 
closedfixed 
 
[OCCT] 7.0.0[OCCT] 7.0.0 
Not needed
0026949: Geom(2d)Adaptor_Curve/Surface should not do down casts in evaluation
When 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.
N/A
No tags attached.
diff patch.diff (38,128) 2015-12-03 19:12
https://tracker.dev.opencascade.org/
log diff-2015-12-01T2116.log (25,883) 2015-12-07 09:40
https://tracker.dev.opencascade.org/
Issue History
2015-12-03 19:12Istvan CsanadyNew Issue
2015-12-03 19:12Istvan CsanadyAssigned To => msv
2015-12-03 19:12Istvan CsanadyFile Added: patch.diff
2015-12-04 18:16msvNote Added: 0048760
2015-12-04 18:24Istvan CsanadyNote Added: 0048761
2015-12-06 22:54gitNote Added: 0048764
2015-12-06 22:59abvNote Added: 0048765
2015-12-06 22:59abvStatusnew => resolved
2015-12-06 22:59abvSteps to Reproduce Updatedbug_revision_view_page.php?rev_id=12442#r12442
2015-12-06 22:59abvNote Added: 0048766
2015-12-06 22:59abvAssigned Tomsv => bugmaster
2015-12-06 22:59abvStatusresolved => reviewed
2015-12-07 09:40msvFile Added: diff-2015-12-01T2116.log
2015-12-07 09:49msvNote Added: 0048769
2015-12-07 09:51msvNote Deleted: 0048769
2015-12-07 09:52msvNote Added: 0048770
2015-12-07 10:18mkvAssigned Tobugmaster => mkv
2015-12-07 10:49Istvan CsanadyNote Added: 0048772
2015-12-07 10:53msvNote Added: 0048773
2015-12-07 11:02Istvan CsanadyNote Added: 0048774
2015-12-07 11:24msvNote Added: 0048775
2015-12-07 12:28abvNote Added: 0048782
2015-12-07 13:17Istvan CsanadyNote Added: 0048788
2015-12-07 18:12mkvNote Added: 0048817
2015-12-07 18:12mkvNote Added: 0048818
2015-12-07 18:12mkvAssigned Tomkv => bugmaster
2015-12-07 18:12mkvStatusreviewed => tested
2015-12-07 18:12mkvTest case number => Not needed
2015-12-11 11:38bugmasterChangeset attached => occt master 3b25c0e8
2015-12-11 11:38bugmasterStatustested => verified
2015-12-11 11:38bugmasterResolutionopen => fixed
2016-04-17 14:01gitNote Added: 0053034
2016-04-20 15:42aivFixed in Version => 7.0.0
2016-04-20 15:50aivStatusverified => closed

Notes
(0048760)
msv   
2015-12-04 18:16   
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.
(0048761)
Istvan Csanady   
2015-12-04 18:24   
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.
(0048764)
git   
2015-12-06 22:54   
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
(0048765)
abv   
2015-12-06 22:59   
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.
(0048766)
abv   
2015-12-06 22:59   
No remarks, please test
(0048770)
msv   
2015-12-07 09:52   
I have deleted my previous message, as I tested not correct version of the fix.
(0048772)
Istvan Csanady   
2015-12-07 10:49   
Shouldn't we do the same for Bezier geometries?
(0048773)
msv   
2015-12-07 10:53   
I think yes, but it is needed to elaborate a solution with using the same memory for alternative handles (union).
(0048774)
Istvan Csanady   
2015-12-07 11:02   
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
(0048775)
msv   
2015-12-07 11:24   
I see. Thank you Istvan.
(0048782)
abv   
2015-12-07 12:28   
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...
(0048788)
Istvan Csanady   
2015-12-07 13:17   
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);
}
(0048817)
mkv   
2015-12-07 18:12   
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.
(0048818)
mkv   
2015-12-07 18:12   
Dear BugMaster,
Branch CR26949 is TESTED.
(0053034)
git   
2016-04-17 14:01   
Branch CR26949 has been deleted by kgv.

SHA-1: 6add904ec8c0e883f774d5191cbcd01b4ecc9464