Replace use of tbb::task with oneapi::tbb::task_group, where available #3146
Conversation
Signed-off-by: Alex Dewar <alex.dewar@gmx.co.uk>
As tbb::task has been removed in oneTBB 2021.01, we need to replace its use with oneapi::tbb::task_group. Define a wrapper so that tbb::task_group is used for newer versions of oneTBB. Fixes gazebosim#2867. Signed-off-by: Alex Dewar <alex.dewar@gmx.co.uk>
|
I forgot to note that I have introduced a small functional change: classes owning a |
There was a problem hiding this comment.
Thanks for your work on this! One requirement that we have for gazebo11 is to preserve API and ABI for the packages already released on 18.04 and 20.04. I think we can do this with #ifdef macros, which hopefully won't be too painful. libtbb-dev has the following versions in these releases:
I think we should aim for keeping the abi-checker CI job happy while being able to change to the tbb dependency in the homebrew formula for gazebo11.
gazebo/transport/TaskGroup.hh
Outdated
| @@ -0,0 +1,84 @@ | |||
| /* | |||
| * Copyright (C) 2021 Alex Dewar | |||
There was a problem hiding this comment.
is it possible to assign copyright to Open Source Robotics Foundation but add your name to the AUTHORS file and in a Changelog entry?
|
It looks like the |
Testing gazebosim/gazebo-classic#3146 Signed-off-by: Steve Peters <scpeters@openrobotics.org>
|
@osrf-jenkins run tests please |
|
I updated our macOS CI to run a job with a very new version of
|
Suggested by @scpeters. Signed-off-by: Alex Dewar <alex.dewar@gmx.co.uk>
Suggested by @scpeters. Signed-off-by: Alex Dewar <alex.dewar@gmx.co.uk>
In any case, this check wasn't actually halting the build in the case that tbb>=2021. Instead, find_package() is used (as it is if tbb.pc isn't present). Signed-off-by: Alex Dewar <alex.dewar@gmx.co.uk>
|
@scpeters Interesting... Could be a |
Signed-off-by: Alex Dewar <alex.dewar@gmx.co.uk>
Done in 49701f4. |
The CI is currently failing and this could be a possible problem.
cmake/SearchForStuff.cmake
Outdated
| set (USE_LEGACY_TBB_TASK OFF) | ||
| endif(${TBB_VERSION} VERSION_LESS 2021) | ||
| endif() | ||
| set(USE_LEGACY_TBB_TASK ${USE_LEGACY_TBB_TASK} CACHE BOOL "Whether to use the deprecated tbb::task class") |
There was a problem hiding this comment.
Nitpick, feel free to ignore: in my experience cache variable vs normal variables is one aspect of CMake that is most confusing for novice users. To improve readability, it may be worth to use a different name for the normal variable name, like USE_LEGACY_TASK_DEFAULT_VALUE.
cmake/SearchForStuff.cmake
Outdated
| endif() | ||
| set(USE_LEGACY_TBB_TASK ${USE_LEGACY_TBB_TASK} CACHE BOOL "Whether to use the deprecated tbb::task class") | ||
| if (USE_LEGACY_TBB_TASK) | ||
| add_definitions(-DUSE_LEGACY_TBB_TASK) |
There was a problem hiding this comment.
This define will be used in public headers, hence it will need to be define also in downstream compilation units. For these reason, it could make sense to use a gazebo-specific name here (like GAZEBO_USE_LEGACY_TBB_TASK), to avoid confusion and the risk of collisions.
cmake/SearchForStuff.cmake
Outdated
| endif() | ||
| set(USE_LEGACY_TBB_TASK ${USE_LEGACY_TBB_TASK} CACHE BOOL "Whether to use the deprecated tbb::task class") | ||
| if (USE_LEGACY_TBB_TASK) | ||
| add_definitions(-DUSE_LEGACY_TBB_TASK) |
There was a problem hiding this comment.
This definition needs to be propagated to downstream compilation units as it is used in public headers, but unfortunately gazebo does not use modern style CMake targets so there is no straightforward way to do so. I can check if there is any facility currently use to expose definitions to downstream projects, like a GAZEBO_DEFINITIONS CMake variable.
There was a problem hiding this comment.
I checked the docs for building downstream projects that use Gazebo (see http://gazebosim.org/tutorials?tut=plugins_hello_world) and there is no GAZEBO_DEFINITIONS or similar. We could try to encode this definition in Gazebo's GAZEBO_CXX_FLAGS, but this is not straightforward. Thinking about it, a possible alternative solution is to remove all the CMake logic related to USE_LEGACY_TBB_TASK, and just define this macro at the C++ preprocessor level, depending on the tbb version. This is the strategy that we used for supporting Ogre 1.12, see bc30cd9 . In this case we would lose the possibility of compiling with tbb task when using tbb <= 2020, but I am not sure this is actually of interest.
There was a problem hiding this comment.
This sounds like a more sensible approach. I'll have a go 😄
Signed-off-by: Alex Dewar <alex.dewar@gmx.co.uk>
As tbb/version.h is seemingly not present before v2021.01, it cannot be used to get the definition for TBB_VERSION_MAJOR, so let's use the main tbb.h header which is present on all versions. Signed-off-by: Alex Dewar <alex.dewar@gmx.co.uk>
|
Arg, so the CI checks are still failing for various reasons. Annoyingly, The Windows build is now complaining about |
|
Thanks a lot @alexdewar for working on this! Note that given the requirement listed by @scpeters on #3146 (review), if it is necessary to preserve the ABI of the library when compiled with tbb < 2021, I guess that we can't:
I guess we need to also put all those changes under the |
|
the work from this branch was merged in #3174 thanks for your contribution! |
|
Nw. Thanks for fixing this 😄 |
As of TBB 2021.01,
tbb::taskhas been removed: #2867.Accordingly, I have implemented a wrapper class,
TaskGroup, which makes use ofoneapi::tbb::task_groupwhere it is available but falls back ontbb::taskotherwise. Note that thetbb::task_groupclass has been around for a while now (predating the rebranding as oneTBB), so we could be less conservative and essentially use this code path for some older versions of TBB which still havetbb::task.Resolves #2867.