From 7b64f1cd005ef4f8e7f0aa5655debca2754b1755 Mon Sep 17 00:00:00 2001 From: Jay Krell Date: Thu, 15 Aug 2019 10:37:34 -0700 Subject: [PATCH] Retain thread names as UTF8 instead of only UTF16. (#15945) Windows can still have its Unicode thread names, since we have UTF16 initially, just don't retain it. This does penalize ves_icall_System_Threading_Thread_GetName_internal (non-netcore only), which has to convert back to UTF16 each time. But seems like a net win overall. There is also a little prep work here for constant thread names and some temporary measures as multiple changes head toward the same code. Rename `mono_thread_set_name_internal` to `mono_thread_set_name`. Close possible race conditions. Extracted from https://github.com/mono/mono/pull/15859. --- configure.ac | 2 +- mcs/class/corlib/System.Threading/Thread.cs | 5 +- mono/metadata/appdomain.c | 2 +- mono/metadata/attach.c | 2 +- mono/metadata/gc.c | 2 +- mono/metadata/object-internals.h | 11 +- mono/metadata/threadpool-io.c | 2 +- mono/metadata/threadpool.c | 6 +- mono/metadata/threads-types.h | 6 +- mono/metadata/threads.c | 132 +++++++++++------- mono/mini/aot-compiler.c | 2 +- mono/mini/debugger-agent.c | 2 +- mono/mini/mini-posix.c | 2 +- mono/profiler/aot.c | 2 +- mono/profiler/log.c | 2 +- .../src/System.Threading/Thread.cs | 3 +- 16 files changed, 113 insertions(+), 70 deletions(-) diff --git a/configure.ac b/configure.ac index f48ee908e91d..fc6f354881bb 100644 --- a/configure.ac +++ b/configure.ac @@ -62,7 +62,7 @@ MONO_VERSION_BUILD=`echo $VERSION | cut -d . -f 3` # This line is parsed by tools besides autoconf, such as msvc/mono.winconfig.targets. # It should remain in the format they expect. # -MONO_CORLIB_VERSION=ea86fac2-6390-4597-9db9-6d3c05adcd06 +MONO_CORLIB_VERSION=1b5be199-6637-495c-9baa-a95056f2137f # # Put a quoted #define in config.h. diff --git a/mcs/class/corlib/System.Threading/Thread.cs b/mcs/class/corlib/System.Threading/Thread.cs index e761e250b85f..acf5c2638e3e 100644 --- a/mcs/class/corlib/System.Threading/Thread.cs +++ b/mcs/class/corlib/System.Threading/Thread.cs @@ -54,9 +54,10 @@ sealed class InternalThread : CriticalFinalizerObject { IntPtr handle; IntPtr native_handle; // used only on Win32 /* accessed only from unmanaged code */ - private IntPtr name; + private IntPtr name_chars; private IntPtr name_generation; - private int name_len; + private int name_free; + private int name_length; private ThreadState state; private object abort_exc; private int abort_state_handle; diff --git a/mono/metadata/appdomain.c b/mono/metadata/appdomain.c index dc9368da9a5a..fa3761e19008 100644 --- a/mono/metadata/appdomain.c +++ b/mono/metadata/appdomain.c @@ -3071,7 +3071,7 @@ unload_thread_main (void *arg) MonoString *thread_name_str = mono_string_new_checked (mono_domain_get (), "Domain unloader", error); if (is_ok (error)) - mono_thread_set_name_internal (internal, thread_name_str, MonoSetThreadNameFlag_Permanent, error); + mono_thread_set_name (internal, thread_name_str, MonoSetThreadNameFlag_Permanent, error); if (!is_ok (error)) { data->failure_reason = g_strdup (mono_error_get_message (error)); goto failure; diff --git a/mono/metadata/attach.c b/mono/metadata/attach.c index 0a74b9eaa8e3..5f07af8615f2 100644 --- a/mono/metadata/attach.c +++ b/mono/metadata/attach.c @@ -513,7 +513,7 @@ receiver_thread (void *arg) internal = mono_thread_internal_current (); MonoString *attach_str = mono_string_new_checked (mono_domain_get (), "Attach receiver", error); mono_error_assert_ok (error); - mono_thread_set_name_internal (internal, attach_str, MonoSetThreadNameFlag_Permanent, error); + mono_thread_set_name (internal, attach_str, MonoSetThreadNameFlag_Permanent, error); mono_error_assert_ok (error); /* Ask the runtime to not abort this thread */ //internal->flags |= MONO_THREAD_FLAG_DONT_MANAGE; diff --git a/mono/metadata/gc.c b/mono/metadata/gc.c index 09b588b30fb1..d88e961506e1 100644 --- a/mono/metadata/gc.c +++ b/mono/metadata/gc.c @@ -948,7 +948,7 @@ finalizer_thread (gpointer unused) MonoString *finalizer = mono_string_new_checked (mono_get_root_domain (), "Finalizer", error); mono_error_assert_ok (error); - mono_thread_set_name_internal (mono_thread_internal_current (), finalizer, MonoSetThreadNameFlag_None, error); + mono_thread_set_name (mono_thread_internal_current (), finalizer, MonoSetThreadNameFlag_None, error); mono_error_assert_ok (error); /* Register a hazard free queue pump callback */ diff --git a/mono/metadata/object-internals.h b/mono/metadata/object-internals.h index 2a12b7b2a1f2..d95528de26fe 100644 --- a/mono/metadata/object-internals.h +++ b/mono/metadata/object-internals.h @@ -555,6 +555,13 @@ typedef enum { struct _MonoThreadInfo; +typedef struct MonoThreadName { + char* chars; + volatile gsize generation; + gint32 free; + gint32 length; +} MonoThreadName; + void mono_gstring_append_thread_name (GString*, MonoInternalThread*); @@ -573,9 +580,7 @@ struct _MonoInternalThread { volatile int lock_thread_id; /* to be used as the pre-shifted thread id in thin locks. Used for appdomain_ref push/pop */ MonoThreadHandle *handle; gpointer native_handle; - gunichar2 *name; - volatile gsize name_generation; - guint32 name_len; + MonoThreadName name; guint32 state; /* must be accessed while longlived->synch_cs is locked */ MonoException *abort_exc; int abort_state_handle; diff --git a/mono/metadata/threadpool-io.c b/mono/metadata/threadpool-io.c index 967970cce459..c2e622e85ab2 100644 --- a/mono/metadata/threadpool-io.c +++ b/mono/metadata/threadpool-io.c @@ -330,7 +330,7 @@ selector_thread (gpointer data) MonoString *thread_name = mono_string_new_checked (mono_get_root_domain (), "Thread Pool I/O Selector", error); mono_error_assert_ok (error); - mono_thread_set_name_internal (mono_thread_internal_current (), thread_name, MonoSetThreadNameFlag_Reset, error); + mono_thread_set_name (mono_thread_internal_current (), thread_name, MonoSetThreadNameFlag_Reset, error); mono_error_assert_ok (error); if (mono_runtime_is_shutting_down ()) { diff --git a/mono/metadata/threadpool.c b/mono/metadata/threadpool.c index 2cf83233a66e..8c6db898d16b 100644 --- a/mono/metadata/threadpool.c +++ b/mono/metadata/threadpool.c @@ -325,7 +325,7 @@ worker_callback (void) previous_tpdomain = NULL; - gsize name_generation = ~thread->name_generation; + gsize name_generation = ~thread->name.generation; while (!mono_runtime_is_shutting_down ()) { gboolean retire = FALSE; @@ -359,10 +359,10 @@ worker_callback (void) // This only partly fights against that -- i.e. not atomic and not a loop. // It is reliable against the thread setting its own name, and somewhat // reliable against other threads setting this thread's name. - if (name_generation != thread->name_generation) { + if (name_generation != thread->name.generation) { MonoString *thread_name = mono_string_new_checked (mono_get_root_domain (), "Thread Pool Worker", error); mono_error_assert_ok (error); - name_generation = mono_thread_set_name_internal (thread, thread_name, MonoSetThreadNameFlag_Reset, error); + name_generation = mono_thread_set_name (thread, thread_name, MonoSetThreadNameFlag_Reset, error); mono_error_assert_ok (error); } diff --git a/mono/metadata/threads-types.h b/mono/metadata/threads-types.h index a29c03853997..eab0dc9494d0 100644 --- a/mono/metadata/threads-types.h +++ b/mono/metadata/threads-types.h @@ -351,9 +351,9 @@ G_ENUM_FUNCTIONS (MonoSetThreadNameFlags) MONO_PROFILER_API gsize -mono_thread_set_name_internal (MonoInternalThread *thread, - MonoString *name, - MonoSetThreadNameFlags flags, MonoError *error); +mono_thread_set_name (MonoInternalThread *thread, + MonoString *name, + MonoSetThreadNameFlags flags, MonoError *error); void mono_thread_suspend_all_other_threads (void); gboolean mono_threads_abort_appdomain_threads (MonoDomain *domain, int timeout); diff --git a/mono/metadata/threads.c b/mono/metadata/threads.c index 90f7f46c2d93..2516ddb6444a 100644 --- a/mono/metadata/threads.c +++ b/mono/metadata/threads.c @@ -1210,11 +1210,18 @@ start_wrapper_internal (StartInfo *start_info, gsize *stack_ptr) fire_attach_profiler_events ((MonoNativeThreadId) tid); /* if the name was set before starting, we didn't invoke the profiler callback */ - if (internal->name) { - char *tname = g_utf16_to_utf8 (internal->name, internal->name_len, NULL, NULL, NULL); - MONO_PROFILER_RAISE (thread_name, (internal->tid, tname)); - mono_native_thread_set_name (MONO_UINT_TO_NATIVE_THREAD_ID (internal->tid), tname); - g_free (tname); + // This is a little racy, ok. + + if (internal->name.chars) { + + LOCK_THREAD (internal); + + if (internal->name.chars) { + MONO_PROFILER_RAISE (thread_name, (internal->tid, internal->name.chars)); + mono_native_thread_set_name (MONO_UINT_TO_NATIVE_THREAD_ID (internal->tid), internal->name.chars); + } + + UNLOCK_THREAD (internal); } /* start_func is set only for unmanaged start functions */ @@ -1702,6 +1709,20 @@ ves_icall_System_Threading_Thread_Thread_internal (MonoThreadObjectHandle thread } #endif +static +void +mono_thread_name_cleanup (MonoThreadName* name) +{ + MonoThreadName const old_name = *name; + // Do not reset generation. + name->chars = 0; + name->length = 0; + name->free = 0; + //memset (name, 0, sizeof (*name)); + if (old_name.free) + g_free (old_name.chars); +} + /* * This is called from the finalizer of the internal thread object. */ @@ -1728,12 +1749,7 @@ ves_icall_System_Threading_InternalThread_Thread_free_internal (MonoInternalThre /* Possibly free synch_cs, if the thread already detached also. */ dec_longlived_thread_data (this_obj->longlived); - - if (this_obj->name) { - void *name = this_obj->name; - this_obj->name = NULL; - g_free (name); - } + mono_thread_name_cleanup (&this_obj->name); } void @@ -1805,12 +1821,15 @@ mono_thread_get_name_utf8 (MonoThread *thread) return NULL; MonoInternalThread *internal = thread->internal_thread; - if (internal == NULL) + + // This is a little racy, ok. + + if (internal == NULL || !internal->name.chars) return NULL; LOCK_THREAD (internal); - char *tname = g_utf16_to_utf8 (internal->name, internal->name_len, NULL, NULL, NULL); + char *tname = (char*)g_memdup (internal->name.chars, internal->name.length + 1); UNLOCK_THREAD (internal); @@ -1844,23 +1863,28 @@ ves_icall_System_Threading_Thread_GetName_internal (MonoInternalThreadHandle thr // InternalThreads are always pinned, so shallowly coop-handleize. MonoInternalThread * const this_obj = mono_internal_thread_handle_ptr (thread_handle); - MonoStringHandle str = MONO_HANDLE_NEW (MonoString, NULL); + MonoStringHandle str = NULL_HANDLE_STRING; - LOCK_THREAD (this_obj); - - if (this_obj->name) - MONO_HANDLE_ASSIGN (str, mono_string_new_utf16_handle (mono_domain_get (), this_obj->name, this_obj->name_len, error)); + // This is a little racy, ok. + + if (this_obj->name.chars) { + + LOCK_THREAD (this_obj); + + if (this_obj->name.chars) + str = mono_string_new_utf8_len (mono_domain_get (), this_obj->name.chars, this_obj->name.length, error); + + UNLOCK_THREAD (this_obj); + } - UNLOCK_THREAD (this_obj); - return str; } #endif gsize -mono_thread_set_name_internal (MonoInternalThread *this_obj, - MonoString *name, - MonoSetThreadNameFlags flags, MonoError *error) +mono_thread_set_name (MonoInternalThread *this_obj, + MonoString *name, + MonoSetThreadNameFlags flags, MonoError *error) { MonoNativeThreadId tid = 0; @@ -1868,9 +1892,25 @@ mono_thread_set_name_internal (MonoInternalThread *this_obj, // It is not exactly thread safe but no use of it could be. gsize name_generation; + // FIXME lots of temporary stuff here. + // Callers will pass utf8 and on Windows utf16. + + const gunichar2* name16 = NULL; + gint32 name16_length = 0; + long name8_length = 0; + const char* name8 = NULL; + + if (name) { + name16 = mono_string_chars_internal (name); + name16_length = mono_string_length_internal (name); + name8 = name16 ? g_utf16_to_utf8 (name16, name16_length, NULL, &name8_length, NULL) : NULL; + } + + const gboolean constant = FALSE; // !!(flags & MonoSetThreadNameFlag_Constant) + LOCK_THREAD (this_obj); - name_generation = this_obj->name_generation; + name_generation = this_obj->name.generation; if (flags & MonoSetThreadNameFlag_Reset) { this_obj->flags &= ~MONO_THREAD_FLAG_NAME_SET; @@ -1878,49 +1918,49 @@ mono_thread_set_name_internal (MonoInternalThread *this_obj, UNLOCK_THREAD (this_obj); mono_error_set_invalid_operation (error, "%s", "Thread.Name can only be set once."); + + if (!constant) + g_free ((char*)name8); return name_generation; } - name_generation = ++this_obj->name_generation; + name_generation = ++this_obj->name.generation; - if (this_obj->name) { - g_free (this_obj->name); - this_obj->name_len = 0; - } - if (name) { - this_obj->name = g_memdup (mono_string_chars_internal (name), mono_string_length_internal (name) * sizeof (gunichar2)); - this_obj->name_len = mono_string_length_internal (name); + mono_thread_name_cleanup (&this_obj->name); + if (name8) { + this_obj->name.chars = (char*)name8; + this_obj->name.length = name8_length; + this_obj->name.free = !constant; if (flags & MonoSetThreadNameFlag_Permanent) this_obj->flags |= MONO_THREAD_FLAG_NAME_SET; } - else - this_obj->name = NULL; if (!(this_obj->state & ThreadState_Stopped)) tid = thread_get_tid (this_obj); UNLOCK_THREAD (this_obj); - if (this_obj->name && tid) { - char *tname = mono_string_to_utf8_checked_internal (name, error); - if (is_ok (error)) { - MONO_PROFILER_RAISE (thread_name, ((uintptr_t)tid, tname)); - mono_native_thread_set_name (tid, tname); - mono_free (tname); - } + if (name8 && tid) { + MONO_PROFILER_RAISE (thread_name, ((uintptr_t)tid, name8)); + mono_native_thread_set_name (tid, name8); } - mono_thread_set_name_windows (this_obj->native_handle, name ? mono_string_chars_internal (name) : NULL); + mono_thread_set_name_windows (this_obj->native_handle, name16); + + mono_free (0); // FIXME keep mono-publib.c in use and its functions exported return name_generation; + } void ves_icall_System_Threading_Thread_SetName_internal (MonoInternalThread *this_obj, MonoString *name) { + // FIXME This function churning. + ERROR_DECL (error); - mono_thread_set_name_internal (this_obj, name, MonoSetThreadNameFlag_Permanent, error); + mono_thread_set_name (this_obj, name, MonoSetThreadNameFlag_Permanent, error); mono_error_set_pending_exception (error); } @@ -3923,16 +3963,12 @@ collect_threads (guint32 *thread_handles, int max_threads) void mono_gstring_append_thread_name (GString* text, MonoInternalThread* thread) { - // FIXME Conversion here is temporary. thread->name will change to UTF8. - char* const name = thread->name - ? g_utf16_to_utf8 (thread->name, thread->name_len, NULL, NULL, NULL) - : NULL; g_string_append (text, "\n\""); + char const * const name = thread->name.chars; g_string_append (text, name ? name : thread->threadpool_thread ? "" : ""); g_string_append (text, "\""); - g_free (name); } static void diff --git a/mono/mini/aot-compiler.c b/mono/mini/aot-compiler.c index 75b8b500a11a..a53be5227d1c 100644 --- a/mono/mini/aot-compiler.c +++ b/mono/mini/aot-compiler.c @@ -8786,7 +8786,7 @@ compile_thread_main (gpointer user_data) MonoInternalThread *internal = mono_thread_internal_current (); MonoString *str = mono_string_new_checked (mono_domain_get (), "AOT compiler", error); mono_error_assert_ok (error); - mono_thread_set_name_internal (internal, str, MonoSetThreadNameFlag_Permanent, error); + mono_thread_set_name (internal, str, MonoSetThreadNameFlag_Permanent, error); mono_error_assert_ok (error); for (i = 0; i < methods->len; ++i) diff --git a/mono/mini/debugger-agent.c b/mono/mini/debugger-agent.c index 4c62823e176e..5cecb4c0a0fc 100644 --- a/mono/mini/debugger-agent.c +++ b/mono/mini/debugger-agent.c @@ -9872,7 +9872,7 @@ debugger_thread (void *arg) MonoInternalThread *internal = mono_thread_internal_current (); MonoString *str = mono_string_new_checked (mono_domain_get (), "Debugger agent", error); mono_error_assert_ok (error); - mono_thread_set_name_internal (internal, str, MonoSetThreadNameFlag_Permanent, error); + mono_thread_set_name (internal, str, MonoSetThreadNameFlag_Permanent, error); mono_error_assert_ok (error); internal->state |= ThreadState_Background; diff --git a/mono/mini/mini-posix.c b/mono/mini/mini-posix.c index 38896f37cb51..f88a5e3c3116 100644 --- a/mono/mini/mini-posix.c +++ b/mono/mini/mini-posix.c @@ -693,7 +693,7 @@ sampling_thread_func (gpointer unused) MonoString *name = mono_string_new_checked (mono_get_root_domain (), "Profiler Sampler", error); mono_error_assert_ok (error); - mono_thread_set_name_internal (thread, name, MonoSetThreadNameFlag_None, error); + mono_thread_set_name (thread, name, MonoSetThreadNameFlag_None, error); mono_error_assert_ok (error); mono_thread_info_set_flags (MONO_THREAD_INFO_FLAGS_NO_GC | MONO_THREAD_INFO_FLAGS_NO_SAMPLE); diff --git a/mono/profiler/aot.c b/mono/profiler/aot.c index e7c59d87fbc2..199b7b0c03e6 100644 --- a/mono/profiler/aot.c +++ b/mono/profiler/aot.c @@ -220,7 +220,7 @@ helper_thread (void *arg) MonoString *name_str = mono_string_new_checked (mono_get_root_domain (), "AOT Profiler Helper", error); mono_error_assert_ok (error); - mono_thread_set_name_internal (internal, name_str, MonoSetThreadNameFlag_None, error); + mono_thread_set_name (internal, name_str, MonoSetThreadNameFlag_None, error); mono_error_assert_ok (error); mono_thread_info_set_flags (MONO_THREAD_INFO_FLAGS_NO_GC | MONO_THREAD_INFO_FLAGS_NO_SAMPLE); diff --git a/mono/profiler/log.c b/mono/profiler/log.c index 223c292b6425..6c1f231d9642 100644 --- a/mono/profiler/log.c +++ b/mono/profiler/log.c @@ -3164,7 +3164,7 @@ profiler_thread_begin (const char *name, gboolean send) MonoString *name_str = mono_string_new_checked (mono_get_root_domain (), name, error); mono_error_assert_ok (error); - mono_thread_set_name_internal (internal, name_str, MonoSetThreadNameFlag_None, error); + mono_thread_set_name (internal, name_str, MonoSetThreadNameFlag_None, error); mono_error_assert_ok (error); mono_thread_info_set_flags (MONO_THREAD_INFO_FLAGS_NO_GC | MONO_THREAD_INFO_FLAGS_NO_SAMPLE); diff --git a/netcore/System.Private.CoreLib/src/System.Threading/Thread.cs b/netcore/System.Private.CoreLib/src/System.Threading/Thread.cs index cedea65b267a..01a93b3d38e6 100644 --- a/netcore/System.Private.CoreLib/src/System.Threading/Thread.cs +++ b/netcore/System.Private.CoreLib/src/System.Threading/Thread.cs @@ -23,7 +23,8 @@ partial class Thread /* accessed only from unmanaged code */ private IntPtr name; private IntPtr name_generation; - private int name_len; + private int name_free; + private int name_length; private ThreadState state; private object abort_exc; private int abort_state_handle;