Skip to content
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

Merged
merged 15 commits into from
Mar 8, 2022
Merged

Named threads for better debugging #98

merged 15 commits into from
Mar 8, 2022

Conversation

BlackMark29A
Copy link

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 named workerN (where N 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.

Copy link
Contributor

@fknorr fknorr left a 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).

include/named_threads.h Outdated Show resolved Hide resolved
src/platform_specific/named_threads.unix.cc Outdated Show resolved Hide resolved
src/platform_specific/named_threads.win.cc Outdated Show resolved Hide resolved
src/platform_specific/named_threads.win.cc Outdated Show resolved Hide resolved
test/runtime_tests.cc Show resolved Hide resolved
@BlackMark29A
Copy link
Author

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.

@BlackMark29A BlackMark29A self-assigned this Feb 28, 2022
Copy link
Contributor

@fknorr fknorr left a 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.

include/named_threads.h Show resolved Hide resolved
Copy link
Member

@psalz psalz left a 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-?

test/runtime_tests.cc Outdated Show resolved Hide resolved
include/host_queue.h Outdated Show resolved Hide resolved
@BlackMark29A BlackMark29A force-pushed the named-threads branch 2 times, most recently from 81a0baa to a27f1c9 Compare March 8, 2022 12:51
@BlackMark29A BlackMark29A merged commit 25d769d into master Mar 8, 2022
@BlackMark29A BlackMark29A deleted the named-threads branch March 8, 2022 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants