Retain thread names as UTF8 instead of only UTF16. (mono#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 mono#15859.

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.

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;

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;

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;

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 */

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;

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 ()) {

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);
 		}
 

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);

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,83 +1863,104 @@ 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;
 
 	// A counter to optimize redundant sets.
 	// 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;
 	} else if (this_obj->flags & MONO_THREAD_FLAG_NAME_SET) {
 		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 ? "<threadpool thread>" :
 						    "<unnamed thread>");
 	g_string_append (text, "\"");
-	g_free (name);
 }
 
 static void

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)

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;

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);

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);

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);