Add a thread name to all DALI threads #3912

JanuszL · 2022-05-17T23:56:51Z

adds an ability to name thread poll and the worker thread
threads so they are easier to identified on the nsight traces

Signed-off-by: Janusz Lisiecki jlisiecki@nvidia.com

Category:

Other (e.g. Documentation, Tests, Configuration)

Description:

adds an ability to name thread poll and the worker thread
threads so they are easier to identified on the nsight traces

Additional information:

Affected modules and functionalities:

worker thread
thread pool
all places (besides tests) that uses them

Key points relevant for the review:

If all places using threads are covered

Checklist

Tests

Documentation

DALI team only

Requirements

Implements new requirements
Affects existing requirements
N/A

REQ IDs: N/A

JIRA TASK: N/A

JanuszL · 2022-05-18T22:36:44Z

!build

JanuszL · 2022-05-18T22:38:03Z

dali-automaton · 2022-05-18T22:40:26Z

CI MESSAGE: [4876435]: BUILD STARTED

dali-automaton · 2022-05-18T23:49:33Z

CI MESSAGE: [4876435]: BUILD PASSED

JanuszL · 2022-05-19T05:23:49Z

!build

dali-automaton · 2022-05-19T05:25:09Z

CI MESSAGE: [4879091]: BUILD STARTED

dali-automaton · 2022-05-19T06:34:29Z

CI MESSAGE: [4879091]: BUILD PASSED

awolant

Maybe it would be good to add pthread_getname_np to tests to make sure that it works and keeps working in the future?
Regardless, cool feature.

JanuszL · 2022-05-20T16:03:24Z

Maybe it would be good to add pthread_getname_np to tests to make sure that it works and keeps working in the future? Regardless, cool feature.

@awolant - done

JanuszL · 2022-05-20T16:03:30Z

!build

dali-automaton · 2022-05-20T16:05:03Z

CI MESSAGE: [4893041]: BUILD STARTED

dali-automaton · 2022-05-20T17:18:39Z

CI MESSAGE: [4893041]: BUILD PASSED

banasraf · 2022-05-23T08:02:27Z

include/dali/core/nvtx.h

@@ -100,6 +100,8 @@ struct DomainTimeRange : RangeBase {
 #endif
 };

+void SetThreadName(const char *name);


Maybe some docs comment that only first 15 characters of the name will be used?

Ah, I see it's only for the pthread name. So maybe it's not that important to note

banasraf · 2022-05-23T08:05:20Z

dali/core/nvtx.cc

+
+#else
+
+void SetThreadName(const char *name) {}


The impl without NVTX enabled still could set the pthread name, right?

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

JanuszL · 2022-05-23T13:06:01Z

!build

dali-automaton · 2022-05-23T13:10:00Z

CI MESSAGE: [4910165]: BUILD STARTED

dali-automaton · 2022-05-23T13:14:24Z

CI MESSAGE: [4910165]: BUILD FAILED

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

dali-automaton · 2022-05-23T13:33:32Z

CI MESSAGE: [4910272]: BUILD STARTED

dali-automaton · 2022-05-23T13:34:31Z

CI MESSAGE: [4910272]: BUILD FAILED

dali-automaton · 2022-05-23T13:40:57Z

CI MESSAGE: [4910311]: BUILD STARTED

dali-automaton · 2022-05-23T13:41:26Z

CI MESSAGE: [4910314]: BUILD STARTED

dali-automaton · 2022-05-23T13:41:51Z

CI MESSAGE: [4910311]: BUILD FAILED

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

mzient · 2022-05-23T13:53:23Z

dali/pipeline/util/worker_thread.h

