MantisBT
Mantis Bug Tracker Workflow

View Issue Details Jump to Notes ] Related Changesets ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0023952Community[OCCT] OCCT:Modeling Algorithmspublic2013-05-10 11:082015-02-01 15:04
ReporterRoman Lygin 
Assigned Toapn 
PrioritynormalSeverityminor 
StatusclosedResolutionfixed 
PlatformAOSLOS VersionL
Product Version 
Target Version[OCCT] 6.7.0Fixed in Version[OCCT] 6.7.0 
Summary0023952: Improving thread-safety of intersections, approximations and other modeling algorithms
DescriptionThe 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 ReproduceUse enclosed C++ test cases along with test models to derive regression test cases suitable for OCC test framework
TagsNo tags attached.
Test case numberbugs modalg_5 bug23952_1 bug23952_2
Attached Fileszip file icon test.zip (2,243 bytes) 2013-05-10 11:08
zip file icon case1-upd.zip (803 bytes) 2013-05-10 15:36

- Relationships
related to 0024500closedbugmaster Open CASCADE Sudden exit of DRAW after multiple execution of test in cycle 
related to 0024156closedbugmaster Open CASCADE It's necessary to add TODO in test cases to avoid known regressions on MacOS 

-  Notes
(0024353)
Roman Lygin (developer)
2013-05-10 12:01

fix pushed into git repository
(0024354)
Roman Lygin (developer)
2013-05-10 12:02

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
(0024355)
abv (manager)
2013-05-10 12:33

No remarks, please test
(0024356)
Roman Lygin (developer)
2013-05-10 15:36

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.
(0024358)
abv (manager)
2013-05-11 09:56

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
(0024359)
abv (manager)
2013-05-11 10:03

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).
(0024362)
abv (manager)
2013-05-11 18:30

Please run complete testing of branch CR23952 on both Windows and Linux
(0024366)
Roman Lygin (developer)
2013-05-13 09:02

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.
(0024381)
abv (manager)
2013-05-14 17:05

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").
(0024431)
Roman Lygin (developer)
2013-05-18 14:32

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.
(0024433)
abv (manager)
2013-05-20 09:04

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.
(0024434)
abv (manager)
2013-05-20 09:29

Please also test on 64-bit Windows
(0024585)
apn (administrator)
2013-05-31 15:54

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.
(0024589)
apn (administrator)
2013-05-31 18:25

Also branch CR23952 was tested on 64-bit Windows.
There are no regressions regarding to results on current state of master.

- Related Changesets
occt: master 1ef32e96
Timestamp: 2013-05-31 13:04:58
Author: Roman Lygin
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
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
Timestamp: 2013-06-05 06:34:49
Author: apn
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
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 ]

- Issue History
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 abv Note Added: 0024355
2013-05-10 12:33 abv Assigned To jgv => bugmaster
2013-05-10 12:33 abv 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 abv Assigned To bugmaster => Roman Lygin
2013-05-11 09:54 abv Status reviewed => assigned
2013-05-11 09:56 abv Note Added: 0024358
2013-05-11 10:03 abv Note Added: 0024359
2013-05-11 18:30 abv Note Added: 0024362
2013-05-11 18:30 abv Assigned To Roman Lygin => bugmaster
2013-05-11 18:30 abv 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 abv Status resolved => reviewed
2013-05-14 17:05 abv Note Added: 0024381
2013-05-14 17:09 mkv Assigned To bugmaster => apn
2013-05-18 14:32 Roman Lygin Note Added: 0024431
2013-05-20 09:04 abv Note Added: 0024433
2013-05-20 09:29 abv 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 abv Relationship added related to 0024500
2015-02-01 15:04 abv Relationship added related to 0024156


Copyright © 2000 - 2018 MantisBT Team
Powered by Mantis Bugtracker