Migrate to modern TBB version by SiarheiFedartsou · Pull Request #6300 · Project-OSRM/osrm-backend

SiarheiFedartsou · 2022-08-08T21:04:40Z

Issue

closes #6297
closes #6181
closes #6087

Tasklist

CHANGELOG.md entry (How to write a changelog entry)
update relevant Wiki pages
add tests (see testing documentation)
review
adjust for comments
cherry pick to release branch

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

SiarheiFedartsou · 2022-08-08T21:07:57Z

.github/workflows/osrm-backend.yml

            CXXFLAGS: "-m32 -msse2 -mfpmath=sse"
            TARGET_ARCH: i686
-
+            ENABLE_CONAN: ON


32-bit build was complaining about some linker errors related to TBB(even though there is 32-bit binary installed and I also tried to build it from source), so it turned out to be easier to simply use Conan instead which seems to work - don't think it is a problem.

CMakeLists.txt

SiarheiFedartsou · 2022-08-08T21:09:37Z

cmake/FindTBB.cmake

@@ -1,286 +1,456 @@
-# Locate Intel Threading Building Blocks include paths and libraries


Took it from here https://github.com/Kitware/VTK/blob/master/CMake/FindTBB.cmake

SiarheiFedartsou · 2022-08-08T21:10:36Z

src/contractor/contractor.cpp


 #include <boost/assert.hpp>
-
-#if TBB_VERSION_MAJOR == 2020


https://oneapi-src.github.io/oneTBB/main/tbb_userguide/Migration_Guide.html

SiarheiFedartsou · 2022-08-08T21:11:27Z

include/util/packed_vector.hpp

    return (word & ~mask) | ((static_cast<WordT>(value) >> offset) & mask);
 }

+inline bool compare_and_swap(uint64_t *ptr, uint64_t old_value, uint64_t new_value)


Tbh I am not sure if we need PackedVector to be thread-safe at all - after brief look I couldn't find a place where we modify it concurrently.

According to #4199 it can happen during edge updates, probably when running osrm-customize

As I can see set_value is used on push_back to - curious if we are safe on reallocations there 🤔

SiarheiFedartsou · 2022-08-08T21:14:32Z

@mjjbell hi, would be great to have your opinion here

SiarheiFedartsou · 2022-08-08T21:14:01Z

CMakeLists.txt


  find_package(BZip2 REQUIRED EXACT ${CONAN_BZIP2_VERSION})
-  find_package(EXPAT REQUIRED EXACT ${CONAN_EXPAT_VERSION})
+  find_package(EXPAT REQUIRED)


SiarheiFedartsou · 2022-08-08T21:59:31Z

I see that Windows build fails. Most likely because it uses old TBB stored on wolt.com. What I am thinking about is may be migrate it to use Conan too - should be probably a bit better approach rather than try to find someone from Wolt to update it now... Another option is to simply install TBB to the system just like I did for Linux/Mac.

SiarheiFedartsou · 2022-08-08T21:59:55Z

I see that Windows build fails. Most likely because it uses old TBB stored on wolt.com. What I am thinking about is may be migrate it to use Conan too - should be probably a bit better approach rather than try to find someone from Wolt to update it now... Another option is to simply install TBB to the system just like I did for Linux/Mac.

Btw worth considering migration to GitHub Actions for Windows CI 🤔

mjjbell · 2022-08-15T21:10:24Z

Agreed, adding a Windows build to Github Actions CI and killing off the old one is probably the direction to take and will make it easier to perform library upgrades.
IIRC the motivation for moving to C++17 was to take advantage of the STL parallel algorithms. Removing TBB completely could solve these problems too, although might require a lot of surgery.

SiarheiFedartsou · 2022-08-16T15:59:34Z

IIRC the motivation for moving to C++17 was to take advantage of the STL parallel algorithms. Removing TBB completely could solve these problems too, although might require a lot of surgery.

Yep, but it turned out that there are a lot of compilers that still do not support STL parallel algorithms from C++17 in 2022. One example is Apple Clang. I haven't dug deep here though, but lets first migrate TBB in order to unblock local builds without Conan - it seems to be easier problem anyway.

SiarheiFedartsou · 2022-08-19T21:10:30Z

src/storage/storage.cpp

