View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0031757Open CASCADEOCCT:Visualizationpublic2020-09-08 16:332020-12-02 17:13
ReporterageAssigned Tobugmaster  
PrioritynormalSeverityfeature 
Status closedResolutionfixed 
Target Version7.5.0Fixed in Version7.5.0 
Summary0031757: Visualization - Prebuild BVH for Select3D_SensitiveEntity in separate threads
DescriptionBVH for Select3D_SensitiveEntity are built when SelectMgr_ViewerSelector::TraverseSensitives() is called. In case of big model this building step can take a lot of time, so the first selection will be slow. It is suggested to build these BVH in parallel with the main thread to decrease time of the first selection.
Steps To ReproduceNot required
TagsNo tags attached.
Test case numberv3d/mesh/C1

Activities

git

2020-09-09 09:19

administrator   ~0094276

Branch CR31757 has been created by age.

SHA-1: 6c427a792155d143385cf96fbde036744cf236c0


Detailed log of new commits:

Author: age
Date: Tue Sep 8 14:44:21 2020 +0300

    0031757: Visualization - Prebuild BVH for Select3D_SensitiveEntity in separate threads

age

2020-09-09 09:23

developer   ~0094277

Please review the first version. Tests will be added after approval.

kgv

2020-09-10 15:10

developer   ~0094377 

+Standard_Address SelectMgr_BVHThreadPool::buildBVHThreadFunc (Standard_Address data)

Please add OSD::SetThreadLocalSignal() at the begginning of thread execution and put algorithm execution into try/catch.

+  Standard_Integer aBVHThreadsNum = theNbThreads > 0 ? theNbThreads : OSD_Parallel::NbLogicalProcessors() > 1 ? OSD_Parallel::NbLogicalProcessors() - 1 : 1;

I would expect the number of background thread to be 1 for this task by default.

+    aPool->myBVHListMutex.Lock();

Please prefer using Standard_Mutex::Sentry when possible.

