From 98a90221ccbaeac1a7a7249e5d61965185d17697 Mon Sep 17 00:00:00 2001 From: acxz <17132214+acxz@users.noreply.github.com> Date: Sat, 11 Jan 2020 12:39:44 -0500 Subject: [PATCH 01/11] remove execution file mode --- gtsam/geometry/Unit3.cpp | 0 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100755 => 100644 gtsam/geometry/Unit3.cpp diff --git a/gtsam/geometry/Unit3.cpp b/gtsam/geometry/Unit3.cpp old mode 100755 new mode 100644 From eb857624839329a9ad0683b2d8d1051480cc6b96 Mon Sep 17 00:00:00 2001 From: acxz <17132214+acxz@users.noreply.github.com> Date: Sat, 11 Jan 2020 13:09:16 -0500 Subject: [PATCH 02/11] change from deprecated tbb::mutex to std::mutex --- gtsam/base/debug.cpp | 16 ++++------------ gtsam/geometry/Unit3.cpp | 4 +--- gtsam/geometry/Unit3.h | 8 ++------ 3 files changed, 7 insertions(+), 21 deletions(-) diff --git a/gtsam/base/debug.cpp b/gtsam/base/debug.cpp index d6d2a4fe0..dbea53c7c 100644 --- a/gtsam/base/debug.cpp +++ b/gtsam/base/debug.cpp @@ -19,31 +19,23 @@ #include #include // for GTSAM_USE_TBB -#ifdef GTSAM_USE_TBB -#include -#endif +#include // std::mutex, std::unique_lock namespace gtsam { GTSAM_EXPORT FastMap > debugFlags; -#ifdef GTSAM_USE_TBB -tbb::mutex debugFlagsMutex; -#endif +std::mutex debugFlagsMutex; /* ************************************************************************* */ bool guardedIsDebug(const std::string& s) { -#ifdef GTSAM_USE_TBB - tbb::mutex::scoped_lock lock(debugFlagsMutex); -#endif + std::unique_lock lock(debugFlagsMutex); return gtsam::debugFlags[s]; } /* ************************************************************************* */ void guardedSetDebug(const std::string& s, const bool v) { -#ifdef GTSAM_USE_TBB - tbb::mutex::scoped_lock lock(debugFlagsMutex); -#endif + std::unique_lock lock(debugFlagsMutex); gtsam::debugFlags[s] = v; } diff --git a/gtsam/geometry/Unit3.cpp b/gtsam/geometry/Unit3.cpp index 533ee500e..655b94fc3 100644 --- a/gtsam/geometry/Unit3.cpp +++ b/gtsam/geometry/Unit3.cpp @@ -80,12 +80,10 @@ static Point3 CalculateBestAxis(const Point3& n) { /* ************************************************************************* */ const Matrix32& Unit3::basis(OptionalJacobian<6, 2> H) const { -#ifdef GTSAM_USE_TBB // NOTE(hayk): At some point it seemed like this reproducably resulted in // deadlock. However, I don't know why and I can no longer reproduce it. // It either was a red herring or there is still a latent bug left to debug. - tbb::mutex::scoped_lock lock(B_mutex_); -#endif + std::unique_lock lock(B_mutex_); const bool cachedBasis = static_cast(B_); const bool cachedJacobian = static_cast(H_B_); diff --git a/gtsam/geometry/Unit3.h b/gtsam/geometry/Unit3.h index 211698806..0a43ce59b 100644 --- a/gtsam/geometry/Unit3.h +++ b/gtsam/geometry/Unit3.h @@ -32,9 +32,7 @@ #include -#ifdef GTSAM_USE_TBB -#include -#endif +#include // std::mutex namespace gtsam { @@ -47,9 +45,7 @@ private: mutable boost::optional B_; ///< Cached basis mutable boost::optional H_B_; ///< Cached basis derivative -#ifdef GTSAM_USE_TBB - mutable tbb::mutex B_mutex_; ///< Mutex to protect the cached basis. -#endif + mutable std::mutex B_mutex_; ///< Mutex to protect the cached basis. public: From b2e93311610a38c34c81c4e4ed80b20ae9c661bc Mon Sep 17 00:00:00 2001 From: acxz <17132214+acxz@users.noreply.github.com> Date: Sat, 11 Jan 2020 14:04:40 -0500 Subject: [PATCH 03/11] remove unnecessary tbb task_scheduler_init --- gtsam/base/types.h | 1 - 1 file changed, 1 deletion(-) diff --git a/gtsam/base/types.h b/gtsam/base/types.h index 3b6c9f337..bf3ed1bc3 100644 --- a/gtsam/base/types.h +++ b/gtsam/base/types.h @@ -28,7 +28,6 @@ #include #ifdef GTSAM_USE_TBB -#include #include #include #endif From 1483df7aa000f8416097bab99e317be8323ff43a Mon Sep 17 00:00:00 2001 From: acxz <17132214+acxz@users.noreply.github.com> Date: Sat, 11 Jan 2020 15:11:50 -0500 Subject: [PATCH 04/11] add TBB guards back for multithread option and potentially breaking changes --- gtsam/base/debug.cpp | 8 ++++++++ gtsam/geometry/Unit3.cpp | 2 ++ gtsam/geometry/Unit3.h | 4 ++++ 3 files changed, 14 insertions(+) diff --git a/gtsam/base/debug.cpp b/gtsam/base/debug.cpp index dbea53c7c..592b42d15 100644 --- a/gtsam/base/debug.cpp +++ b/gtsam/base/debug.cpp @@ -19,23 +19,31 @@ #include #include // for GTSAM_USE_TBB +#ifdef GTSAM_USE_TBB #include // std::mutex, std::unique_lock +#endif namespace gtsam { GTSAM_EXPORT FastMap > debugFlags; +#ifdef GTSAM_USE_TBB std::mutex debugFlagsMutex; +#endif /* ************************************************************************* */ bool guardedIsDebug(const std::string& s) { +#ifdef GTSAM_USE_TBB std::unique_lock lock(debugFlagsMutex); +#endif return gtsam::debugFlags[s]; } /* ************************************************************************* */ void guardedSetDebug(const std::string& s, const bool v) { +#ifdef GTSAM_USE_TBB std::unique_lock lock(debugFlagsMutex); +#endif gtsam::debugFlags[s] = v; } diff --git a/gtsam/geometry/Unit3.cpp b/gtsam/geometry/Unit3.cpp index 655b94fc3..f33e08a83 100644 --- a/gtsam/geometry/Unit3.cpp +++ b/gtsam/geometry/Unit3.cpp @@ -80,10 +80,12 @@ static Point3 CalculateBestAxis(const Point3& n) { /* ************************************************************************* */ const Matrix32& Unit3::basis(OptionalJacobian<6, 2> H) const { +#ifdef GTSAM_USE_TBB // NOTE(hayk): At some point it seemed like this reproducably resulted in // deadlock. However, I don't know why and I can no longer reproduce it. // It either was a red herring or there is still a latent bug left to debug. std::unique_lock lock(B_mutex_); +#endif const bool cachedBasis = static_cast(B_); const bool cachedJacobian = static_cast(H_B_); diff --git a/gtsam/geometry/Unit3.h b/gtsam/geometry/Unit3.h index 0a43ce59b..a4043be85 100644 --- a/gtsam/geometry/Unit3.h +++ b/gtsam/geometry/Unit3.h @@ -32,7 +32,9 @@ #include +#ifdef GTSAM_USE_TBB #include // std::mutex +#endif namespace gtsam { @@ -45,7 +47,9 @@ private: mutable boost::optional B_; ///< Cached basis mutable boost::optional H_B_; ///< Cached basis derivative +#ifdef GTSAM_USE_TBB mutable std::mutex B_mutex_; ///< Mutex to protect the cached basis. +#endif public: From af5d22248b6219a46ca06f35fe1aa0f3c8e15ea4 Mon Sep 17 00:00:00 2001 From: acxz <17132214+acxz@users.noreply.github.com> Date: Sat, 11 Jan 2020 16:37:13 -0500 Subject: [PATCH 05/11] remove deprecated tbb_exception in favor of std_exception --- gtsam/base/ThreadsafeException.h | 46 ++++---------------------------- gtsam/base/types.h | 3 ++- 2 files changed, 7 insertions(+), 42 deletions(-) diff --git a/gtsam/base/ThreadsafeException.h b/gtsam/base/ThreadsafeException.h index 15a76fbf1..0f7c6131d 100644 --- a/gtsam/base/ThreadsafeException.h +++ b/gtsam/base/ThreadsafeException.h @@ -11,7 +11,7 @@ /** * @file ThreadSafeException.h - * @brief Base exception type that uses tbb_exception if GTSAM is compiled with TBB + * @brief Base exception type that uses tbb_allocator if GTSAM is compiled with TBB * @author Richard Roberts * @date Aug 21, 2010 * @addtogroup base @@ -25,34 +25,28 @@ #include #include #include +#include #ifdef GTSAM_USE_TBB #include -#include #include #include #endif namespace gtsam { -/// Base exception type that uses tbb_exception if GTSAM is compiled with TBB. +/// Base exception type that uses tbb_allocator if GTSAM is compiled with TBB. template class ThreadsafeException: -#ifdef GTSAM_USE_TBB - public tbb::tbb_exception -#else public std::exception -#endif { -#ifdef GTSAM_USE_TBB private: - typedef tbb::tbb_exception Base; + typedef std::exception Base; +#ifdef GTSAM_USE_TBB protected: typedef std::basic_string, tbb::tbb_allocator > String; #else -private: - typedef std::exception Base; protected: typedef std::string String; #endif @@ -82,36 +76,6 @@ protected: } public: - // Implement functions for tbb_exception -#ifdef GTSAM_USE_TBB - virtual tbb::tbb_exception* move() throw () { - void* cloneMemory = scalable_malloc(sizeof(DERIVED)); - if (!cloneMemory) { - std::cerr << "Failed allocating memory to copy thrown exception, exiting now." << std::endl; - exit(-1); - } - DERIVED* clone = ::new(cloneMemory) DERIVED(static_cast(*this)); - clone->dynamic_ = true; - return clone; - } - - virtual void destroy() throw () { - if (dynamic_) { - DERIVED* derivedPtr = static_cast(this); - derivedPtr->~DERIVED(); - scalable_free(derivedPtr); - } - } - - virtual void throw_self() { - throw *static_cast(this); - } - - virtual const char* name() const throw () { - return typeid(DERIVED).name(); - } -#endif - virtual const char* what() const throw () { return description_ ? description_->c_str() : ""; } diff --git a/gtsam/base/types.h b/gtsam/base/types.h index bf3ed1bc3..d12a4209a 100644 --- a/gtsam/base/types.h +++ b/gtsam/base/types.h @@ -27,8 +27,9 @@ #include #include +#include + #ifdef GTSAM_USE_TBB -#include #include #endif From aff24bd77b196c6e218dc716351741efbcf3c401 Mon Sep 17 00:00:00 2001 From: acxz <17132214+acxz@users.noreply.github.com> Date: Sat, 11 Jan 2020 18:11:59 -0500 Subject: [PATCH 06/11] remove tbb.h include and specify individual includes needed remove deprecated tbb::task_scheduler_init --- examples/SolverComparer.cpp | 8 -------- examples/TimeTBB.cpp | 8 ++++---- gtsam/base/treeTraversal/parallelTraversalTasks.h | 7 ++----- 3 files changed, 6 insertions(+), 17 deletions(-) diff --git a/examples/SolverComparer.cpp b/examples/SolverComparer.cpp index 80ac08e03..528a441b3 100644 --- a/examples/SolverComparer.cpp +++ b/examples/SolverComparer.cpp @@ -57,12 +57,6 @@ #include #include -#ifdef GTSAM_USE_TBB -#include -#undef max // TBB seems to include windows.h and we don't want these macros -#undef min -#endif - using namespace std; using namespace gtsam; using namespace gtsam::symbol_shorthand; @@ -206,10 +200,8 @@ int main(int argc, char *argv[]) { } #ifdef GTSAM_USE_TBB - std::unique_ptr init; if(nThreads > 0) { cout << "Using " << nThreads << " threads" << endl; - init.reset(new tbb::task_scheduler_init(nThreads)); } else cout << "Using threads for all processors" << endl; #else diff --git a/examples/TimeTBB.cpp b/examples/TimeTBB.cpp index 178fa513f..58da3d67b 100644 --- a/examples/TimeTBB.cpp +++ b/examples/TimeTBB.cpp @@ -28,9 +28,10 @@ using boost::assign::list_of; #ifdef GTSAM_USE_TBB -#include -#undef max // TBB seems to include windows.h and we don't want these macros -#undef min +#include // tbb::blocked_range +#include // tbb::tick_count +#include // tbb::parallel_for +#include // tbb::cache_aligned_allocator static const DenseIndex numberOfProblems = 1000000; static const DenseIndex problemSize = 4; @@ -153,7 +154,6 @@ int main(int argc, char* argv[]) for(size_t n: numThreads) { cout << "With " << n << " threads:" << endl; - tbb::task_scheduler_init init((int)n); results[(int)n].grainSizesWithoutAllocation = testWithoutMemoryAllocation(); results[(int)n].grainSizesWithAllocation = testWithMemoryAllocation(); cout << endl; diff --git a/gtsam/base/treeTraversal/parallelTraversalTasks.h b/gtsam/base/treeTraversal/parallelTraversalTasks.h index 9b2dae3d0..3dd6e307f 100644 --- a/gtsam/base/treeTraversal/parallelTraversalTasks.h +++ b/gtsam/base/treeTraversal/parallelTraversalTasks.h @@ -22,11 +22,8 @@ #include #ifdef GTSAM_USE_TBB -# include -# include -# undef max // TBB seems to include windows.h and we don't want these macros -# undef min -# undef ERROR +#include // tbb::task, tbb::task_list +#include // tbb::scalable_allocator namespace gtsam { From bfc32e9f69d8b06f2533661ee71bfd04d3196c73 Mon Sep 17 00:00:00 2001 From: acxz <17132214+acxz@users.noreply.github.com> Date: Mon, 13 Jan 2020 07:29:41 -0500 Subject: [PATCH 07/11] add deprecated task_scheduler_init until alternative is found --- examples/TimeTBB.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/examples/TimeTBB.cpp b/examples/TimeTBB.cpp index 58da3d67b..09a02faaa 100644 --- a/examples/TimeTBB.cpp +++ b/examples/TimeTBB.cpp @@ -32,6 +32,7 @@ using boost::assign::list_of; #include // tbb::tick_count #include // tbb::parallel_for #include // tbb::cache_aligned_allocator +#include // tbb::task_scheduler_init static const DenseIndex numberOfProblems = 1000000; static const DenseIndex problemSize = 4; @@ -154,6 +155,7 @@ int main(int argc, char* argv[]) for(size_t n: numThreads) { cout << "With " << n << " threads:" << endl; + tbb::task_scheduler_init init((int)n); results[(int)n].grainSizesWithoutAllocation = testWithoutMemoryAllocation(); results[(int)n].grainSizesWithAllocation = testWithMemoryAllocation(); cout << endl; From 8096b0e251c88b3df18eb9ec840f773376a05fec Mon Sep 17 00:00:00 2001 From: acxz <17132214+acxz@users.noreply.github.com> Date: Mon, 16 Mar 2020 00:49:17 -0400 Subject: [PATCH 08/11] add deprecated task_scheduler_init unitl alternative is found --- examples/SolverComparer.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/examples/SolverComparer.cpp b/examples/SolverComparer.cpp index 528a441b3..d9dce7181 100644 --- a/examples/SolverComparer.cpp +++ b/examples/SolverComparer.cpp @@ -57,6 +57,10 @@ #include #include +#ifdef GTSAM_USE_TBB +#include // tbb::task_scheduler_init +#endif + using namespace std; using namespace gtsam; using namespace gtsam::symbol_shorthand; @@ -200,8 +204,10 @@ int main(int argc, char *argv[]) { } #ifdef GTSAM_USE_TBB + std::unique_ptr init; if(nThreads > 0) { cout << "Using " << nThreads << " threads" << endl; + init.reset(new tbb::task_scheduler_init(nThreads)); } else cout << "Using threads for all processors" << endl; #else From b4b487695d6fe64a983097c921cda88b6d2f77bb Mon Sep 17 00:00:00 2001 From: acxz <17132214+acxz@users.noreply.github.com> Date: Sun, 12 Apr 2020 12:48:25 -0400 Subject: [PATCH 09/11] replace task_scheduler_init with task arena/group --- examples/TimeTBB.cpp | 40 ++++++++++++++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 8 deletions(-) diff --git a/examples/TimeTBB.cpp b/examples/TimeTBB.cpp index 09a02faaa..c25a19107 100644 --- a/examples/TimeTBB.cpp +++ b/examples/TimeTBB.cpp @@ -32,7 +32,8 @@ using boost::assign::list_of; #include // tbb::tick_count #include // tbb::parallel_for #include // tbb::cache_aligned_allocator -#include // tbb::task_scheduler_init +#include // tbb::task_arena +#include // tbb::task_group static const DenseIndex numberOfProblems = 1000000; static const DenseIndex problemSize = 4; @@ -69,10 +70,14 @@ struct WorkerWithoutAllocation }; /* ************************************************************************* */ -map testWithoutMemoryAllocation() +map testWithoutMemoryAllocation(int num_threads) { // A function to do some matrix operations without allocating any memory + // Create task_arena and task_group + tbb::task_arena arena(num_threads); + tbb::task_group tg; + // Now call it vector results(numberOfProblems); @@ -81,7 +86,15 @@ map testWithoutMemoryAllocation() for(size_t grainSize: grainSizes) { tbb::tick_count t0 = tbb::tick_count::now(); - tbb::parallel_for(tbb::blocked_range(0, numberOfProblems), WorkerWithoutAllocation(results)); + + // Run parallel code (as a task group) inside of task arena + arena.execute([&]{ + tg.run([&]{ + tbb::parallel_for(tbb::blocked_range(0, numberOfProblems), WorkerWithoutAllocation(results)); + }); + tg.wait(); + }); + tbb::tick_count t1 = tbb::tick_count::now(); cout << "Without memory allocation, grain size = " << grainSize << ", time = " << (t1 - t0).seconds() << endl; timingResults[(int)grainSize] = (t1 - t0).seconds(); @@ -122,10 +135,14 @@ struct WorkerWithAllocation }; /* ************************************************************************* */ -map testWithMemoryAllocation() +map testWithMemoryAllocation(int num_threads) { // A function to do some matrix operations with allocating memory + // Create task_arena and task_group + tbb::task_arena arena(num_threads); + tbb::task_group tg; + // Now call it vector results(numberOfProblems); @@ -134,7 +151,15 @@ map testWithMemoryAllocation() for(size_t grainSize: grainSizes) { tbb::tick_count t0 = tbb::tick_count::now(); - tbb::parallel_for(tbb::blocked_range(0, numberOfProblems), WorkerWithAllocation(results)); + + // Run parallel code (as a task group) inside of task arena + arena.execute([&]{ + tg.run([&]{ + tbb::parallel_for(tbb::blocked_range(0, numberOfProblems), WorkerWithAllocation(results)); + }); + tg.wait(); + }); + tbb::tick_count t1 = tbb::tick_count::now(); cout << "With memory allocation, grain size = " << grainSize << ", time = " << (t1 - t0).seconds() << endl; timingResults[(int)grainSize] = (t1 - t0).seconds(); @@ -155,9 +180,8 @@ int main(int argc, char* argv[]) for(size_t n: numThreads) { cout << "With " << n << " threads:" << endl; - tbb::task_scheduler_init init((int)n); - results[(int)n].grainSizesWithoutAllocation = testWithoutMemoryAllocation(); - results[(int)n].grainSizesWithAllocation = testWithMemoryAllocation(); + results[(int)n].grainSizesWithoutAllocation = testWithoutMemoryAllocation((int)n); + results[(int)n].grainSizesWithAllocation = testWithMemoryAllocation((int)n); cout << endl; } From df9cf86cb00a8915e8b0c0087b78cf0bf8b4a5fe Mon Sep 17 00:00:00 2001 From: acxz <17132214+acxz@users.noreply.github.com> Date: Mon, 13 Apr 2020 11:30:57 -0400 Subject: [PATCH 10/11] replace task_scheduler_init with task arena/group --- examples/SolverComparer.cpp | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/examples/SolverComparer.cpp b/examples/SolverComparer.cpp index d9dce7181..cc5fdb26d 100644 --- a/examples/SolverComparer.cpp +++ b/examples/SolverComparer.cpp @@ -58,7 +58,8 @@ #include #ifdef GTSAM_USE_TBB -#include // tbb::task_scheduler_init +#include // tbb::task_arena +#include // tbb::task_group #endif using namespace std; @@ -204,10 +205,11 @@ int main(int argc, char *argv[]) { } #ifdef GTSAM_USE_TBB - std::unique_ptr init; + tbb::task_arena arena; + tbb::task_group tg; if(nThreads > 0) { cout << "Using " << nThreads << " threads" << endl; - init.reset(new tbb::task_scheduler_init(nThreads)); + arena.initialize(nThreads); } else cout << "Using threads for all processors" << endl; #else @@ -217,6 +219,10 @@ int main(int argc, char *argv[]) { } #endif +#ifdef GTSAM_USE_TBB + arena.execute([&]{ + tg.run([&]{ +#endif // Run mode if(incremental) runIncremental(); @@ -228,6 +234,10 @@ int main(int argc, char *argv[]) { runPerturb(); else if(stats) runStats(); +#ifdef GTSAM_USE_TBB + }); + }); +#endif return 0; } From e0cbc76456d5729b8b0a563595bc609535d6fcad Mon Sep 17 00:00:00 2001 From: acxz <17132214+acxz@users.noreply.github.com> Date: Wed, 6 May 2020 16:47:16 -0400 Subject: [PATCH 11/11] change to more efficient call of threaded functions --- examples/SolverComparer.cpp | 2 +- examples/TimeTBB.cpp | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/examples/SolverComparer.cpp b/examples/SolverComparer.cpp index cc5fdb26d..0845b42ae 100644 --- a/examples/SolverComparer.cpp +++ b/examples/SolverComparer.cpp @@ -221,7 +221,7 @@ int main(int argc, char *argv[]) { #ifdef GTSAM_USE_TBB arena.execute([&]{ - tg.run([&]{ + tg.run_and_wait([&]{ #endif // Run mode if(incremental) diff --git a/examples/TimeTBB.cpp b/examples/TimeTBB.cpp index c25a19107..4bc5144f4 100644 --- a/examples/TimeTBB.cpp +++ b/examples/TimeTBB.cpp @@ -89,10 +89,9 @@ map testWithoutMemoryAllocation(int num_threads) // Run parallel code (as a task group) inside of task arena arena.execute([&]{ - tg.run([&]{ + tg.run_and_wait([&]{ tbb::parallel_for(tbb::blocked_range(0, numberOfProblems), WorkerWithoutAllocation(results)); }); - tg.wait(); }); tbb::tick_count t1 = tbb::tick_count::now(); @@ -154,10 +153,9 @@ map testWithMemoryAllocation(int num_threads) // Run parallel code (as a task group) inside of task arena arena.execute([&]{ - tg.run([&]{ + tg.run_and_wait([&]{ tbb::parallel_for(tbb::blocked_range(0, numberOfProblems), WorkerWithAllocation(results)); }); - tg.wait(); }); tbb::tick_count t1 = tbb::tick_count::now();