+    constexpr bool IS_REQUIRED = true;
+    constexpr bool IS_OPTIONAL = false;


windows.h defines OPTIONAL macro 🤦🏻

SiarheiFedartsou · 2022-08-23T21:14:10Z

docker/Dockerfile

+    apt-get -y --no-install-recommends install ca-certificates cmake make git gcc g++ libbz2-dev libxml2-dev wget \
+    libzip-dev libboost1.74-all-dev lua5.2 liblua5.2-dev -o APT::Install-Suggests=0 -o APT::Install-Recommends=0
+
+RUN NPROC=${BUILD_CONCURRENCY:-$(nproc)} && \


We could download pre-built binaries, but TBB does not provide arm64 binaries at the moment

SiarheiFedartsou · 2022-08-23T21:31:43Z

@mjjbell I think I am done with it, so please take a look when you have a chance

mjjbell

LGTM. Thanks for working though all the required changes 👍

v5.27.0 - Changes from 5.26.0 - API: - ADDED: Add Flatbuffers support to NodeJS bindings. [Project-OSRM#6338](Project-OSRM#6338) - CHANGED: Add `data_version` field to responses of all services. [Project-OSRM#5387](Project-OSRM#5387) - FIXED: Use Boost.Beast to parse HTTP request. [Project-OSRM#6294](Project-OSRM#6294) - FIXED: Fix inefficient osrm-routed connection handling [Project-OSRM#6113](https://gihub.com/Project-OSRM/osrm-backend/pull/6113) - FIXED: Fix HTTP compression precedence [Project-OSRM#6113](Project-OSRM#6113) - NodeJS: - FIXED: Support `skip_waypoints` in Node bindings [Project-OSRM#6060](Project-OSRM#6060) - Misc: - ADDED: conanbuildinfo.json for easy reading of dependencies [Project-OSRM#6388](Project-OSRM#6388) - CHANGED: Improve performance of JSON rendering. Fix undefined behaviour in JSON numbers formatting. [Project-OSRM#6380](Project-OSRM#6380) - ADDED: Add timestamps for logs. [Project-OSRM#6375](Project-OSRM#6375) - CHANGED: Improve performance of map matching via getPathDistance optimization. [Project-OSRM#6378](Project-OSRM#6378) - CHANGED: Optimize RestrictionParser performance. [Project-OSRM#6344](Project-OSRM#6344) - ADDED: Support floats for speed value in traffic updates CSV. [Project-OSRM#6327](Project-OSRM#6327) - CHANGED: Use Lua 5.4 in Docker image. [Project-OSRM#6346](Project-OSRM#6346) - CHANGED: Remove redundant nullptr check. [Project-OSRM#6326](Project-OSRM#6326) - CHANGED: missing files list is included in exception message. [Project-OSRM#5360](Project-OSRM#5360) - CHANGED: Do not use deprecated Callback::Call overload in Node bindings. [Project-OSRM#6318](Project-OSRM#6318) - FIXED: Fix distance calculation consistency. [Project-OSRM#6315](Project-OSRM#6315) - FIXED: Fix performance issue after migration to sol2 3.3.0. [Project-OSRM#6304](Project-OSRM#6304) - CHANGED: Pass osm_node_ids by reference in osrm::updater::Updater class. [Project-OSRM#6298](Project-OSRM#6298) - FIXED: Fix bug with reading Set values from Lua scripts. [Project-OSRM#6285](Project-OSRM#6285) - FIXED: Bug in bicycle profile that caused exceptions if there is a highway=bicycle in the data. [Project-OSRM#6296](Project-OSRM#6296) - FIXED: Internal refactoring of identifier types used in data facade [Project-OSRM#6044](Project-OSRM#6044) - CHANGED: Update docs to reflect recent build and dependency changes [Project-OSRM#6383](Project-OSRM#6383) - Build: - REMOVED: Get rid of Mason. [Project-OSRM#6387](Project-OSRM#6387) - CHANGED: Use clang-format from CI base image. [Project-OSRM#6391](Project-OSRM#6391) - ADDED: Build Node bindings on Windows. [Project-OSRM#6334](Project-OSRM#6334) - ADDED: Configure cross-compilation for Apple Silicon. [Project-OSRM#6360](Project-OSRM#6360) - CHANGED: Use apt-get to install Clang on CI. [Project-OSRM#6345](Project-OSRM#6345) - CHANGED: Fix TBB in case of Conan + NodeJS build. [Project-OSRM#6333](Project-OSRM#6333) - CHANGED: Migrate to modern TBB version. [Project-OSRM#6300](Project-OSRM#6300) - CHANGED: Enable performance-move-const-arg clang-tidy check. [Project-OSRM#6319](Project-OSRM#6319) - CHANGED: Use the latest node on CI. [Project-OSRM#6317](Project-OSRM#6317) - CHANGED: Migrate Windows CI to GitHub Actions. [Project-OSRM#6312](Project-OSRM#6312) - ADDED: Add smoke test for Docker image. [Project-OSRM#6313](Project-OSRM#6313) - CHANGED: Update libosmium to version 2.18.0. [Project-OSRM#6303](Project-OSRM#6303) - CHANGED: Remove EXACT from find_package if using Conan. [Project-OSRM#6299](Project-OSRM#6299) - CHANGED: Configure Undefined Behaviour Sanitizer. [Project-OSRM#6290](Project-OSRM#6290) - CHANGED: Use Conan instead of Mason to install code dependencies. [Project-OSRM#6284](Project-OSRM#6284) - CHANGED: Migrate to C++17. Update sol2 to 3.3.0. [Project-OSRM#6279](Project-OSRM#6279) - CHANGED: Update macOS CI image to macos-11. [Project-OSRM#6286](Project-OSRM#6286) - CHANGED: Enable even more clang-tidy checks. [Project-OSRM#6273](Project-OSRM#6273) - CHANGED: Configure CMake to not build flatbuffers tests and samples. [Project-OSRM#6274](Project-OSRM#6274) - CHANGED: Enable more clang-tidy checks. [Project-OSRM#6270](Project-OSRM#6270) - CHANGED: Configure clang-tidy job on CI. [Project-OSRM#6261](Project-OSRM#6261) - CHANGED: Use Github Actions for building container images [Project-OSRM#6138](Project-OSRM#6138) - CHANGED: Upgrade Boost dependency to 1.70 [Project-OSRM#6113](Project-OSRM#6113) - CHANGED: Upgrade Ubuntu CI builds to 20.04 [Project-OSRM#6119](Project-OSRM#6119) - CHANGED: Make building osrm-routed optional [Project-OSRM#6144](Project-OSRM#6144) - FIXED: Run all unit tests in CI [Project-OSRM#5248](Project-OSRM#5248) - FIXED: Fix installation of Mason CMake and 32 bit CI build [Project-OSRM#6170](Project-OSRM#6170) - FIXED: Fixed Node docs generation check in CI. [Project-OSRM#6058](Project-OSRM#6058) - CHANGED: Docker build, enabled arm64 build layer [Project-OSRM#6172](Project-OSRM#6172) - CHANGED: Docker build, enabled apt-get update/install caching in separate layer for build phase [Project-OSRM#6175](Project-OSRM#6175) - FIXED: Bump CI complete meta job to ubuntu-20.04 [Project-OSRM#6323](Project-OSRM#6323) - CHANGED: Node packages are now scoped by @Project-OSRM [Project-OSRM#6386](Project-OSRM#6386) - Routing: - CHANGED: Lazily generate optional route path data [Project-OSRM#6045](Project-OSRM#6045) - FIXED: Completed support for no_entry and no_exit turn restrictions. [Project-OSRM#5988](Project-OSRM#5988) - ADDED: Add support for non-round-trips with a single fixed endpoint. [Project-OSRM#6050](Project-OSRM#6050) - FIXED: Improvements to maneuver override processing [Project-OSRM#6125](Project-OSRM#6125) - ADDED: Support snapping to multiple ways at an input location. [Project-OSRM#5953](Project-OSRM#5953) - FIXED: Fix snapping target locations to ways used in turn restrictions. [Project-OSRM#6339](Project-OSRM#6339) - ADDED: Support OSM traffic signal directions. [Project-OSRM#6153](Project-OSRM#6153) - FIXED: Ensure u-turn exists in intersection view. [Project-OSRM#6376](Project-OSRM#6376) - FIXED: Gracefully handle no-turn intersections in guidance processing. [Project-OSRM#6382](Project-OSRM#6382) - Profile: - CHANGED: Bicycle surface speeds [Project-OSRM#6212](Project-OSRM#6212) - Tools: - CHANGED: Do not generate intermediate .osrm file in osrm-extract. [Project-OSRM#6354](Project-OSRM#6354)

SiarheiFedartsou commented Aug 8, 2022

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

SiarheiFedartsou commented Aug 8, 2022

View reviewed changes

SiarheiFedartsou force-pushed the sf-tbb3 branch from 67e21ee to 1bf46d6 Compare August 8, 2022 21:13

SiarheiFedartsou marked this pull request as ready for review August 8, 2022 21:14

SiarheiFedartsou commented Aug 8, 2022

View reviewed changes

SiarheiFedartsou mentioned this pull request Aug 16, 2022

Migrate Windows CI to GitHub Actions. #6312

Merged

6 tasks

SiarheiFedartsou force-pushed the sf-tbb3 branch 2 times, most recently from 5579895 to a4de7fc Compare August 19, 2022 19:24

SiarheiFedartsou commented Aug 19, 2022

View reviewed changes

SiarheiFedartsou added 10 commits August 21, 2022 15:12

Migrate to modern TBB version

ed912c5

Migrate to modern TBB version

1d59fdd

Migrate to modern TBB version

1657c7a

Migrate to modern TBB version

d0f7174

Migrate to modern TBB version

a125c48

Migrate to modern TBB version

3925616

Migrate to modern TBB version

57a2293

Migrate to modern TBB version

2b316f0

Migrate to modern TBB version

b27a43d

Migrate to modern TBB version

02cb20e

SiarheiFedartsou force-pushed the sf-tbb3 branch from 4baffaa to 02cb20e Compare August 21, 2022 13:12

SiarheiFedartsou added 2 commits August 21, 2022 15:32

Migrate to modern TBB version

51cfb1e

Migrate to modern TBB version

2c6527a

Migrate to modern TBB version

a64e586

mjjbell mentioned this pull request Aug 23, 2022

5.27 Release? #6325

Closed

7 tasks

SiarheiFedartsou added 2 commits August 23, 2022 22:13

Migrate to modern TBB version

98ec276

Merge branch 'master' into sf-tbb3

d68f66f

SiarheiFedartsou commented Aug 23, 2022

View reviewed changes

SiarheiFedartsou requested a review from mjjbell August 23, 2022 21:31

mjjbell approved these changes Aug 24, 2022

View reviewed changes

SiarheiFedartsou merged commit 9189560 into Project-OSRM:master Aug 24, 2022

vklachkov mentioned this pull request Aug 24, 2022

Build on Arch Linux #6282

Closed

cho-m mentioned this pull request Sep 9, 2022

osrm-backend: use tbb for HEAD build Homebrew/homebrew-core#110153

Closed

6 tasks

datendelphin mentioned this pull request Dec 23, 2022

make it tolerant of TBB 2020 #6493

Closed

mattwigway pushed a commit to mattwigway/osrm-backend that referenced this pull request Jul 20, 2023

Migrate to modern TBB version (Project-OSRM#6300)

8a9d2f2

		@@ -1,286 +1,456 @@
		# Locate Intel Threading Building Blocks include paths and libraries

		constexpr bool IS_REQUIRED = true;
		constexpr bool IS_OPTIONAL = false;

Uh oh!

Conversation

SiarheiFedartsou commented Aug 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Tasklist

Requirements / Relations

Uh oh!

SiarheiFedartsou Aug 8, 2022 • 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!

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!

SiarheiFedartsou commented Aug 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SiarheiFedartsou commented Aug 8, 2022

Uh oh!

SiarheiFedartsou commented Aug 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjjbell commented Aug 15, 2022

Uh oh!

SiarheiFedartsou commented Aug 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SiarheiFedartsou commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjjbell left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SiarheiFedartsou commented Aug 8, 2022 •

edited

Loading

SiarheiFedartsou Aug 8, 2022 •

edited

Loading

SiarheiFedartsou commented Aug 8, 2022 •

edited

Loading

SiarheiFedartsou commented Aug 23, 2022 •

edited

Loading