Archived issue #0032265
Modeling Algorithms - some trivial code improvements in CSLib_Class2d and GCPnts_TangentialDeflection
Description
This issue is a part of changes, which were initially submitted into 0032261.
Hi,
in course of investigating diverse meshing problems with different models I have produced several trivial code improvements in the context of meshing algorithm, which I would like to share here.
The improvements are the following:
6. CSLib_Class2d even thought the implementation of NCollection_Handle::get is quite trivial, I saw it several times in the profiler taking some 5% of meshing time (maybe this was just due to debug mode and some address sanitizing - I am not sure)
7. GCPnts_TangentialDeflection::PerformCurve consider near points not only by parameter value, but also by 3D location
[signature removed]
Dima
Hi,
in course of investigating diverse meshing problems with different models I have produced several trivial code improvements in the context of meshing algorithm, which I would like to share here.
The improvements are the following:
6. CSLib_Class2d even thought the implementation of NCollection_Handle::get is quite trivial, I saw it several times in the profiler taking some 5% of meshing time (maybe this was just due to debug mode and some address sanitizing - I am not sure)
7. GCPnts_TangentialDeflection::PerformCurve consider near points not only by parameter value, but also by 3D location
[signature removed]
Dima
Steps to reproduce
N/A
Public activity
17 archived notes
Participants are labeled by their role within this record.
Branch [archived branch] has been created by Author.
[revision removed]
Detailed log of new commits:
Author: Dzmitry Razmyslovich
Date: Tue Mar 30 07:27:47 2021 +0200
0032265: Some trivial code improvements for CSLib_Class2d and GCPnts_TangentialDeflection
[revision removed]
Detailed log of new commits:
Author: Dzmitry Razmyslovich
Date: Tue Mar 30 07:27:47 2021 +0200
0032265: Some trivial code improvements for CSLib_Class2d and GCPnts_TangentialDeflection
I wonder what is the benefit of using reference instead of pointer in CSLib_Class2d.cxx.
I guess the changes in GCPnts_TangentialDeflection.pxx solve some real bug in your application. Is it possible to attach a shape and create a draw script to reproduce it?
Also, it is desirable to add a detailed info about changes in the commit message. What for the changes, what problem they solve.
Also, it is desirable to add a detailed info about changes in the commit message. What for the changes, what problem they solve.
> 6. CSLib_Class2d even thought the implementation of NCollection_Handle::get is quite trivial,
> I saw it several times in the profiler taking some 5% of meshing time (maybe this was just due to debug mode and some address sanitizing - I am not sure)
I think that profiler statistics was misleading - I highly doubt that operator->() may involve any extra CPU cycles in Release builds.
It would be useful to have some real performance estimations of entire function logic before/after patch to ensure that it is justified.
And, of course, profiling of Debug builds is usually not helpful (it is better using ReleaseWithDebugInfo builds for profiling).
> I saw it several times in the profiler taking some 5% of meshing time (maybe this was just due to debug mode and some address sanitizing - I am not sure)
I think that profiler statistics was misleading - I highly doubt that operator->() may involve any extra CPU cycles in Release builds.
It would be useful to have some real performance estimations of entire function logic before/after patch to ensure that it is justified.
And, of course, profiling of Debug builds is usually not helpful (it is better using ReleaseWithDebugInfo builds for profiling).
T* get () { return ((Ptr*)opencascade::handle<Standard_Transient>::get())->myPtr; }
The changes in GCPnts_TangentialDeflection.pxx is an early bailout, which just saves some CPU cycles, as the later conditions are going to fail anyway.
Regarding the changes in CSLib_Class2d - I surely agree, that there should be no impact of get method.
I will split the changes into 2 commits, so that they can be picked by one.
Regarding the changes in CSLib_Class2d - I surely agree, that there should be no impact of get method.
I will split the changes into 2 commits, so that they can be picked by one.
Branch [archived branch] has been updated forcibly by Author.
[revision removed]
[revision removed]
Please, check the latest commits
I would discard the changes in CSLib_Class2d.
Also, please do the commit message as follows:
1. The first line is a copy from bug summary.
2. 2nd line is empty
3. Add description of changes here.
1. The first line is a copy from bug summary.
2. 2nd line is empty
3. Add description of changes here.
Branch [archived branch] has been updated forcibly by Author.
[revision removed]
[revision removed]
Done
Branch [archived branch] has been updated forcibly by Participant.
[revision removed]
[revision removed]
Rebased on master, corrected the title of the commit message.
Testing http://jenkins-test-12.nnov.opencascade.com/view/CR0032265-master-msv/view/COMPARE/.
Testing http://jenkins-test-12.nnov.opencascade.com/view/CR0032265-master-msv/view/COMPARE/.
Dear Dmitry,
There was already discussion about checking 3D points in this code. It was in the bug #0030874. There we agreed to check points only by parameter value.
The current patch introduces the risk of skipping loops in a curve if the curve has self-intersection. We cannot pass such regression in the code.
Moreover, the benefit of this patch is not obvious. Could you give some numbers to compare performance?
There was already discussion about checking 3D points in this code. It was in the bug #0030874. There we agreed to check points only by parameter value.
The current patch introduces the risk of skipping loops in a curve if the curve has self-intersection. We cannot pass such regression in the code.
Moreover, the benefit of this patch is not obvious. Could you give some numbers to compare performance?
Dear Mikhail,
the performance impact of this change is not remarkable.
I am sorry, that I have spent your time on investigating again the already discussed topic. Somehow I forgot, that I have already submitted this patch.
Let's just close this issue.
the performance impact of this change is not remarkable.
I am sorry, that I have spent your time on investigating again the already discussed topic. Somehow I forgot, that I have already submitted this patch.
Let's just close this issue.
Dear Commenter 1, please close this bug.
Branch [archived branch] has been deleted by Commenter 4.
[revision removed]
[revision removed]
Related records