Added support for TBB 2021 by Idclip · Pull Request #1027 · AcademySoftwareFoundation/openvdb

Idclip · 2021-04-13T16:22:51Z

I believe this is the remaining work needed to support TBB 2021 except for the updates to Queue.h. This also needs testing so marking as a draft and pushing up for visibility.

Signed-off-by: Nick Avramoussis 4256455+Idclip@users.noreply.github.com

For details see: AcademySoftwareFoundation#1027

e4lam · 2021-05-10T16:37:41Z

@Idclip I've noticed that in tools/PointAdvect.h we're still making calls to tbb::task::self().cancel_group_execution();. Is this still something yet to be addressed for TBB 2021 support?

In H19, I've added a UTparallelCancelGroupExecution(); that wraps this in UT_ParallelUtil.h with the appropriate defines to suppress the TBB 2020 deprecation warnings. The intention here being that when we move up to oneTBB 2021 (or beyond), then we can switch this to current_context()->cancel_group_execution() in one place. I've made a stab at fixing this in openvdb_houdini/Utils.h like this:

#if SYS_VERSION_FULL_INT < 0x130000f7 // 19.0.247
#include <tbb/task.h>
static inline void
UTparallelCancelGroupExecution()
{
    tbb::task::self().cancel_group_execution();
}
#endif

However, then I realized that this is insufficient because of tools/PointAdvect.h. Have you had any thoughts on this? Offhand, it feels like we would need to have a similar wrapper in openvdb itself.

Idclip · 2021-05-12T13:32:06Z

hey @e4lam! I don't remember running into this, but it may very well be that I didn't properly try to build all the unit tests. As this isn't instantiated anywhere in the core lib/bins I probably didn't see it. You're saying that tbb::task::self().cancel_group_execution(); needs replacing with current_context()->cancel_group_execution()? Did current_context()->cancel_group_execution() exist in TBB 2019/2020? If so we might just be able to switch to that outright? If not, and tbb::task::self().cancel_group_execution(); was completely removed, that complicates things. We'd need to compile time branch in the headers, so we'd need a reliable #TBB define to rely on along with some wrapper that would exist in VDB itself.

e4lam · 2021-05-12T14:36:33Z

@Idclip Starting with oneTBB 2021, tbb::task::self() was removed and current_context() was added. If we want to support both TBB and oneTBB, then a compile time branch is necessary. Did you try building openvdb_houdini with oneTBB 2021? I would expect it to fail.

Idclip · 2021-05-12T14:40:17Z

@e4lam I did not get that far, thanks for confirming. Up till this point it seemed everything had a replacement which overlapped the versions we support. As you say, we'll need a wrapper in the VDB headers for this which could subsequently be used in the houdini plugin.

e4lam · 2021-05-13T15:04:37Z

For reference, I've looked into this and it looks like TBB_INTERFACE_VERSION_MAJOR >= 12 could be a good test for oneTBB.

Annoyingly, the macro is provided by different headers though.
TBB: #include <tbb/tbb_stddef.h>
oneTBB: #include <tbb/version.h>

Idclip · 2021-05-13T15:41:30Z

Annoyingly, the macro is provided by different headers though.

I'm going to assume that at least something consistent in TBB 2020/2021 propagates this define? i.e. maybe we include blocked_range.h or something. Not ideal, but don't see a better way if the version header has changed.

e4lam · 2021-05-13T16:00:58Z

Oh interesting, yeah including blocked_range.h looks like the easiest thing to do.

Signed-off-by: Nick Avramoussis <4256455+Idclip@users.noreply.github.com>

Idclip · 2021-06-10T15:58:44Z

I think I'm almost at a point to make this PR official. @e4lam I've added the wrapper logic to a new header util/Threading.h. I'm going to add at least one kind of CI test for TBB2021 before un-drafting(?) this PR

danrbailey · 2021-06-10T17:08:12Z

I like this new threading header a lot, that's a good idea. One question is about whether we could potentially introduce a new namespace for all threading-related functionality? openvdb::thread::cancelGroupExecution() ? I don't know whether that would mandate a new openvdb/thread dir as well, but it would be nice to at least separate out everything related to threading and interaction with TBB in terms of namespacing if possible. It could also pave the way for potentially adopting a wrapper to TBB in the future that would allow us to build without it (openvdb::thread::ParallelFor, openvdb::thread::BlockedRange, etc), similar to the approach that nanovdb has taken.

e4lam

Thank you for all the work on this!

openvdb_houdini/openvdb_houdini/SOP_OpenVDB_Sample_Points.cc

Idclip · 2021-06-10T19:04:29Z

One question is about whether we could potentially introduce a new namespace for all threading-related functionality?

I have no problem with this, but yeah it should probably live in a new appropriately named folder. Slightly odd to have such small logic introduce a new folder but, as you say, the new infrastructure will set a precedent to allow us to extending any more threading logic.

Signed-off-by: Nick Avramoussis <4256455+Idclip@users.noreply.github.com>

Idclip · 2021-06-11T10:01:55Z

