-
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
Conversation
gtsam/geometry/Unit3.cpp
Outdated
@@ -80,12 +80,10 @@ static Point3 CalculateBestAxis(const Point3& n) { | |||
|
|||
/* ************************************************************************* */ | |||
const Matrix32& Unit3::basis(OptionalJacobian<6, 2> H) const { | |||
#ifdef GTSAM_USE_TBB |
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.
I would consider leaving these #ifdef GTSAM_USE_TBB
guards in place. They have the benefit of making single-threaded builds (without TBB) completely mutex-free.
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.
hmm, I see, yeah that would be useful. But I don't like keeping the TBB name since it is not a TBB function/class. Maybe renaming the guard to GTSAM_MULTITHREADED
would be more accurate?
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.
I agree it would be more accurate, but let's keep that change until TBB is not the only way to do multi-threading. A change like this has the potential to break some setups of users in ways we can't anticipate. Better to do this as part of a version bump with release docs - I propose this (awesome!) PR should simply addresses the warnings.
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.
Okay I'll add the guards back, but I really hope we don't forget. Maybe we can keep an issue and PR open specifically for this until we decide to add it in.
In modern TBB we should be able to use plain |
Do you mean modern C++? But what is modern C++? C++11,14, or 17? |
gtsam/geometry/Unit3.cpp
Outdated
@@ -80,12 +80,10 @@ static Point3 CalculateBestAxis(const Point3& n) { | |||
|
|||
/* ************************************************************************* */ | |||
const Matrix32& Unit3::basis(OptionalJacobian<6, 2> H) const { | |||
#ifdef GTSAM_USE_TBB |
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.
I agree it would be more accurate, but let's keep that change until TBB is not the only way to do multi-threading. A change like this has the potential to break some setups of users in ways we can't anticipate. Better to do this as part of a version bump with release docs - I propose this (awesome!) PR should simply addresses the warnings.
PS, is tbb::exception giving warnings? If not I guess it's a non-issue. If so, we have to figure it out... |
It is, basically the |
TBB uses |
Ahh I understand. Thats p cool. Okay then, I'll try to remove |
I have gotten rid of the tbb_exception warnings. The biggest change was instead of using tbb_exception we are using std_exception, but still using tbb's allocator if GTSAM_USE_TBB is true. Note not all the warnings are removed yet. Still getting something like this:
Haven't dug into this one, but it's weird that it doesn't explicitly state what functionality in |
remove deprecated tbb::task_scheduler_init
Fixed the |
It appears TBB is not currently in the Travis builds? |
You're right, just created a PR to add tbb into the Travis builds. |
OK, let’s wait until that PR passes and then merge that branch in here. That will then be proof things work as expected - provided we have enough coverage, which is unclear. |
Sounds appropriate. |
@@ -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); |
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you say that? The task_scheduler_init
was optional in the first place. If we don't provide one, TBB uses one internally. We are still timing how fast the parallelization occurs. From what I can tell the functionality of the script has not changed one bit.
Suppressing the deprecation warning is just hiding the problem. In future releases this will become an error, instead of a warning.
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.
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 comment
The 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.
Removing tbb::task_scheduler_init init((int)n);
causes number of threads to be equal to number of cores for all cases, rendering the for loops in this test redundant and pointless.
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.
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 comment
The 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 task_scheduler_init
Some quotes from it:
An optional parameter to the constructor and method initialize allows you to specify the number of threads to be used for task execution. This parameter is useful for scaling studies during development, but should not be set for production use.
Tip
The reason for not specifying the number of threads in production code is that in a large software project, there is no way for various components to know how many threads would be optimal for other threads. Hardware threads are a shared global resource. It is best to leave the decision of how many threads to use to the task scheduler.
To minimize time overhead, it is best to rely upon automatic creation of the task scheduler, or create a single task_scheduler_init object whose activation spans all uses of the library's task scheduler. A task_scheduler_init is not assignable or copy-constructible.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we can use task_arena
instead. (https://software.intel.com/en-us/node/506359)
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.
I'm not sure on how to replicate this script without task_scheduler_init
so I am going to leave this script be, hopefully it will get tackled before the feature gets removed.
if(nThreads > 0) { | ||
cout << "Using " << nThreads << " threads" << endl; | ||
init.reset(new tbb::task_scheduler_init(nThreads)); |
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 turns nThreads
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.
Ping. Seems like tests are passing. Should we merge? |
I think |
We got TBB into the travis build! 🎉 Basically, this PR is NOT ready to merge. Here's the section in the TBB reference manual |
We can just use #208 which this PR attempts to resolve. Is there anything specific you would like me to do? For example explain the changes behind each changed line in this PR, and point to the respective TBB documentation? |
Ah I missed the link in your PR description, thank you. I think this PR needs a concrete plan of attack, something which we don't have the time for right now, unfortunately, since this is a large engineering task due to the various things involved. Maintaining backwards compatibility is the number one priority (unless we do a version bump). I am going to read up a bit more on how to fix all the deprecations using C++11 and then we can go from there. In the meantime, if you wish to suppress the warnings, you can just add this line to the
|
Sounds good. Just let me know wherever I can help, I would love to get rid of the warnings and deprecated features. |
Oh also one more thing, can you force a rebuild for the Travis CI, since building with tbb is now included in the develop branch. (https://stackoverflow.com/questions/17606874/trigger-a-travis-ci-rebuild-without-pushing-a-commit) |
@acxz already did. |
I took a look at the changes and it aligned with the TBB 2020 manual. So I think after the Actions CI is merged we can also get TBB going by CI-ing |
Thank you for reviewing my changes! Sounds good. I think it would be a good idea to target the April 23rd since that is when the next Ubuntu LTS release is and that LTS will ship with TBB 2020. |
I found some time to replace I believe this removes all deprecation warnings up to TBB 2020.1 which will land in Ubuntu 20.04. There is still some more deprecations warnings to tackle for TBB 2020.2 so I will leave the partial in the PR name. |
examples/SolverComparer.cpp
Outdated
@@ -228,6 +234,10 @@ int main(int argc, char *argv[]) { | |||
runPerturb(); | |||
else if(stats) | |||
runStats(); | |||
#ifdef gtsam_use_tbb |
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.
Should be upper cased?
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.
yikes, thx for catching that. Fixed it up.
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.
I am surprised that it didn't trigger CI fails (or I missed it?)
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.
No it passed, im p sure preprocessor directives can be lowercase, just highly frowned upon since it can much more easily conflict with a variable name.
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.
I mean in your previous file I see two types of macros, one in upper case and one in smaller case.
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.
That is not triggering a CI fail because by default the smaller case is not defined, HOWEVER this means that our test suite is not catching that that piece of code is missing.
@varunagrawal have you gone through this PR on a deeper level to make more comments/suggestions? |
@ProfFan I see that the Github Actions CI PR has been merged in, can we test this PR against it? |
@varunagrawal I don't know what you want here, either :-) |
This is now an old PR and I'd love to merge it. To get things going, I compiled on an Ubuntu machine and had zero TBB-related warnings. Yay! However, when I tried to run SolverComparer, I had an issue. This is an old script, so I just got the input arguments from the comments in the file, but got:
|
Yikes, don't know how that slipped by me. I guess I forgot to add I can fix it up later this afternoon. Thx for catching that @dellaert. Fixed it up and changed the separate run/wait commands into the combined SolverComparer does not segfault for me now. |
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.
Seems to work now :-) I'll merge.
Some timing work might be needed to figure out how we can make TBB really help.
Sorry I didn't get a chance to look at this. I trust everything works, and if not, we can open a regression bug. |
This PR addresses #208.
tbb::mutex
is replaced withstd::mutex
tbb::task_scheduler_init
is removed since it is deprecated and not used in the codebase. For tests it is still kept since it adds functionality, specifically specifying the number of threads. It needs to be replaced withtbb:task_arena
in those cases. See discussion below for more details.tbb::exception
is slightly harder to get rid of, mostly because I am not sure if the C++11std::exception
has the thread synchronous properties thattbb::exception
has. Specifically ifstd::exceptions
can be propagated across threads.exception_ptr
may be the likely substitute fortbb::exception
Based on discussion below, it seems
tbb::exception
s do so it is fine.tbb/task.h
is also deprecated but this PR does not address it, hence the partial in the name.Useful TBB deprecation information
Note based on this
tbb::exception
s are properly propagated. Consider looking at tbb::exception changes in this PR.These changes require C++11 which should be fine for the rest of the codebase.
This change is