Skip to content

feat: set thread name #115

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

Merged
merged 1 commit into from
May 9, 2025
Merged

feat: set thread name #115

merged 1 commit into from
May 9, 2025

Conversation

hs0225
Copy link
Contributor

@hs0225 hs0225 commented May 8, 2025

Signed-off-by: Hosung Kim [email protected]

Copy link
Member

@daeyeon daeyeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s worth checking --v8-pool-size as well, in relation to internal Node.js threads.

@@ -1051,6 +1051,7 @@ InitializationResult InitializeOncePerProcess(int argc, char** argv) {
V8::SetEntropySource(crypto::EntropySource);
#endif // HAVE_OPENSSL

uv_thread_setname("lwnode::Main"); // @lwnode
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
uv_thread_setname("lwnode::Main"); // @lwnode
uv_thread_setname("MainThread"); // @lwnode

@@ -27,7 +27,7 @@ struct PlatformWorkerData {
static void PlatformWorkerThread(void* data) {
std::unique_ptr<PlatformWorkerData>
worker_data(static_cast<PlatformWorkerData*>(data));

uv_thread_setname("lwnode::PT");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
uv_thread_setname("lwnode::PT");
uv_thread_setname("V8Worker");

Copy link
Member

@daeyeon daeyeon May 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, it's a V8 feature.

Comment on lines 57 to 58
uv_thread_setname("lwnode::DTask");
printf("Starting delay thread\n");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
uv_thread_setname("lwnode::DTask");
printf("Starting delay thread\n");
uv_thread_setname("V8Worker");

Comment on lines 57 to 58
static void worker(void* arg) {
uv_thread_setname("lwnode::uv");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static void worker(void* arg) {
uv_thread_setname("lwnode::uv");
static void worker(void* arg) {

Copy link
Member

@daeyeon daeyeon May 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, naming here isn’t ideal. Naming for debugging should be set at the higher level that uses uv.

@@ -45,6 +47,7 @@ void DelayedGC::handle(v8::Isolate* isolate) {

if (state_ == DelayedGCState::TIMER_END) {
std::thread([&]() {
uv_thread_setname("lwnode::gc");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
uv_thread_setname("lwnode::gc");
uv_thread_setname("ScheduleGC");

@@ -1763,6 +1763,7 @@ UV_EXTERN int uv_thread_create_ex(uv_thread_t* tid,
void* arg);
UV_EXTERN uv_thread_t uv_thread_self(void);
UV_EXTERN int uv_thread_join(uv_thread_t *tid);
UV_EXTERN int uv_thread_setname(const char* name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
UV_EXTERN int uv_thread_setname(const char* name);
// @lwnode
UV_EXTERN int uv_thread_setname(const char* name);

Does this API need the // @lwnode tag like uv_watcher_queue_empty?

Signed-off-by: Hosung Kim [email protected]
@daeyeon daeyeon merged commit 08c05f9 into Samsung:release May 9, 2025
6 checks passed
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.

2 participants