Visualization - Prebuild BVH for Select3D_SensitiveEntity in separate threads

Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

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

    0031757: Visualization - Prebuild BVH for Select3D_SensitiveEntity in separate threads

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

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

Branch [archived branch] has been updated by Author.

[revision removed]


Detailed log of new commits:

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

    Fixed remarks

Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

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

    0031757: Visualization - Prebuild BVH for Select3D_SensitiveEntity in separate threads

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Remarks have been fixed, please review branch CR31757_1

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

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

Branch [archived branch] has been updated by Author.

[revision removed]


Detailed log of new commits:

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

    Fixed remarks

Branch [archived branch] has been updated by Author.

[revision removed]


Detailed log of new commits:

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

    Fix remarks 2

Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

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

    0031757: Visualization - Prebuild BVH for Select3D_SensitiveEntity in separate threads

Remarks have been fixed, please review

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

0031757: Visualization - Prebuild BVH for Select3D_SensitiveEntity in separate threads CR31757_2
author	age <[email removed]>	
    Tue, 8 Sep 2020 11:44:21 +0000 (14:44 +0300)
committer	age <[email removed]>	
    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.

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Done

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Please raise the patch
- OCCT branch: [archived branch]

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

Combination -
OCCT branch : [archived branch]
master SHA - [revision removed]
[revision removed]
Products branch : [archived branch] SHA - [revision removed]
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

Branch [archived branch] has been deleted by Participant.

[revision removed]

Branch [archived branch] has been deleted by Participant.

[revision removed]

Branch [archived branch] has been deleted by Participant.

[revision removed]