I've added a basic test with an additional Mac build which builds the lib, binaries and unit tests. This really warrants something like a new latest.yml file which builds everything, or additions to the ax and houdini configs too, but I've left that out for now.

@danrbailey I've moved Threading.h into thread/Threading.h and renamed the namespace

Idclip · 2021-06-11T10:38:47Z

.github/workflows/build.yml

+         -DOPENVDB_CXX_STRICT=OFF -DUSE_BLOSC=OFF -DUSE_ZLIB=OFF
+    - name: test
+      shell: bash
+      run: cd build && ctest -V


New, very basic CI for TBB2021 in lieu of the intel docker images

Idclip · 2021-06-11T10:39:25Z

openvdb/openvdb/cmd/openvdb_render.cc

+// compile time branching (as we still support 2018), we use it in 2018 too by
+// enabling the below define.
+#define TBB_PREVIEW_GLOBAL_CONTROL 1
+#include <tbb/global_control.h>


Significant change

The VDB Render binary now uses tbb::global_control over tbb::task_scheduler_init

Idclip · 2021-06-11T10:41:16Z

openvdb/openvdb/io/Queue.cc

+
+        // get the global task arena
+        tbb::task_arena arena(tbb::task_arena::attach{});
+        arena.enqueue([task = std::move(task)] { task.execute(); });


Significant change

Instead of queueing tasks through task::enqueue, add them to the global task arena.

@jmlait I think this is correct? i.e. tbb::task_arena arena(tbb::task_arena::attach{}); gets a hooks to the "default" arena so I don't need to keep the arena in scope? This could be improved by having an arena per Queue, but I'm trying to keep the changes as minimal as possible.

The change is faithful to what the code previously did. Offhand, I don't see an advantage to using an arena per queue?

Thanks for confirming @e4lam. I'm trying to figure out what the better API would be, us wrapping the logic or delegating it to the caller. i.e:

Queue q; q.concurrency(/*max threads*/1); // where each queue has its own configurable arena q.write(...);

vs

// set arena for current scope { tbb::task_arena arena(/*1*/); Queue q; q.write(...) }

It's the same debate as the bool threaded = true argument that appears everywhere. In any case, not relevant for this PR.

Idclip · 2021-06-11T10:41:56Z

openvdb/openvdb/math/Coord.h


-namespace tbb { class split; } // forward declaration
-
+#include <tbb/blocked_range.h> // for tbb::split


Significant change

I couldn't find a nice way to fix the forward declaration between versions.

Idclip · 2021-06-11T10:42:49Z

openvdb/openvdb/tools/MeshToVolume.h

 {
    const auto grainSize = std::max<size_t>(
-        length / tbb::task_scheduler_init::default_num_threads(), 1024);
+        length / tbb::this_task_arena::max_concurrency(), 1024);


Significant change

This should now be affected by the caller configuring their own arena

Idclip · 2021-06-11T10:43:02Z

openvdb/openvdb/tools/Morphology.h


        // @note this grain size is used for optimal threading
-        const size_t numThreads = size_t(tbb::task_scheduler_init::default_num_threads());
+        const size_t numThreads = size_t(tbb::this_task_arena::max_concurrency());


Significant change

This should now be affected by the caller configuring their own arena

Idclip · 2021-06-11T10:43:19Z

openvdb/openvdb/tools/PointPartitioner.h

    using IndexPairListMapPtr = std::shared_ptr<IndexPairListMap>;

-    size_t numTasks = 1, numThreads = size_t(tbb::task_scheduler_init::default_num_threads());
+    size_t numTasks = 1, numThreads = size_t(tbb::this_task_arena::max_concurrency());


Significant change

This should now be affected by the caller configuring their own arena

Idclip · 2021-06-11T10:43:36Z

openvdb/openvdb/tools/VolumeToMesh.h

 {
    const auto grainSize = std::max<size_t>(
-        length / tbb::task_scheduler_init::default_num_threads(), 1024);
+        length / tbb::this_task_arena::max_concurrency(), 1024);


Significant change

This should now be affected by the caller configuring their own arena

Idclip · 2021-06-11T10:44:33Z

openvdb/openvdb/unittest/TestValueAccessor.cc

-                r[i] = new(allocate_child()) ReadTask(acc);
-                w[i] = new(allocate_child()) WriteTask(acc);
+                tasks.run([&] { ReadTask r(acc); r.execute(); });
+                tasks.run([&] { WriteTask w(acc); w.execute(); });


Significant change

This test tests multiple read/write accesses running concurrently. I believe I've ported it to represent that, even though the behaviour is not exactly identical.

Idclip · 2021-06-11T10:45:59Z

I've commented the significant changes that may warrant more review. The rest should be pretty trivial.

Signed-off-by: Nick Avramoussis <4256455+Idclip@users.noreply.github.com>

danrbailey

This is great, thanks Nick!

Signed-off-by: Nick Avramoussis <4256455+Idclip@users.noreply.github.com>

Idclip requested review from apradhana, danrbailey, jmlait and kmuseth April 13, 2021 16:22

