-
Notifications
You must be signed in to change notification settings - Fork 764
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TBB deprecation fix (partial) #209
Changes from all commits
98a9022
eb85762
b2e9331
1483df7
af5d222
aff24bd
bfc32e9
8096b0e
b4b4876
df9cf86
e0cbc76
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,9 +28,12 @@ using boost::assign::list_of; | |
|
||
#ifdef GTSAM_USE_TBB | ||
|
||
#include <tbb/tbb.h> | ||
#undef max // TBB seems to include windows.h and we don't want these macros | ||
#undef min | ||
#include <tbb/blocked_range.h> // tbb::blocked_range | ||
#include <tbb/tick_count.h> // tbb::tick_count | ||
#include <tbb/parallel_for.h> // tbb::parallel_for | ||
#include <tbb/cache_aligned_allocator.h> // tbb::cache_aligned_allocator | ||
#include <tbb/task_arena.h> // tbb::task_arena | ||
#include <tbb/task_group.h> // tbb::task_group | ||
|
||
static const DenseIndex numberOfProblems = 1000000; | ||
static const DenseIndex problemSize = 4; | ||
|
@@ -67,10 +70,14 @@ struct WorkerWithoutAllocation | |
}; | ||
|
||
/* ************************************************************************* */ | ||
map<int, double> testWithoutMemoryAllocation() | ||
map<int, double> 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<double> results(numberOfProblems); | ||
|
||
|
@@ -79,7 +86,14 @@ map<int, double> testWithoutMemoryAllocation() | |
for(size_t grainSize: grainSizes) | ||
{ | ||
tbb::tick_count t0 = tbb::tick_count::now(); | ||
tbb::parallel_for(tbb::blocked_range<size_t>(0, numberOfProblems), WorkerWithoutAllocation(results)); | ||
|
||
// Run parallel code (as a task group) inside of task arena | ||
arena.execute([&]{ | ||
tg.run_and_wait([&]{ | ||
tbb::parallel_for(tbb::blocked_range<size_t>(0, numberOfProblems), WorkerWithoutAllocation(results)); | ||
}); | ||
}); | ||
|
||
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(); | ||
|
@@ -120,10 +134,14 @@ struct WorkerWithAllocation | |
}; | ||
|
||
/* ************************************************************************* */ | ||
map<int, double> testWithMemoryAllocation() | ||
map<int, double> 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<double> results(numberOfProblems); | ||
|
||
|
@@ -132,7 +150,14 @@ map<int, double> testWithMemoryAllocation() | |
for(size_t grainSize: grainSizes) | ||
{ | ||
tbb::tick_count t0 = tbb::tick_count::now(); | ||
tbb::parallel_for(tbb::blocked_range<size_t>(0, numberOfProblems), WorkerWithAllocation(results)); | ||
|
||
// Run parallel code (as a task group) inside of task arena | ||
arena.execute([&]{ | ||
tg.run_and_wait([&]{ | ||
tbb::parallel_for(tbb::blocked_range<size_t>(0, numberOfProblems), WorkerWithAllocation(results)); | ||
}); | ||
}); | ||
|
||
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(); | ||
|
@@ -153,9 +178,8 @@ int main(int argc, char* argv[]) | |
for(size_t n: numThreads) | ||
{ | ||
cout << "With " << n << " threads:" << endl; | ||
tbb::task_scheduler_init init((int)n); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without this line this entire timing script is pointless. Is there not a replacement for this deprecated function? Otherwise, could we just suppress the deprecation warning? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you say that? The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do understand that it may be hard to support both TBB 4.4 and TBB 2020 and if that is the case, we can just add a requirement during the installation procedure that warns the use of TBB > 2020. Although I think we can get GTSAM to support both versions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. Read the for loop more carefully. The purpose of this script is to time the performance of TBB with different grain sizes, and this default grain size varies with number of threads. In order to read this default grain size, this loop initializes tbb with 1, 4, 8 threads, and reads the grain size. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh I see now, yes we are specifying the number of threads each iteration of the for loop. Hmmm, well this is a predicament. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me link the TBB documentation: https://software.intel.com/en-us/node/506296 regarding Some quotes from it:
But I mean this is just saying don't use it in production. Basically we want a feature to specify the number of threads we create. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe we can use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure on how to replicate this script without |
||
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; | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. This line was important to the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just because we do not explicitly create a
task_scheduler_init
does not mean, TBB or C++11 is not using its internal one. n threads are still created and the test is run in parallel if TBB is being used, although not needed now, since C++11 has concurrency.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we always wanted max number of threads (i.e. threads = number of cores) that might be ok, but
nThreads
is an optional parameter to this program, so that the user can test timing for a specific number of threads. As I see it, removing this line effectively turnsnThreads
into an on/off switch.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, thats right. Thx for the thorough review! I'll add a commit that adds this deprecated feature for now until we figure out how to handle it in the future.