MantisBT - Community
View Issue Details
0028325Community[OCCT] OCCT:Modeling Algorithmspublic2016-12-29 20:252020-09-15 12:39
Istvan Csanady 
msv 
normaltweak 
assignedopen 
 
[OCCT] 7.6.0* 
0028325: Modeling Algorithms - ProjLib_ComputeApproxOnPolarSurface unnecessary copies the surface
ProjLib_ComputeApproxOnPolarSurface unnecessarily copies a surface object, that can lead to significant performance loss in some cases. Please find the attached patch file.
Not needed
No tags attached.
diff surfacecopy.diff (1,896) 2016-12-29 20:25
https://tracker.dev.opencascade.org/
log diffcpu-2017-01-24T1522.log (5,399) 2017-01-30 10:24
https://tracker.dev.opencascade.org/
Issue History
2016-12-29 20:25Istvan CsanadyNew Issue
2016-12-29 20:25Istvan CsanadyAssigned To => msv
2016-12-29 20:25Istvan CsanadyFile Added: surfacecopy.diff
2017-01-24 12:25msvSummaryUnecessary copying of surface => Unnecessary copying of surface
2017-01-30 10:12msvStatusnew => assigned
2017-01-30 10:13gitNote Added: 0063172
2017-01-30 10:14msvStatusassigned => resolved
2017-01-30 10:14msvSteps to Reproduce Updatedbug_revision_view_page.php?rev_id=15961#r15961
2017-01-30 10:16msvNote Added: 0063173
2017-01-30 10:23msvNote Added: 0063174
2017-01-30 10:24msvFile Added: diffcpu-2017-01-24T1522.log
2017-01-30 10:58msvNote Added: 0063176
2017-01-30 11:03msvNote Added: 0063177
2017-01-30 11:23msvNote Added: 0063178
2017-01-30 11:23msvAssigned Tomsv => Istvan Csanady
2017-01-30 11:23msvStatusresolved => feedback
2017-01-30 15:05Istvan CsanadyNote Added: 0063194
2017-02-01 10:59msvNote Added: 0063279
2017-02-01 11:01msvAssigned ToIstvan Csanady => msv
2017-02-01 11:01msvStatusfeedback => assigned
2017-07-21 11:22msvTarget Version7.2.0 => 7.3.0
2017-12-05 17:09msvTarget Version7.3.0 => 7.4.0
2019-08-12 16:44msvTarget Version7.4.0 => 7.5.0
2019-12-26 15:37msvSummaryUnnecessary copying of surface => Modeling Algorithms - ProjLib_ComputeApproxOnPolarSurface unnecessary copies the surface
2020-09-15 12:39msvTarget Version7.5.0 => 7.6.0*

Notes
(0063172)
git   
2017-01-30 10:13   
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.
(0063173)
msv   
2017-01-30 10:16   
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.
(0063174)
msv   
2017-01-30 10:23   
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%]
(0063176)
msv   
2017-01-30 10:58   
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
(0063177)
msv   
2017-01-30 11:03   
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.
(0063178)
msv   
2017-01-30 11:23   
Dear Istvan, could you provide your test case? We would like to analyze it before taking the final decision.
(0063194)
Istvan Csanady   
2017-01-30 15:05   
I believe the test files were related to the issue 0028274 and I used the same or similar geometries to test it.
(0063279)
msv   
2017-02-01 10:59   
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).