Assign names to threads #716
Conversation
github-actions
bot
added
work in progress
|
🤖 The latest upstream change made this pull request unmergeable. Please resolve the merge conflicts. |
github-actions
bot
added
ready for review
|
Hi, thanks for the patch! Could you please rebase on fresh develop? There are conflicts. |
| // Thread::Thread() | ||
| // : started_(0) | ||
| // , joinable_(0) | ||
| // , name_(nullptr) | ||
| // { | ||
| // } |
| #else | ||
| pthread_setname_np(pthread_self(), name_); | ||
| return true; | ||
| #endif |
There was a problem hiding this comment.
Since pthread_setname_np() is not portable, we should guard this too, I think in this case it can be if defined(__GLIBC__) (because not all linux libc-s have that function).
| if (!assign_thread_name_()) { | ||
| roc_log(LogError, "thread: unable to assign thread name"); | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
We should call it from thread_runner_(), not in start(), because __APPLE__ version work only for current thread.
| } | ||
| return false; |
There was a problem hiding this comment.
Since this is an optional feature for debugging, if there is no appropriate ifdef (e.g. when compiling for some less popular unix variant), it should not be an error. We should just do nothing.
| // for my own debugging purposes | ||
| void Thread::print_name() { | ||
| printf("Thread name: %s\n", name_); | ||
| } |
There was a problem hiding this comment.
Please remove debug printfs when submitting patches.
| @@ -15,7 +15,8 @@ namespace roc { | |||
| namespace ctl { | |||
|
|
|||
| ControlTaskQueue::ControlTaskQueue() | |||
| : started_(false) | |||
| : Thread("roc_ctrl_task_queue") | |||
There was a problem hiding this comment.
Since this thread actually serves requests to ctl::ControlLoop, let's call it roc_control_loop.
| @@ -36,7 +36,8 @@ class Receiver : public core::Thread { | |||
| size_t num_chans, | |||
| size_t frame_size, | |||
| unsigned flags) | |||
| : recv_(NULL) | |||
| : Thread("roc_receiver") | |||
There was a problem hiding this comment.
Here in below: to avoid inventing names for each test, since these names in tests are not important anyway, maybe let's just use same name like "roc_test", "test_thread", or something like this?
I don't want to make name optional, so we have compiler error if non-test code forgets to assign a name. But on the other hand maintaining those names in tests feels unnecessary.
| // { | ||
| // } | ||
|
|
||
| Thread::Thread(const char* name_) |
There was a problem hiding this comment.
name_ => name
(trailing underscore is for private members)
| , joinable_(0) | ||
| , name_(name_) { | ||
| } |
There was a problem hiding this comment.
Let's add a panic triggered if name is NULL.
gavv
added
needs revision
Added a thread name char * argument to Thread constructor and applied changes to inherited classes. Added assign_name_() method for different architectures. Also added print_name() method for debugging to keep name_ member private.
Issue link: #618