MantisBT - Community
View Issue Details
0023952Community[OCCT] OCCT:Modeling Algorithmspublic2013-05-10 11:082015-02-01 15:04
Roman Lygin 
[OCCT] 6.7.0[OCCT] 6.7.0 
bugs modalg_5 bug23952_1 bug23952_2
0023952: Improving thread-safety of intersections, approximations and other modeling algorithms
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.
Use enclosed C++ test cases along with test models to derive regression test cases suitable for OCC test framework
No tags attached.
related to 0024500closed bugmaster Open CASCADE Sudden exit of DRAW after multiple execution of test in cycle 
related to 0024156closed bugmaster Open CASCADE It's necessary to add TODO in test cases to avoid known regressions on MacOS 
zip (2,243) 2013-05-10 11:08
zip (803) 2013-05-10 15:36
Issue History
2013-05-10 11:08Roman LyginNew Issue
2013-05-10 11:08Roman LyginAssigned To => jgv
2013-05-10 11:08Roman LyginFile Added:
2013-05-10 12:01Roman LyginNote Added: 0024353
2013-05-10 12:01Roman LyginStatusnew => resolved
2013-05-10 12:02Roman LyginNote Added: 0024354
2013-05-10 12:33abvNote Added: 0024355
2013-05-10 12:33abvAssigned Tojgv => bugmaster
2013-05-10 12:33abvStatusresolved => reviewed
2013-05-10 15:36Roman LyginNote Added: 0024356
2013-05-10 15:36Roman LyginFile Added:
2013-05-11 09:54abvAssigned Tobugmaster => Roman Lygin
2013-05-11 09:54abvStatusreviewed => assigned
2013-05-11 09:56abvNote Added: 0024358
2013-05-11 10:03abvNote Added: 0024359
2013-05-11 18:30abvNote Added: 0024362
2013-05-11 18:30abvAssigned ToRoman Lygin => bugmaster
2013-05-11 18:30abvStatusassigned => feedback
2013-05-13 09:02Roman LyginNote Added: 0024366
2013-05-13 09:02Roman LyginStatusfeedback => resolved
2013-05-13 09:23abvStatusresolved => reviewed
2013-05-14 17:05abvNote Added: 0024381
2013-05-14 17:09mkvAssigned Tobugmaster => apn
2013-05-18 14:32Roman LyginNote Added: 0024431
2013-05-20 09:04abvNote Added: 0024433
2013-05-20 09:29abvNote Added: 0024434
2013-05-31 13:38apnTest case number => bugs modalg_5 bug23952_1 bug23952_2
2013-05-31 15:54apnNote Added: 0024585
2013-05-31 16:21apnStatusreviewed => tested
2013-05-31 16:23bugmasterTarget Version => 6.7.0
2013-05-31 16:33apnAssigned Toapn => bugmaster
2013-05-31 18:25apnNote Added: 0024589
2013-06-03 14:48Roman LyginChangeset attached => occt master 1ef32e96
2013-06-03 14:48Roman LyginAssigned Tobugmaster => Roman Lygin
2013-06-03 14:48Roman LyginStatustested => verified
2013-06-03 14:48Roman LyginResolutionopen => fixed
2013-06-10 10:30apnChangeset attached => occt master 85c44a05
2013-06-10 10:30apnAssigned ToRoman Lygin => apn
2013-12-19 13:53bugmasterStatusverified => closed
2013-12-19 13:55bugmasterFixed in Version => 6.7.0
2013-12-26 12:28abvRelationship addedrelated to 0024500
2015-02-01 15:04abvRelationship addedrelated to 0024156

Roman Lygin   
2013-05-10 12:01   
fix pushed into git repository
Roman Lygin   
2013-05-10 12:02   
Highlighted modifications:

- 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.

- SQR made inline function

- 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.

- the solution is to take advantage of stack allocation (vs expensive heap allocation) and using respective math_* object constructors.

mmprcsn_ - safe to use as static var, as it is initialized once and then accessed for read-only

- 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
2013-05-10 12:33   
No remarks, please test
Roman Lygin   
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.
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
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).
2013-05-11 18:30   
Please run complete testing of branch CR23952 on both Windows and Linux
Roman Lygin   
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.
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").
Roman Lygin   
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.
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.
2013-05-20 09:29   
Please also test on 64-bit Windows
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)

No regressions

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.
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.