@@ -66,15 +67,15 @@ class WorkerThread {
 public:
  typedef std::function<void(void)> Work;

-  inline WorkerThread(int device_id, bool set_affinity) :
+  inline WorkerThread(int device_id, bool set_affinity, const std::string name) :


Suggested change

inline WorkerThread(int device_id, bool set_affinity, const std::string name) :

inline WorkerThread(int device_id, bool set_affinity, const std::string &name) :

mzient · 2022-05-23T13:53:59Z

dali/pipeline/util/thread_pool.cc


 namespace dali {

-ThreadPool::ThreadPool(int num_thread, int device_id, bool set_affinity)
+ThreadPool::ThreadPool(int num_thread, int device_id, bool set_affinity, const std::string name)


Suggested change

ThreadPool::ThreadPool(int num_thread, int device_id, bool set_affinity, const std::string name)

ThreadPool::ThreadPool(int num_thread, int device_id, bool set_affinity, const std::string &name)

mzient · 2022-05-23T13:54:18Z

dali/pipeline/util/thread_pool.h

+  DLL_PUBLIC ThreadPool(int num_thread, int device_id, bool set_affinity,
+                        const std::string name);


Suggested change

DLL_PUBLIC ThreadPool(int num_thread, int device_id, bool set_affinity,

const std::string name);

DLL_PUBLIC ThreadPool(int num_thread, int device_id, bool set_affinity,

const std::string &name);

mzient

Other than a few missing &, OK.

dali-automaton · 2022-05-23T14:06:19Z

CI MESSAGE: [4910314]: BUILD FAILED

dali-automaton · 2022-05-23T18:07:58Z

CI MESSAGE: [4912612]: BUILD STARTED

dali-automaton · 2022-05-23T18:32:14Z

CI MESSAGE: [4912612]: BUILD FAILED

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

dali-automaton · 2022-05-23T18:37:35Z

CI MESSAGE: [4912820]: BUILD STARTED

dali-automaton · 2022-05-23T19:04:19Z

CI MESSAGE: [4912820]: BUILD FAILED

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

dali-automaton · 2022-05-23T19:35:41Z

CI MESSAGE: [4913316]: BUILD STARTED

dali-automaton · 2022-05-23T20:52:03Z

CI MESSAGE: [4913316]: BUILD PASSED

- adds an ability to name thread poll and the worker thread threads so they are easier to identify on the nsight traces - use NVTX API to set full names visible in nsight traces and pthread_setname_np to set truncated names visible in the GDB debugger Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

JanuszL marked this pull request as draft May 17, 2022 23:56

JanuszL force-pushed the name_threads branch 10 times, most recently from 77ce73c to ef02b63 Compare May 18, 2022 22:34

JanuszL marked this pull request as ready for review May 18, 2022 22:36

JanuszL force-pushed the name_threads branch from ef02b63 to a1acac6 Compare May 18, 2022 22:36

JanuszL force-pushed the name_threads branch from a1acac6 to 9bc699d Compare May 19, 2022 05:23

awolant self-assigned this May 19, 2022

jantonguirao assigned banasraf May 19, 2022

awolant approved these changes May 20, 2022

View reviewed changes

banasraf reviewed May 23, 2022

View reviewed changes

Review fixes

18ce87b

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

Fix

a2fc3ee

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

Fix

7398e0b

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

mzient reviewed May 23, 2022

View reviewed changes

mzient approved these changes May 23, 2022

View reviewed changes

JanuszL force-pushed the name_threads branch 2 times, most recently from e4b6c19 to eecd062 Compare May 23, 2022 18:00

Review fixes

95f9e2b

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

JanuszL force-pushed the name_threads branch from eecd062 to 95f9e2b Compare May 23, 2022 18:36

Fix

27bf665

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

JanuszL merged commit cad8540 into NVIDIA:main May 24, 2022

JanuszL deleted the name_threads branch May 24, 2022 07:24

	inline WorkerThread(int device_id, bool set_affinity, const std::string name) :
	inline WorkerThread(int device_id, bool set_affinity, const std::string &name) :

	ThreadPool::ThreadPool(int num_thread, int device_id, bool set_affinity, const std::string name)
	ThreadPool::ThreadPool(int num_thread, int device_id, bool set_affinity, const std::string &name)

		DLL_PUBLIC ThreadPool(int num_thread, int device_id, bool set_affinity,
		const std::string name);

Add a thread name to all DALI threads #3912

Add a thread name to all DALI threads #3912

Conversation

JanuszL commented May 17, 2022 • edited Loading

Category:

Description:

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Checklist

Tests

Documentation

DALI team only

Requirements

JanuszL commented May 18, 2022

JanuszL commented May 18, 2022

dali-automaton commented May 18, 2022

dali-automaton commented May 18, 2022

JanuszL commented May 19, 2022

dali-automaton commented May 19, 2022

dali-automaton commented May 19, 2022

awolant left a comment

Choose a reason for hiding this comment

JanuszL commented May 20, 2022

JanuszL commented May 20, 2022

dali-automaton commented May 20, 2022

dali-automaton commented May 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JanuszL commented May 23, 2022

dali-automaton commented May 23, 2022

dali-automaton commented May 23, 2022

dali-automaton commented May 23, 2022

dali-automaton commented May 23, 2022

dali-automaton commented May 23, 2022

dali-automaton commented May 23, 2022

dali-automaton commented May 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzient left a comment

Choose a reason for hiding this comment

dali-automaton commented May 23, 2022

dali-automaton commented May 23, 2022

dali-automaton commented May 23, 2022

dali-automaton commented May 23, 2022

dali-automaton commented May 23, 2022

dali-automaton commented May 23, 2022

dali-automaton commented May 23, 2022

JanuszL commented May 17, 2022 •

edited

Loading