View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0023952 | Community | OCCT:Modeling Algorithms | public | 2013-05-10 11:08 | 2021-07-28 09:38 |
| Reporter | Roman Lygin | Assigned To | apn | ||
| Priority | normal | Severity | minor | ||
| Status | closed | Resolution | fixed | ||
| Platform | A | OS | L | ||
| Target Version | 6.7.0 | Fixed in Version | 6.7.0 | ||
| Summary | 0023952: Improving thread-safety of intersections, approximations and other modeling algorithms | ||||
| Description | The modifications make core classes used in surface-surface intersections and surface approximations thread-safe. The latter improves thread-safety of using algorithms such as sweeps, plates and alike. The notes below will highlight some key details of the modifications. The rest should be straightforward to understand. | ||||
| Steps To Reproduce | Use enclosed C++ test cases along with test models to derive regression test cases suitable for OCC test framework | ||||
| Tags | No tags attached. | ||||
| Test case number | bugs modalg_5 bug23952_1 bug23952_2 | ||||
| parent of | 0032495 | closed | bugmaster | Community | Coding rules - eliminate CLang UndefinedBehaviorSanitizer warnings |
| related to | 0024500 | closed | bugmaster | Open CASCADE | Sudden exit of DRAW after multiple execution of test in cycle |
| related to | 0024156 | closed | bugmaster | Open CASCADE | It's necessary to add TODO in test cases to avoid known regressions on MacOS |
|
|
test.zip (2,243 bytes) |
|
|
fix pushed into git repository |
|
|
Highlighted modifications: math_FunctionRoots.cxx - the variable 'methode' made static const as it's effectively defined in compile time - StaticSol of TColStd_SequenceOfReal made an auto-variable which is created upon every math_FunctionRoots constructor call. It is impossible to be located only in occurrences where it is used, as they are located in different scopes. Moreover, those branches are never executed with methode equal to 1. So this is pure extra overhead. However it is a mere initialization of 5 primitive values in TCollection_BaseSequence constructor, and thus is negligible comparing to computational part. math_Powell.cxx - SQR made inline function math_Recipies.cxx - PYTHAG() made inline function. Note that the method Random2() remains not thread-safe by its design. So thread-safety must be guaranteed by its callers. ApproxInt_* - the solution is to take advantage of stack allocation (vs expensive heap allocation) and using respective math_* object constructors. AdvApp2Var_MathBase.cxx mmprcsn_ - safe to use as static var, as it is initialized once and then accessed for read-only AdvApp2Var_SysBase.cxx - methods related to object allocation (e.g. mcrrqst_(), mcrdelt_(), etc) have been made local methods, and allocation management has been made class fields. Respectively callers have been updated to allocate an instance of AdvApp2Var_SysBase on stack and use it. - note that making these structures object fields introduces stack footprint (1000 * 12* sizeof (intptr_t)) which on 64bit architectures is about 94K. The way to address footprint could be to substitute with struct where each field could be less than intptr_t (remember that introduction of intptr_t was made as part of 0022786). IntWalk_IWalking (and similarly IntWalk_PWalking) - removed undefined default constructor - added former static variables NbPointsConfondusConsecutifs and EpsilonSembleTropGrand to be class members. - TestDeflection() uses NbPointsConfondusConsecutifs and EpsilonSembleTropGrand relying on previous calls (i.e. it increments NbPointsConfondusConsecutifs assuming that this is the call of on the same data used in previous call to TestDeflection()). However previous memory model (static) violated this logic (in principle the values of NbPointsConfondusConsecutifs could remain from working on other data) what could result in unpredictable behavior. Now when correcting this logic and making them local to an object may theoretically have some side effect. Such effects should be analyzed if they have been previously just hidden by the wrong logic. - warning: I did not have test case that go through IntWalk_PWalking and it looks like its instantiation is never used. If so, perhaps it warrants removing IntWalk_PWalking completely from the source base |
|
|
No remarks, please test |
|
|
AdvApp2Var_SysBase.hxx has been reworked: 1. The number of maximum allowed allocation request has been reduced from 1000 to 32 (in fact the actual maximum known number is 7) 2. The explicit structure type has been defined to describe each allocation request - this will help data access and reduce memory footprint 3. The documentation has been added to ease future maintenance The code has been pushed in the repository. The test case has been made more aggressive to launch 32 threads to check no stack overflow. |
|
|
case1-upd.zip (803 bytes) |
|
|
Roman, running tests on Windows I got a plenty of regressions. Try in DRAW: Draw[]> testgrid blend You should see at least these regressions (other tests require data files): blend simple F5 blend tolblend_simple A1 A2 D3 D5 |
|
|
Note that AFAIR static variables are initialized to zero by compiler, and converting them to local variables you should initialize them explicitly to zero to keep the same behavior (I believe something may depend on this). |
|
|
Please run complete testing of branch CR23952 on both Windows and Linux |
|
|
As discussed in several emails with Andrey, the failures are not reproduced. A side issue in IntAna_Curve::D1u() has been discovered, which provoked sporadic behavior and might lead to floating point exceptions when a local variable SigneSqrtDis remained uninitialized as #NAN#. This issue could be provoked on a prior version. The issue has been fixed and the fix pushed into the repository (same branch CR23952). The version of plain 6.6.0 + the total fix produces clean results, with a handful of failures specific to my environment and not to this fix. For instance Windows fails on bugs vis bug23747_{1,2}, Linux - on 3rdparty export (and others, related to UnitsAPI.dat), and boolean bopfuse_simple ZP6. Tested on Windows, vc10, 32bit, debug/release; Linux, gcc4.1.2, 64bit, release. With that, the ownership is returned to the bugmaster. |
|
|
Just as a side note, I confirm that problems reported on 0023952:0024358 are stably reproduced on another machine (Windows 7, AMD 64-bit), with VS 2008 and VS 2010, in Debug and Release mode. In Debug, it complains on heap corruption ("application wrote to memory after end of heap buffer"). |
|
|
After further analysis, it has been revealed that the failures stem from the fact that making variables local the alignment of doubles became 4 bytes (vs 8 bytes when using static variables - even if this is not formally guaranteed by a compiler). The code in AdvApp2Var has been sensitive to 8 bytes alignment and did not treat well 4 bytes alignment: there appeared heap corruption when using MMGT_OPT=0 (due to writing at the end of allocated buffer). When using OPT=2 (TBB allocator) this was not an issue as TBB allocated slightly larger chunks and overflow did not cause real corruption. The fix has been made to enforce 8 bytes alignment (by simply using double* t = 0 instead of double t[1] as the access is anyway done via computed offset). A few other simplifications have been added. The fix has been pushed into the repository (CR23952). testgrid passes for vc10/debug/32 with MMGT_OPT=0. |
|
|
No remarks, I have made just one minor correction renaming const N to MAX_ALLOC_NB. Tests pass Ok on my system. For testers: after reporting results of on-going testing (no in-depth analysis is needed!), please re-test the current state of CR23952. |
|
|
Please also test on 64-bit Windows |
|
|
Dear BugMaster, Branch CR23952(and products from GIT master) was compiled on Linux and Windows platforms and tested without rebase. SHA-1: 60594345d84dcc94627aa68cb51eb75c422ba0d5 Number of compiler warnings: occt component : Linux: 2 (2 on master) Windows: 0 (11 on master) products component : Linux: 0 (0 on master) Windows: 63 (63 on master) Regressions: No regressions Improvements: No improvements Testing cases: bugs modalg_5 bug23952_1 bug23952_2 - OK Testing on Linux: Total MEMORY difference: 366173884 / 365700080 Total CPU difference: 48228.89000000025 / 44945.65000000121 Testing on Windows: Total MEMORY difference: 406488344 / 405134120 Total CPU difference: 30903.78125 / 35138.015625 There are not serious differences in images found by testdiff. |
|
|
Also branch CR23952 was tested on 64-bit Windows. There are no regressions regarding to results on current state of master. |
|
occt: master 1ef32e96 2013-05-31 13:04:58 Details Diff |
0023952: Improving thread-safety of intersections, approximations and other modeling algorithms AdvApp2Var_SysBase::mcrgene_ size reduced from 1000 to 32 elements, and each element reworked into typed structure. fixed IntAna_Curve.cxx to prevent access to #NAN# SigneSqrtDis fixed alignment of doubles by 8 bytes, and minor corrections Minor correction: static const N given more specific name (MAX_ALLOC_NB) Added QAcommands OCC23952sweep and OCC23952intersect Added test cases bugs/modalg_5/bug23952_1 bug23952_2 |
Affected Issues 0023952 |
|
| mod - src/AdvApp2Var/AdvApp2Var_ApproxF2var.cxx | Diff File | ||
| mod - src/AdvApp2Var/AdvApp2Var_MathBase.cxx | Diff File | ||
| mod - src/AdvApp2Var/AdvApp2Var_SysBase.cxx | Diff File | ||
| mod - src/AdvApp2Var/AdvApp2Var_SysBase.hxx | Diff File | ||
| mod - src/AdvApp2Var/AdvApp2Var_SysBase_baseinit.cxx | Diff File | ||
| mod - src/ApproxInt/ApproxInt_ImpPrmSvSurfaces.gxx | Diff File | ||
| mod - src/ApproxInt/ApproxInt_PrmPrmSvSurfaces.gxx | Diff File | ||
| mod - src/IntAna/IntAna_Curve.cxx | Diff File | ||
| mod - src/IntPatch/IntPatch_HInterTool.cdl | Diff File | ||
| mod - src/IntPatch/IntPatch_HInterTool.cxx | Diff File | ||
| mod - src/IntPatch/IntPatch_ImpPrmIntersection.cxx | Diff File | ||
| mod - src/IntPatch/IntPatch_RstInt.cxx | Diff File | ||
| mod - src/IntStart/IntStart_SearchInside.gxx | Diff File | ||
| mod - src/IntTools/IntTools_SurfaceRangeLocalizeData.cxx | Diff File | ||
| mod - src/IntWalk/IntWalk_IWalking.cdl | Diff File | ||
| mod - src/IntWalk/IntWalk_IWalking_1.gxx | Diff File | ||
| mod - src/IntWalk/IntWalk_IWalking_2.gxx | Diff File | ||
| mod - src/IntWalk/IntWalk_IWalking_3.gxx | Diff File | ||
| mod - src/IntWalk/IntWalk_IWalking_4.gxx | Diff File | ||
| mod - src/IntWalk/IntWalk_IWalking_5.gxx | Diff File | ||
| mod - src/IntWalk/IntWalk_IWalking_6.gxx | Diff File | ||
| mod - src/IntWalk/IntWalk_PWalking.cdl | Diff File | ||
| mod - src/IntWalk/IntWalk_PWalking_1.gxx | Diff File | ||
| mod - src/IntWalk/IntWalk_PWalking_3.gxx | Diff File | ||
| mod - src/math/math_FunctionRoots.cxx | Diff File | ||
| mod - src/math/math_Powell.cxx | Diff File | ||
| mod - src/math/math_Recipes.cxx | Diff File | ||
| mod - src/QABugs/QABugs_19.cxx | Diff File | ||
| add - tests/bugs/modalg_5/bug23952_1 | Diff File | ||
| add - tests/bugs/modalg_5/bug23952_2 | Diff File | ||
|
occt: master 85c44a05 2013-06-05 06:34:49 Details Diff |
0023952: Improving thread-safety of intersections, approximations and other modeling algorithms Modified QAcommands OCC23952sweep and OCC23952intersect to prevent exception in appropriate test cases |
Affected Issues 0023952 |
|
| mod - src/QABugs/QABugs_19.cxx | Diff File | ||
| add - tests/bugs/modalg_5/bug23952_1 | Diff File | ||
| add - tests/bugs/modalg_5/bug23952_2 | Diff File | ||
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2013-05-10 11:08 | Roman Lygin | New Issue | |
| 2013-05-10 11:08 | Roman Lygin | Assigned To | => jgv |
| 2013-05-10 11:08 | Roman Lygin | File Added: test.zip | |
| 2013-05-10 12:01 | Roman Lygin | Note Added: 0024353 | |
| 2013-05-10 12:01 | Roman Lygin | Status | new => resolved |
| 2013-05-10 12:02 | Roman Lygin | Note Added: 0024354 | |
| 2013-05-10 12:33 |
|
Note Added: 0024355 | |
| 2013-05-10 12:33 |
|
Assigned To | jgv => bugmaster |
| 2013-05-10 12:33 |
|
Status | resolved => reviewed |
| 2013-05-10 15:36 | Roman Lygin | Note Added: 0024356 | |
| 2013-05-10 15:36 | Roman Lygin | File Added: case1-upd.zip | |
| 2013-05-11 09:54 |
|
Assigned To | bugmaster => Roman Lygin |
| 2013-05-11 09:54 |
|
Status | reviewed => assigned |
| 2013-05-11 09:56 |
|
Note Added: 0024358 | |
| 2013-05-11 10:03 |
|
Note Added: 0024359 | |
| 2013-05-11 18:30 |
|
Note Added: 0024362 | |
| 2013-05-11 18:30 |
|
Assigned To | Roman Lygin => bugmaster |
| 2013-05-11 18:30 |
|
Status | assigned => feedback |
| 2013-05-13 09:02 | Roman Lygin | Note Added: 0024366 | |
| 2013-05-13 09:02 | Roman Lygin | Status | feedback => resolved |
| 2013-05-13 09:23 |
|
Status | resolved => reviewed |
| 2013-05-14 17:05 |
|
Note Added: 0024381 | |
| 2013-05-14 17:09 |
|
Assigned To | bugmaster => apn |
| 2013-05-18 14:32 | Roman Lygin | Note Added: 0024431 | |
| 2013-05-20 09:04 |
|
Note Added: 0024433 | |
| 2013-05-20 09:29 |
|
Note Added: 0024434 | |
| 2013-05-31 13:38 | apn | Test case number | => bugs modalg_5 bug23952_1 bug23952_2 |
| 2013-05-31 15:54 | apn | Note Added: 0024585 | |
| 2013-05-31 16:21 | apn | Status | reviewed => tested |
| 2013-05-31 16:23 | bugmaster | Target Version | => 6.7.0 |
| 2013-05-31 16:33 | apn | Assigned To | apn => bugmaster |
| 2013-05-31 18:25 | apn | Note Added: 0024589 | |
| 2013-06-03 14:48 | Roman Lygin | Changeset attached | => occt master 1ef32e96 |
| 2013-06-03 14:48 | Roman Lygin | Assigned To | bugmaster => Roman Lygin |
| 2013-06-03 14:48 | Roman Lygin | Status | tested => verified |
| 2013-06-03 14:48 | Roman Lygin | Resolution | open => fixed |
| 2013-06-10 10:30 | apn | Changeset attached | => occt master 85c44a05 |
| 2013-06-10 10:30 | apn | Assigned To | Roman Lygin => apn |
| 2013-12-19 13:53 | bugmaster | Status | verified => closed |
| 2013-12-19 13:55 | bugmaster | Fixed in Version | => 6.7.0 |
| 2013-12-26 12:28 |
|
Relationship added | related to 0024500 |
| 2015-02-01 15:04 |
|
Relationship added | related to 0024156 |
| 2021-07-28 09:38 | kgv | Relationship added | parent of 0032495 |
