-
Notifications
You must be signed in to change notification settings - Fork 18
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
Named threads for better debugging #98
Conversation
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.
Thank you! Sane overall, but get_thread_name
is only used in tests. Maybe it can be part of runtime_tests.cpp
instead (precedence SetProcessAffinityMask
).
Yes, there is precedence, but I don't think it's good precedence. I think having the setters and getters in the same place makes more sense, it also follows the principle of least surprise. Sure, they're only used for testing, but I don't see any benefit from splitting up this semantic grouping of code. |
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.
Thanks, LGTM now.
Yes, there is precedence, but I don't think it's good precedence. I think having the setters and getters in the same place makes more sense, it also follows the principle of least surprise.
Im fine with that.
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.
Thanks, this is great! After trying it out I would wish however that all Celerity threads would be clearly distinguishable from other threads (for example I have several CUDA threads and some other unknown ones, presumably MPI). CUDA prefixes its threads with cuda-
, so maybe we could do celerity-
, or if that's too long, something like clty
or c6y-
?
81a0baa
to
a27f1c9
Compare
This PR is based on #96, so that one should be merged first.
This PR assigns names to all threads used by celerity for better and easier debugging. Because the
pthread
API limits thread names to only 15 characters, the names were kept relatively short. Especially the host task worker threads are only namedworkerN
(whereN
is the thread number inside the thread pool), even though they could also include the MPI communicator name, as celerity uses a thread pool per communicator. This means that there will be worker threads belonging to different communicators, but sharing the same name.