Idclip marked this pull request as draft April 13, 2021 16:23

Idclip force-pushed the tbb2021 branch from 506fd80 to 8c65fa1 Compare April 19, 2021 15:49

e4lam added a commit to sideeffects/openvdb_dev that referenced this pull request May 4, 2021

Merge branch 'pr1027'

b44f46f

For details see: AcademySoftwareFoundation#1027

e4lam mentioned this pull request May 5, 2021

Return the thread limit stored in TBB instead of the local _threadLimit value PixarAnimationStudios/OpenUSD#1368

Closed

loqs mentioned this pull request May 18, 2021

[BUILD] Can't build against TBB 2021 AcademySoftwareFoundation/OpenImageIO#2963

Closed

Idclip force-pushed the tbb2021 branch from 8c65fa1 to 76f4e0a Compare June 10, 2021 11:41

Idclip added 4 commits June 10, 2021 13:12

Changes to support TBB 2021

5b0ec82

Signed-off-by: Nick Avramoussis <4256455+Idclip@users.noreply.github.com>

Improved the TBB_INTERFACE switch

b486e5c

Signed-off-by: Nick Avramoussis <4256455+Idclip@users.noreply.github.com>

Updated the maya plugin to work with TBB 2021

21586c1

Signed-off-by: Nick Avramoussis <4256455+Idclip@users.noreply.github.com>

whitespace fix

2acb1a8

Signed-off-by: Nick Avramoussis <4256455+Idclip@users.noreply.github.com>

Idclip force-pushed the tbb2021 branch from 76f4e0a to 2acb1a8 Compare June 10, 2021 12:17

Idclip added 2 commits June 10, 2021 14:43

Added missing include

b9cf5a3

Signed-off-by: Nick Avramoussis <4256455+Idclip@users.noreply.github.com>

Fixed TBB_PREVIEW_GLOBAL_CONTROL define value

a7042bc

Signed-off-by: Nick Avramoussis <4256455+Idclip@users.noreply.github.com>

e4lam reviewed Jun 10, 2021

View reviewed changes

openvdb_houdini/openvdb_houdini/SOP_OpenVDB_Sample_Points.cc Outdated Show resolved Hide resolved

This was linked to issues Jun 10, 2021

[REQUEST] Please support openTBB #932

Closed

[BUILD] Please support tbb new version 2021.1.1 #985

Closed

Idclip linked an issue Jun 10, 2021 that may be closed by this pull request

Incompatible with oneapi 2021.02 tbb. #1095

Closed

Idclip added 3 commits June 10, 2021 20:33

Added a basic TBB 2021 CI test

ce514d9

Signed-off-by: Nick Avramoussis <4256455+Idclip@users.noreply.github.com>

Minor improvement to verbose timing in SOP_OpenVDB_Sample_Points.cc

b7bd85f

Signed-off-by: Nick Avramoussis <4256455+Idclip@users.noreply.github.com>

Moved util/Threading.h to thread/Threading.h and renamed the namespace

58fdc54

Signed-off-by: Nick Avramoussis <4256455+Idclip@users.noreply.github.com>

Idclip marked this pull request as ready for review June 11, 2021 10:38

Idclip commented Jun 11, 2021

View reviewed changes

Idclip changed the title ~~Work in progress to support TBB 2021~~ Added support for TBB 2021 Jun 11, 2021

Added pendingchanges/tbb2021.txt

43cca59

Signed-off-by: Nick Avramoussis <4256455+Idclip@users.noreply.github.com>

danrbailey approved these changes Jun 14, 2021

View reviewed changes

Updated pendingchanges/tbb2021.txt

e20dbf0

Signed-off-by: Nick Avramoussis <4256455+Idclip@users.noreply.github.com>

Idclip merged commit e620aa6 into AcademySoftwareFoundation:master Jun 25, 2021

Idclip deleted the tbb2021 branch June 25, 2021 22:40


		namespace tbb { class split; } // forward declaration

		#include <tbb/blocked_range.h> // for tbb::split

Conversation

Idclip commented Apr 13, 2021

Uh oh!

e4lam commented May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Idclip commented May 12, 2021

Uh oh!

e4lam commented May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Idclip commented May 12, 2021

Uh oh!

e4lam commented May 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Idclip commented May 13, 2021

Uh oh!

e4lam commented May 13, 2021

Uh oh!

Idclip commented Jun 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danrbailey commented Jun 10, 2021

Uh oh!

e4lam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Idclip commented Jun 10, 2021

Uh oh!

Idclip commented Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Idclip Jun 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Idclip commented Jun 11, 2021

Uh oh!

danrbailey left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

e4lam commented May 10, 2021 •

edited

Loading

e4lam commented May 12, 2021 •

edited

Loading

e4lam commented May 13, 2021 •

edited

Loading

Idclip commented Jun 10, 2021 •

edited

Loading

Idclip commented Jun 11, 2021 •

edited

Loading

Idclip Jun 14, 2021 •

edited

Loading