MantisBT
Mantis Bug Tracker Workflow

View Issue Details Jump to Notes ] Related Changesets ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0026949Community[OCCT] OCCT:Modeling Datapublic2015-12-03 19:122016-04-20 15:50
ReporterIstvan Csanady 
Assigned Tobugmaster 
PrioritynormalSeverityminor 
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version 
Target Version[OCCT] 7.0.0Fixed in Version[OCCT] 7.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 Filesdiff file icon patch.diff (38,128 bytes) 2015-12-03 19:12
log file icon diff-2015-12-01T2116.log (25,883 bytes) 2015-12-07 09:40

- Relationships

-  Notes
(0048760)
msv (developer)
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 (developer)
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 (administrator)
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 (manager)
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 (manager)
2015-12-06 22:59

No remarks, please test
(0048770)
msv (developer)
2015-12-07 09:52

I have deleted my previous message, as I tested not correct version of the fix.
(0048772)
Istvan Csanady (developer)
2015-12-07 10:49

Shouldn't we do the same for Bezier geometries?
(0048773)
msv (developer)
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 (developer)
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 (developer)
2015-12-07 11:24

I see. Thank you Istvan.
(0048782)
abv (manager)
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 (developer)
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 (tester)
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 (tester)
2015-12-07 18:12

Dear BugMaster,
Branch CR26949 is TESTED.
(0053034)
git (administrator)
2016-04-17 14:01

Branch CR26949 has been deleted by kgv.

SHA-1: 6add904ec8c0e883f774d5191cbcd01b4ecc9464

- Related Changesets
occt: master 3b25c0e8
Timestamp: 2015-12-03 16:13:08
Author: 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
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 View Revisions
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:49 msv Note Added: 0048769
2015-12-07 09:51 msv Note Deleted: 0048769
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 user533 Fixed in Version => 7.0.0
2016-04-20 15:50 user533 Status verified => closed


Copyright © 2000 - 2018 MantisBT Team
Powered by Mantis Bugtracker