View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0026949 | Community | OCCT:Modeling Data | public | 2015-12-03 19:12 | 2016-04-20 15:50 |
| Reporter | Istvan Csanady | Assigned To | bugmaster | ||
| Priority | normal | Severity | minor | ||
| Status | closed | Resolution | fixed | ||
| Target Version | 7.0.0 | Fixed in Version | 7.0.0 | ||
| Summary | 0026949: Geom(2d)Adaptor_Curve/Surface should not do down casts in evaluation | ||||
| Description | 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. | ||||
| Steps To Reproduce | N/A | ||||
| Tags | No tags attached. | ||||
| Test case number | Not needed | ||||
|
|
patch.diff (38,128 bytes) |
|
|
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 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 |
|
|
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 |
|
2015-12-07 09:40 developer |
diff-2015-12-01T2116.log (25,883 bytes) |
|
|
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: /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 |
|
|
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 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. |
|
|
Dear BugMaster, Branch CR26949 is TESTED. |
|
|
Branch CR26949 has been deleted by kgv. SHA-1: 6add904ec8c0e883f774d5191cbcd01b4ecc9464 |
|
occt: master 3b25c0e8 2015-12-03 16:13:08 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 | ||
| 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 |
|
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 |
|
Note Added: 0048765 | |
| 2015-12-06 22:59 |
|
Status | new => resolved |
| 2015-12-06 22:59 |
|
Steps to Reproduce Updated | |
| 2015-12-06 22:59 |
|
Note Added: 0048766 | |
| 2015-12-06 22:59 |
|
Assigned To | msv => bugmaster |
| 2015-12-06 22:59 |
|
Status | resolved => reviewed |
| 2015-12-07 09:40 |
|
File Added: diff-2015-12-01T2116.log | |
| 2015-12-07 09:52 |
|
Note Added: 0048770 | |
| 2015-12-07 10:18 |
|
Assigned To | bugmaster => mkv |
| 2015-12-07 10:49 | Istvan Csanady | Note Added: 0048772 | |
| 2015-12-07 10:53 |
|
Note Added: 0048773 | |
| 2015-12-07 11:02 | Istvan Csanady | Note Added: 0048774 | |
| 2015-12-07 11:24 |
|
Note Added: 0048775 | |
| 2015-12-07 12:28 |
|
Note Added: 0048782 | |
| 2015-12-07 13:17 | Istvan Csanady | Note Added: 0048788 | |
| 2015-12-07 18:12 |
|
Note Added: 0048817 | |
| 2015-12-07 18:12 |
|
Note Added: 0048818 | |
| 2015-12-07 18:12 |
|
Assigned To | mkv => bugmaster |
| 2015-12-07 18:12 |
|
Status | reviewed => tested |
| 2015-12-07 18:12 |
|
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 |
|
Fixed in Version | => 7.0.0 |
| 2016-04-20 15:50 |
|
Status | verified => closed |