+  for()
...
+    if (aPool->myToStopBVHThread)
+    {
+      break;
...
+  return (Standard_Address)(0);

Tip: single `return` without `break` would be clearer.

+//! Class defining a thread pool for building BVH for Select3D_SensitiveEntity in multi-threaded mode.
+class SelectMgr_BVHThreadPool : public Standard_Transient

> Class defining a thread pool for building BVH for the list of Select3D_SensitiveEntity within background thread(s).

+  Standard_EXPORT void BuildBVH (const Handle(Select3D_SensitiveEntity)& theEntity);

void AddEntity()

+protected:
+  static Standard_Address buildBVHThreadFunc (Standard_Address data);

private

+  //! Structure that will be passed to a separate thread
+  struct BVHBuild_Data
+  {
+    SelectMgr_BVHThreadPool* Pool;
+    Standard_Mutex Mutex;
+  };

+  NCollection_Array1<OSD_Thread> myBVHThreads;                         //!< threads to build BVH
+  NCollection_Array1<BVHBuild_Data> myBVHBuildData;                    //!< list of mutexes for building BVH

It would be clearer subclassing OSD_Thread instead of managing several arrays.

+void SelectMgr_BVHThreadPool::BuildBVH (const Handle(Select3D_SensitiveEntity)& theEntity)
+{

Makes sense to check if BVH tree is already build in this method.

+          NCollection_Vector<Handle(SelectMgr_SensitiveEntity)>::Iterator anIter(aNewSel->Entities());
+          for (; anIter.More(); anIter.Next())

Please declare iterator within for(;;) header.

+  if (myToPrebuildBVH)
+  {
+    myBVHThreadPool->LockBVHBuildMutex();
+  }

Please implement SelectMgr_BVHThreadPool::Sentry object that would care about this locking and unlocking in similar manner as Standard_Mutex::Sentry does.

+  Standard_Boolean ToPrebuildBVH()

const method.


StdSelect_BRepSelectionTool::PreBuildBVH() will still perform synchronous BVH build for large Trees within the patch, which I suppose should be avoided while enabling a background thread.

git

2020-09-11 16:50

administrator   ~0094482

Branch CR31757 has been updated by age.

SHA-1: 0656cd43c4b72990b6f55b07cf8767a2284e6abd


Detailed log of new commits:

Author: age
Date: Fri Sep 11 16:45:39 2020 +0300

    Fixed remarks

git

2020-09-11 16:51

administrator   ~0094483

Branch CR31757_1 has been created by age.

SHA-1: 5801e9bb0bfaf9c36382cf4e56f6bd196ba36f24


Detailed log of new commits:

Author: age
Date: Tue Sep 8 14:44:21 2020 +0300

    0031757: Visualization - Prebuild BVH for Select3D_SensitiveEntity in separate threads

git

2020-09-11 17:13

administrator   ~0094487

Branch CR31757_1 has been updated forcibly by age.

SHA-1: e10662c7dea5c3e6b0767f01cdf79f0380cd552a

git

2020-09-11 17:42

administrator   ~0094495

Branch CR31757_1 has been updated forcibly by age.

SHA-1: e31def80317f2d3168b67a92ea248c37aaaafde8

git

2020-09-14 10:26

administrator   ~0094695

Branch CR31757_1 has been updated forcibly by age.

SHA-1: 062e6d45fd2c0c0b17a64cbb20ee2b7c60e07898

git

2020-09-14 11:24

administrator   ~0094705

Branch CR31757_1 has been updated forcibly by age.

SHA-1: 631ab66ad9e7d170db29fd1e5240166ed6a1e64d

git

2020-09-14 15:24

administrator   ~0094737

Branch CR31757_1 has been updated forcibly by age.

SHA-1: 4c3ff79d2ac2e3588444774bf9b9367f7bf286dc

age

2020-09-14 15:26

developer   ~0094738

Remarks have been fixed, please review branch CR31757_1

http://occt-tests/CR31757_1-master-AGE-OCCT/Windows-64-VC14/diff_summary.html

kgv

2020-09-14 16:45

developer   ~0094755 

+  myBVHThreads = NCollection_Array1<BVHThread>(1, aBVHThreadsNum);

NCollection_Array1::Resize().

+void SelectMgr_BVHThreadPool::AddEntity (const Handle(Select3D_SensitiveEntity)& theEntity)
+{
+  myBVHListMutex.Lock();
+  myBVHToBuildList.Append (theEntity);

Method still doesn't check if BVH tree of entity has been invalidated or not.

+  Standard_Boolean CheckAndResetFailures (TCollection_AsciiString& theFailures, Standard_Boolean theToRaise);

Current design does not force application to call CheckAndResetFailures()/WaitForBVHBuild() methods, so that failures list might potentially grow without limits.
So far we do not expect BVH builders to throw any exceptions, so that it sounds reasonable simplifying this logic and just print error messages to Messenger within working threads rather than introducing mechanism propagating them to main thread.

+  theCommands.Add("vbvhprebuild",

vselbvhbuild


+ theCommands.Add("vbvhprebuildwait",

vselbvhbuild -wait

+                  "vbvhprebuild {0|1} [-threadsNb value]"

-nbThreads

+      if (!anEntity->IsInstance (STANDARD_TYPE(Select3D_SensitiveGroup)))
+      {
+        continue;
+      }
+
+      Handle(Select3D_SensitiveGroup) aGroup = Handle(Select3D_SensitiveGroup)::DownCast (anEntity);

Redundant IsInstance() check - DownCast() should be enough.
> if (Handle(Select3D_SensitiveGroup) aGroup = Handle(Select3D_SensitiveGroup)::DownCast (anEntity)) {}

+{
+  myBVHListMutex.Lock();
+  myBVHToBuildList.Append (theEntity);
+  myWakeEvent.Set();
+  myIdleEvent.Reset();
+  myBVHListMutex.Unlock();
+

Standard_Mutex::Sentry for myBVHListMutex.

--- /dev/null
+++ b/src/SelectMgr/SelectMgr_BVHThreadPool.hxx
@@ -0,0 +1,148 @@
+#ifndef _SelectMgr_BVHThreadPool_HeaderFile
+#define _SelectMgr_BVHThreadPool_HeaderFile
...
--- /dev/null
+++ b/src/SelectMgr/SelectMgr_BVHThreadPool.cxx
@@ -0,0 +1,199 @@
+#include <SelectMgr_BVHThreadPool.hxx>

Missing file header.

git

2020-09-16 12:50

administrator   ~0094904

Branch CR31757_1 has been updated by age.

SHA-1: 917c93eb4ed78e2c66f1b4ea4919d6d9f8882d95


Detailed log of new commits:

Author: age
Date: Wed Sep 16 12:53:19 2020 +0300

    Fixed remarks

git

2020-09-16 14:23

administrator   ~0094909

Branch CR31757_1 has been updated by age.

SHA-1: d4f4e33449391feb6ca4f120cb17cb3b0ada2a94


Detailed log of new commits:

Author: age
Date: Wed Sep 16 14:26:26 2020 +0300

    Fix remarks 2

git

2020-09-16 14:24

administrator   ~0094910

Branch CR31757_2 has been created by age.

SHA-1: 9769cc1e438e00d97758352ffaef53e696fa9e93


Detailed log of new commits:

Author: age
Date: Tue Sep 8 14:44:21 2020 +0300

    0031757: Visualization - Prebuild BVH for Select3D_SensitiveEntity in separate threads

age

2020-09-16 14:25

developer   ~0094911

Remarks have been fixed, please review

http://occt-tests/CR31757_1-master-AGE-OCCT/Windows-64-VC14/diff_summary.html

kgv

2020-09-16 14:45

developer   ~0094913 

0031757: Visualization - Prebuild BVH for Select3D_SensitiveEntity in separate threads CR31757_2
author	age <age@opencascade.com>	
    Tue, 8 Sep 2020 11:44:21 +0000 (14:44 +0300)
committer	age <age@opencascade.com>	
    Wed, 16 Sep 2020 11:27:08 +0000 (14:27 +0300)

Please add description of changes into commit message shortly describing:
- Addition of new method to Select3D_SensitiveEntity interface.
- Addition of new public method in selector for computing BVH in background and default behavior (remains the same).
- Addition of new class for managing background tasks.

git

2020-09-16 15:03

administrator   ~0094914

Branch CR31757_2 has been updated forcibly by age.

SHA-1: 6fbb61cc2da1608d48ee87094a0a0d520c983137

age

2020-09-16 15:04

developer   ~0094916

Done

git

2020-09-16 15:20

administrator   ~0094920

Branch CR31757_2 has been updated forcibly by age.

SHA-1: 3fb1070d2cbc9e7e3f700dac31522ab398338d73

kgv

2020-09-16 15:21

developer   ~0094922

Last edited: 2020-09-16 15:21

 Please raise the patch
- OCCT branch: CR31757_2.

abv

2020-09-19 09:30

manager   ~0095058

The declared purpose of this change is to improve performance. Are there any measurable improvements actually achieved?

bugmaster

2020-09-19 17:19

administrator   ~0095065

Combination -
OCCT branch : IR-2020-09-18
master SHA - b0b766826118f74b9857a932b8cec8c52a25c492
a206de37fbfa0bf71bd534ae47192bbec23b8522
Products branch : IR-2020-09-18 SHA - a6486d839da1ba1383ef6cc1a1a446a172f494c7
was compiled on Linux, MacOS and Windows platforms and tested in optimize mode.

Number of compiler warnings:
No new/fixed warnings

Regressions/Differences/Improvements:
No regressions/differences

CPU differences:
Debian80-64:
OCCT
Total CPU difference: 17528.97000000011 / 17378.390000000145 [+0.87%]
Products
Total CPU difference: 12127.32000000009 / 12079.540000000095 [+0.40%]
Windows-64-VC14:
OCCT
Total CPU difference: 18862.703125 / 18898.921875 [-0.19%]
Products
Total CPU difference: 13314.828125 / 13329.21875 [-0.11%]


Image differences :
No differences that require special attention

Memory differences :
No differences that require special attention

git

2020-09-20 11:13

administrator   ~0095111

Branch CR31757_2 has been deleted by inv.

SHA-1: 3fb1070d2cbc9e7e3f700dac31522ab398338d73

git

2020-09-20 11:13

administrator   ~0095112

Branch CR31757_1 has been deleted by inv.

SHA-1: d4f4e33449391feb6ca4f120cb17cb3b0ada2a94

git

2020-09-20 11:14

administrator   ~0095133

Branch CR31757 has been deleted by inv.

SHA-1: 0656cd43c4b72990b6f55b07cf8767a2284e6abd

Related Changesets

occt: master 6a2fb7a1

2020-09-08 11:44:21

age


Committer: bugmaster Details Diff		 0031757: Visualization - Prebuild BVH for Select3D_SensitiveEntity in separate threads

- Added a new mode in SelectMgr_ViewerSelector for computing BVH for Select3D_SensitiveEntity in background which can be activated via method SelectMgr_ViewerSelector::SetToPrebuildBVH(). Default behavior has not been changed.
- New class SelectMgr_BVHThreadPool manages background processing of BVH building queue.
- Added Select3D_SensitiveEntity::ToBuildBVH() method that checks if BVH (if it used) is in invalidated state. Defined this method for all standard classes inherited from Select3D_SensitiveEntity.		 Affected Issues
0031757
mod - src/AIS/AIS_ColoredShape.cxx Diff File
mod - src/MeshVS/MeshVS_DummySensitiveEntity.hxx Diff File
mod - src/MeshVS/MeshVS_Mesh.cxx Diff File
mod - src/Select3D/Select3D_SensitiveBox.hxx Diff File
mod - src/Select3D/Select3D_SensitiveCircle.cxx Diff File
mod - src/Select3D/Select3D_SensitiveCircle.hxx Diff File
mod - src/Select3D/Select3D_SensitiveEntity.hxx Diff File
mod - src/Select3D/Select3D_SensitiveFace.hxx Diff File
mod - src/Select3D/Select3D_SensitivePoint.hxx Diff File
mod - src/Select3D/Select3D_SensitiveSegment.hxx Diff File
mod - src/Select3D/Select3D_SensitiveSet.hxx Diff File
mod - src/Select3D/Select3D_SensitiveTriangle.hxx Diff File
mod - src/SelectMgr/FILES Diff File
add - src/SelectMgr/SelectMgr_BVHThreadPool.cxx Diff File
add - src/SelectMgr/SelectMgr_BVHThreadPool.hxx Diff File
mod - src/SelectMgr/SelectMgr_SelectionManager.cxx Diff File
mod - src/SelectMgr/SelectMgr_SelectionManager.hxx Diff File
mod - src/SelectMgr/SelectMgr_ViewerSelector.cxx Diff File
mod - src/SelectMgr/SelectMgr_ViewerSelector.hxx Diff File
mod - src/StdSelect/StdSelect_BRepSelectionTool.cxx Diff File
mod - src/ViewerTest/ViewerTest_ViewerCommands.cxx Diff File
add - tests/v3d/mesh/C1 Diff File

Issue History

Date Modified Username Field Change
2020-09-08 16:33 age New Issue
2020-09-08 16:33 age Assigned To => age
2020-09-09 09:19 git Note Added: 0094276
2020-09-09 09:20 age Status new => assigned
2020-09-09 09:23 age Note Added: 0094277
2020-09-09 09:23 age Assigned To age => kgv
2020-09-09 09:23 age Status assigned => resolved
2020-09-09 09:23 age Steps to Reproduce Updated
2020-09-10 15:10 kgv Note Added: 0094377
2020-09-11 16:50 git Note Added: 0094482
2020-09-11 16:51 git Note Added: 0094483
2020-09-11 17:13 git Note Added: 0094487
2020-09-11 17:42 git Note Added: 0094495
2020-09-14 10:26 git Note Added: 0094695
2020-09-14 11:24 git Note Added: 0094705
2020-09-14 15:24 git Note Added: 0094737
2020-09-14 15:26 age Note Added: 0094738
2020-09-14 16:45 kgv Note Added: 0094755
2020-09-16 12:50 git Note Added: 0094904
2020-09-16 14:06 kgv Assigned To kgv => age
2020-09-16 14:06 kgv Status resolved => assigned
2020-09-16 14:06 kgv Severity minor => feature
2020-09-16 14:06 kgv Product Version 7.5.0 =>
2020-09-16 14:23 git Note Added: 0094909
2020-09-16 14:24 git Note Added: 0094910
2020-09-16 14:25 age Note Added: 0094911
2020-09-16 14:25 age Assigned To age => kgv
2020-09-16 14:25 age Status assigned => resolved
2020-09-16 14:45 kgv Note Added: 0094913
2020-09-16 14:45 kgv Assigned To kgv => age
2020-09-16 14:45 kgv Status resolved => assigned
2020-09-16 15:03 git Note Added: 0094914
2020-09-16 15:04 age Note Added: 0094916
2020-09-16 15:04 age Assigned To age => kgv
2020-09-16 15:04 age Status assigned => resolved
2020-09-16 15:20 git Note Added: 0094920
2020-09-16 15:21 kgv Note Added: 0094922
2020-09-16 15:21 kgv Assigned To kgv => bugmaster
2020-09-16 15:21 kgv Status resolved => reviewed
2020-09-16 15:21 kgv Note Edited: 0094922
2020-09-19 09:30 abv Note Added: 0095058
2020-09-19 15:41 bugmaster Test case number => v3d/mesh/C1
2020-09-19 17:19 bugmaster Note Added: 0095065
2020-09-19 17:19 bugmaster Status reviewed => tested
2020-09-20 10:55 bugmaster Changeset attached => occt master 6a2fb7a1
2020-09-20 10:55 bugmaster Status tested => verified
2020-09-20 10:55 bugmaster Resolution open => fixed
2020-09-20 11:13 git Note Added: 0095111
2020-09-20 11:13 git Note Added: 0095112
2020-09-20 11:14 git Note Added: 0095133
2020-12-02 16:43 emo Fixed in Version => 7.5.0
2020-12-02 17:13 emo Status verified => closed