View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0028325 | Community | OCCT:Modeling Algorithms | public | 2016-12-29 20:25 | 2023-08-01 15:08 |
| Reporter | Istvan Csanady | Assigned To | |||
| Priority | normal | Severity | tweak | ||
| Status | assigned | Resolution | open | ||
| Target Version | Unscheduled | ||||
| Summary | 0028325: Modeling Algorithms - ProjLib_ComputeApproxOnPolarSurface unnecessary copies the surface | ||||
| Description | ProjLib_ComputeApproxOnPolarSurface unnecessarily copies a surface object, that can lead to significant performance loss in some cases. Please find the attached patch file. | ||||
| Steps To Reproduce | Not needed | ||||
| Tags | No tags attached. | ||||
| Test case number | |||||
|
|
surfacecopy.diff (1,896 bytes) |
|
|
Branch CR28325 has been created by msv. SHA-1: d2c3d328efbd79697827514c36c064c07ed8cbe8 Detailed log of new commits: Author: msv Date: Tue Jan 24 12:29:02 2017 +0300 0028325: Unnecessary copying of surface Giving up creation of new adaptor of the surface without real need. Using the available adaptor instead. |
|
|
Actually, the surface is not copied in the patched code. Just new GeomAdaptor_Surface is created with reduced domain from the surface of another adaptor defined in the upper scope. |
|
|
I have tested the patch. It brings no regressions. Here are the results of performance comparison. The patch code figures are on the left, master are on the right. I am giving here the figures only for the tests having difference of more than 10%. The more detailed log with difference 5% and higher is attached. CPU boolean bcommon_2d: 7.859375 / 9.140625 [-14.02%] CPU boolean bopcommon_complex C6: 12.5625 / 14.890625 [-15.63%] CPU boolean boptuc_complex A4: 10.921875 / 12.640625 [-13.60%] CPU boolean boptuc_complex A6: 12.65625 / 15.734375 [-19.56%] CPU bugs mesh bug25378_3_3: 26.609375 / 30.671875 [-13.25%] CPU bugs mesh bug27384_1: 35.078125 / 39.4375 [-11.05%] CPU bugs modalg_2 bug22109_2: 1.96875 / 2.75 [-28.41%] CPU bugs modalg_2 bug22109_4: 1.90625 / 2.703125 [-29.48%] CPU bugs modalg_2 bug22109_5: 1.671875 / 2.53125 [-33.95%] CPU bugs modalg_6 bug26567: 503.40625 / 566.25 [-11.10%] CPU bugs moddata_1 bug20391: 2.390625 / 3.59375 [-33.48%] CPU bugs step bug365_2: 11.96875 / 13.953125 [-14.22%] CPU heal surface_to_revolution_standard: 17.203125 / 19.453125 [-11.57%] CPU mesh advanced_incmesh_parallel B5: 19.78125 / 24.171875 [-18.16%] CPU offset with_intersect_20: 12.296875 / 13.703125 [-10.26%] CPU perf fclasses: 5.53125 / 6.40625 [-13.66%] CPU perf moddata bug623: 4.03125 / 5.953125 [-32.28%] CPU thrusection specific: 3.765625 / 4.578125 [-17.75%] CPU v3d edge: 4.421875 / 4.984375 [-11.29%] CPU bugs modalg_6 bug26188: 23.546875 / 7.09375 [+231.94%] CPU bugs modalg_6 bug26196: 22.109375 / 6.015625 [+267.53%] CPU de step_3 A6: 22.609375 / 11.015625 [+105.25%] CPU de step_3 A7: 22.8125 / 10.96875 [+107.98%] CPU de step_3 F2: 656.15625 / 181.671875 [+261.18%] CPU de step_3: 1572.90625 / 1093.390625 [+43.86%] CPU perf modalg bug27085_2: 5.5625 / 4.390625 [+26.69%] CPU thrusection not_solids: 6.8125 / 5.703125 [+19.45%] CPU v3d glsl: 6.859375 / 6.21875 [+10.30%] CPU v3d raytrace bug26617: 4.171875 / 2.6875 [+55.23%] CPU v3d raytrace sample_cube_clamp: 8.46875 / 6.484375 [+30.60%] Total CPU difference: 21008.796875 / 21027.125 [-0.09%] |
|
2017-01-30 10:24 developer |
diffcpu-2017-01-24T1522.log (5,399 bytes) |
|
|
We can see that the patch brings as performance improvements (-9% in average for the full log) as degradations (+64% in average). I have analyzed what happens and found the causes of both improvement and degradation. Let's consider the test: CPU perf moddata bug623: 4.03125 / 5.953125 [-32.28%] In this case the master version spends much time to rebuild bspline cache. In the patched version the cache is not rebuilt, because the same adaptor is used. This is the cause of performance improvement. Now let's consider the test: CPU bugs modalg_6 bug26188: 23.546875 / 7.09375 [+231.94%] In this case in the master version the most number of times when a point is projected on the surface, the local search is used (the class Extrema_GenLocateExtPS). And in the patched version significant number of times local search is failed and global search is used (Extrema_ExtPS). Below are some figures from Intel amplifier. Number of spent seconds and approximate number of calls is shown: patched: Extrema_GenLocateExtPS 2.8s 188000 Extrema_ExtPS 11s 13000 ref: Extrema_GenLocateExtPS 3s 249000 Extrema_ExtPS 0.2s 0 |
|
|
Summarizing and taking into account that total CPU difference is very small (-0.09%), I can say that the patch (as is) is not worth to be taken. However, I think we could do something to improve. If we could create a copy of adaptor with preserving bspline cache and with new parameters range it could speed up execution without performance degradation. |
|
|
Dear Istvan, could you provide your test case? We would like to analyze it before taking the final decision. |
|
|
I believe the test files were related to the issue 0028274 and I used the same or similar geometries to test it. |
|
|
Using the test files attached to 0028274, I did not found any change in execution time between master version and this patch. So, I would keep idea expressed in the post above (0028325:0063177). |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2016-12-29 20:25 | Istvan Csanady | New Issue | |
| 2016-12-29 20:25 | Istvan Csanady | Assigned To | => msv |
| 2016-12-29 20:25 | Istvan Csanady | File Added: surfacecopy.diff | |
| 2017-01-24 12:25 |
|
Summary | Unecessary copying of surface => Unnecessary copying of surface |
| 2017-01-30 10:12 |
|
Status | new => assigned |
| 2017-01-30 10:13 | git | Note Added: 0063172 | |
| 2017-01-30 10:14 |
|
Status | assigned => resolved |
| 2017-01-30 10:14 |
|
Steps to Reproduce Updated | |
| 2017-01-30 10:16 |
|
Note Added: 0063173 | |
| 2017-01-30 10:23 |
|
Note Added: 0063174 | |
| 2017-01-30 10:24 |
|
File Added: diffcpu-2017-01-24T1522.log | |
| 2017-01-30 10:58 |
|
Note Added: 0063176 | |
| 2017-01-30 11:03 |
|
Note Added: 0063177 | |
| 2017-01-30 11:23 |
|
Note Added: 0063178 | |
| 2017-01-30 11:23 |
|
Assigned To | msv => Istvan Csanady |
| 2017-01-30 11:23 |
|
Status | resolved => feedback |
| 2017-01-30 15:05 | Istvan Csanady | Note Added: 0063194 | |
| 2017-02-01 10:59 |
|
Note Added: 0063279 | |
| 2017-02-01 11:01 |
|
Assigned To | Istvan Csanady => msv |
| 2017-02-01 11:01 |
|
Status | feedback => assigned |
| 2017-07-21 11:22 |
|
Target Version | 7.2.0 => 7.3.0 |
| 2017-12-05 17:09 |
|
Target Version | 7.3.0 => 7.4.0 |
| 2019-08-12 16:44 |
|
Target Version | 7.4.0 => 7.5.0 |
| 2019-12-26 15:37 |
|
Summary | Unnecessary copying of surface => Modeling Algorithms - ProjLib_ComputeApproxOnPolarSurface unnecessary copies the surface |
| 2020-09-15 12:39 |
|
Target Version | 7.5.0 => 7.6.0 |
| 2021-08-29 18:53 |
|
Target Version | 7.6.0 => 7.7.0 |
| 2022-10-24 10:42 |
|
Target Version | 7.7.0 => 7.8.0 |
| 2023-08-01 15:08 | dpasukhi | Target Version | 7.8.0 => Unscheduled |
