From 47f402b55c74bbb3009de667377781b8a844eecb Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Fri, 19 Apr 2024 15:53:36 -0700 Subject: [PATCH 1/7] Remove unnecessary indirect use of managed wcslen from AppContext setup Remove unnecessary use of EqualityComparer.Default from AppContext setup --- .../src/System/AppContext.cs | 10 ++++ src/mono/mono/metadata/appdomain.c | 50 ++++++++++++++----- 2 files changed, 48 insertions(+), 12 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/AppContext.cs b/src/libraries/System.Private.CoreLib/src/System/AppContext.cs index b59a7bdcc372fd..f3f7a003f8fc5e 100644 --- a/src/libraries/System.Private.CoreLib/src/System/AppContext.cs +++ b/src/libraries/System.Private.CoreLib/src/System/AppContext.cs @@ -151,6 +151,16 @@ internal static unsafe void Setup(char** pNames, char** pValues, int count) s_dataStore.Add(new string(pNames[i]), new string(pValues[i])); } } + + internal static unsafe void Setup(char** pNames, uint** pNameLengths, char** pValues, uint** pValueLengths, int count) + { + Debug.Assert(s_dataStore == null, "s_dataStore is not expected to be inited before Setup is called"); + s_dataStore = new Dictionary(count, StringComparer.Ordinal); + for (int i = 0; i < count; i++) + { + s_dataStore.Add(new string(pNames[i], 0, (int)pNameLengths[i]), new string(pValues[i], 0, (int)pValueLengths[i])); + } + } #endif } } diff --git a/src/mono/mono/metadata/appdomain.c b/src/mono/mono/metadata/appdomain.c index e41b4fb1ca8b48..1426986e55e648 100644 --- a/src/mono/mono/metadata/appdomain.c +++ b/src/mono/mono/metadata/appdomain.c @@ -99,7 +99,11 @@ static const char * runtimeconfig_json_get_buffer (MonovmRuntimeConfigArguments *arg, MonoFileMap **file_map, gpointer *buf_handle); static void -runtimeconfig_json_read_props (const char *ptr, const char **endp, int nprops, gunichar2 **dest_keys, gunichar2 **dest_values); +runtimeconfig_json_read_props ( + const char *ptr, const char **endp, int nprops, + gunichar2 **dest_keys, guint32 *dest_key_lengths, + gunichar2 **dest_values, guint32 *dest_value_lengths +); static MonoLoadFunc load_function = NULL; @@ -846,17 +850,19 @@ void mono_runtime_install_appctx_properties (void) { ERROR_DECL (error); - gpointer args [3]; + gpointer args [5]; int n_runtimeconfig_json_props = 0; int n_combined_props; gunichar2 **combined_keys; gunichar2 **combined_values; + guint32 *combined_key_lengths; + guint32 *combined_value_lengths; MonoFileMap *runtimeconfig_json_map = NULL; gpointer runtimeconfig_json_map_handle = NULL; const char *buffer_start = runtimeconfig_json_get_buffer (runtime_config_arg, &runtimeconfig_json_map, &runtimeconfig_json_map_handle); const char *buffer = buffer_start; - MonoMethod *setup = mono_class_get_method_from_name_checked (mono_class_get_appctx_class (), "Setup", 3, 0, error); + MonoMethod *setup = mono_class_get_method_from_name_checked (mono_class_get_appctx_class (), "Setup", 5, 0, error); g_assert (setup); // FIXME: TRUSTED_PLATFORM_ASSEMBLIES is very large @@ -867,19 +873,30 @@ mono_runtime_install_appctx_properties (void) n_combined_props = n_appctx_props + n_runtimeconfig_json_props; combined_keys = g_new0 (gunichar2 *, n_combined_props); + combined_key_lengths = g_new0 (guint32, n_combined_props); combined_values = g_new0 (gunichar2 *, n_combined_props); + combined_value_lengths = g_new0 (guint32, n_combined_props); for (int i = 0; i < n_appctx_props; ++i) { - combined_keys [i] = g_utf8_to_utf16 (appctx_keys [i], -1, NULL, NULL, NULL); - combined_values [i] = g_utf8_to_utf16 (appctx_values [i], -1, NULL, NULL, NULL); + glong num_chars; + combined_keys [i] = g_utf8_to_utf16 (appctx_keys [i], -1, NULL, &num_chars, NULL); + combined_key_lengths[i] = GLONG_TO_UINT32(num_chars); + combined_values [i] = g_utf8_to_utf16 (appctx_values [i], -1, NULL, &num_chars, NULL); + combined_value_lengths[i] = GLONG_TO_UINT32(num_chars); } - runtimeconfig_json_read_props (buffer, &buffer, n_runtimeconfig_json_props, combined_keys + n_appctx_props, combined_values + n_appctx_props); + runtimeconfig_json_read_props ( + buffer, &buffer, n_runtimeconfig_json_props, + combined_keys + n_appctx_props, combined_key_lengths + n_appctx_props, + combined_values + n_appctx_props, combined_value_lengths + n_appctx_props + ); - /* internal static unsafe void Setup(char** pNames, char** pValues, int count) */ + /* internal static unsafe void Setup_Mono(char** pNames, uint** pNameLengths, char** pValues, uint** pValueLengths, int count) */ args [0] = combined_keys; - args [1] = combined_values; - args [2] = &n_combined_props; + args [1] = combined_key_lengths; + args [2] = combined_values; + args [3] = combined_value_lengths; + args [4] = &n_combined_props; mono_runtime_invoke_checked (setup, NULL, args, error); mono_error_assert_ok (error); @@ -900,6 +917,8 @@ mono_runtime_install_appctx_properties (void) } g_free (combined_keys); g_free (combined_values); + g_free (combined_key_lengths); + g_free (combined_value_lengths); for (int i = 0; i < n_appctx_props; ++i) { g_free (appctx_keys [i]); g_free (appctx_values [i]); @@ -949,17 +968,24 @@ runtimeconfig_json_get_buffer (MonovmRuntimeConfigArguments *arg, MonoFileMap ** } static void -runtimeconfig_json_read_props (const char *ptr, const char **endp, int nprops, gunichar2 **dest_keys, gunichar2 **dest_values) +runtimeconfig_json_read_props ( + const char *ptr, const char **endp, int nprops, + gunichar2 **dest_keys, guint32 *dest_key_lengths, + gunichar2 **dest_values, guint32 *dest_value_lengths +) { for (int i = 0; i < nprops; ++i) { int str_len; + glong chars_written; str_len = mono_metadata_decode_value (ptr, &ptr); - dest_keys [i] = g_utf8_to_utf16 (ptr, str_len, NULL, NULL, NULL); + dest_keys [i] = g_utf8_to_utf16 (ptr, str_len, NULL, &chars_written, NULL); + dest_key_lengths [i] = GLONG_TO_UINT32 (chars_written); ptr += str_len; str_len = mono_metadata_decode_value (ptr, &ptr); - dest_values [i] = g_utf8_to_utf16 (ptr, str_len, NULL, NULL, NULL); + dest_values [i] = g_utf8_to_utf16 (ptr, str_len, NULL, &chars_written, NULL); + dest_value_lengths [i] = GLONG_TO_UINT32 (chars_written); ptr += str_len; } From e4562459c64f3198ab26537ab77a8e4df83e25dc Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Fri, 19 Apr 2024 16:20:47 -0700 Subject: [PATCH 2/7] Add a path for startup to bypass NonRandomizedStringEqualityComparer and EqualityComparer.Default when creating a dictionary with string keys --- .../src/System/AppContext.cs | 18 ++++++++++++++++- .../System/Collections/Generic/Dictionary.cs | 20 +++++++++++-------- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/AppContext.cs b/src/libraries/System.Private.CoreLib/src/System/AppContext.cs index f3f7a003f8fc5e..341c2daa515682 100644 --- a/src/libraries/System.Private.CoreLib/src/System/AppContext.cs +++ b/src/libraries/System.Private.CoreLib/src/System/AppContext.cs @@ -152,10 +152,26 @@ internal static unsafe void Setup(char** pNames, char** pValues, int count) } } + private sealed class AppContextKeyComparer : IEqualityComparer + { + public bool Equals(string? x, string? y) => string.Equals(x, y); + + public int GetHashCode(string? obj) + { + Debug.Assert(obj != null, "This implementation is only called from first-party collection types that guarantee non-null parameters."); + return obj.GetNonRandomizedHashCode(); + } + } + internal static unsafe void Setup(char** pNames, uint** pNameLengths, char** pValues, uint** pValueLengths, int count) { Debug.Assert(s_dataStore == null, "s_dataStore is not expected to be inited before Setup is called"); - s_dataStore = new Dictionary(count, StringComparer.Ordinal); + // HACK: Use a special comparer that does not pull in all the dependencies that + // the default BCL string comparers pull in + IEqualityComparer comparer = new AppContextKeyComparer(); + // HACK: Use a special version of the Dictionary constructor that does not pull in a + // large amount of extra code in order to instantiate this dictionary + s_dataStore = new Dictionary(count, comparer, true); for (int i = 0; i < count; i++) { s_dataStore.Add(new string(pNames[i], 0, (int)pNameLengths[i]), new string(pValues[i], 0, (int)pValueLengths[i])); diff --git a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs index 9b18d4a125ed21..4efad297a9b0ef 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs @@ -43,7 +43,9 @@ public Dictionary(int capacity) : this(capacity, null) { } public Dictionary(IEqualityComparer? comparer) : this(0, comparer) { } - public Dictionary(int capacity, IEqualityComparer? comparer) + public Dictionary(int capacity, IEqualityComparer? comparer) : this(0, comparer, false) { } + + internal Dictionary(int capacity, IEqualityComparer? comparer, bool useProvidedComparer) { if (capacity < 0) { @@ -66,13 +68,15 @@ public Dictionary(int capacity, IEqualityComparer? comparer) { _comparer = comparer ?? EqualityComparer.Default; - // Special-case EqualityComparer.Default, StringComparer.Ordinal, and StringComparer.OrdinalIgnoreCase. - // We use a non-randomized comparer for improved perf, falling back to a randomized comparer if the - // hash buckets become unbalanced. - if (typeof(TKey) == typeof(string) && - NonRandomizedStringEqualityComparer.GetStringComparer(_comparer!) is IEqualityComparer stringComparer) - { - _comparer = (IEqualityComparer)stringComparer; + if (!useProvidedComparer) { + // Special-case EqualityComparer.Default, StringComparer.Ordinal, and StringComparer.OrdinalIgnoreCase. + // We use a non-randomized comparer for improved perf, falling back to a randomized comparer if the + // hash buckets become unbalanced. + if (typeof(TKey) == typeof(string) && + NonRandomizedStringEqualityComparer.GetStringComparer(_comparer!) is IEqualityComparer stringComparer) + { + _comparer = (IEqualityComparer)stringComparer; + } } } else if (comparer is not null && // first check for null to avoid forcing default comparer instantiation unnecessarily From a6a55133d2a0780086d396cc4eae8fe9d8d6f824 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Fri, 19 Apr 2024 16:30:20 -0700 Subject: [PATCH 3/7] Whitespace cleanup --- .../src/System/Collections/Generic/Dictionary.cs | 3 ++- src/mono/mono/metadata/appdomain.c | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs index 4efad297a9b0ef..da6198f909db82 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs @@ -68,7 +68,8 @@ internal Dictionary(int capacity, IEqualityComparer? comparer, bool usePro { _comparer = comparer ?? EqualityComparer.Default; - if (!useProvidedComparer) { + if (!useProvidedComparer) + { // Special-case EqualityComparer.Default, StringComparer.Ordinal, and StringComparer.OrdinalIgnoreCase. // We use a non-randomized comparer for improved perf, falling back to a randomized comparer if the // hash buckets become unbalanced. diff --git a/src/mono/mono/metadata/appdomain.c b/src/mono/mono/metadata/appdomain.c index 1426986e55e648..18dca251ea7cde 100644 --- a/src/mono/mono/metadata/appdomain.c +++ b/src/mono/mono/metadata/appdomain.c @@ -880,9 +880,9 @@ mono_runtime_install_appctx_properties (void) for (int i = 0; i < n_appctx_props; ++i) { glong num_chars; combined_keys [i] = g_utf8_to_utf16 (appctx_keys [i], -1, NULL, &num_chars, NULL); - combined_key_lengths[i] = GLONG_TO_UINT32(num_chars); + combined_key_lengths [i] = GLONG_TO_UINT32 (num_chars); combined_values [i] = g_utf8_to_utf16 (appctx_values [i], -1, NULL, &num_chars, NULL); - combined_value_lengths[i] = GLONG_TO_UINT32(num_chars); + combined_value_lengths [i] = GLONG_TO_UINT32 (num_chars); } runtimeconfig_json_read_props ( From 0bf8e17679a2c5a7f445760a7637610375592e20 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Mon, 22 Apr 2024 19:50:45 -0700 Subject: [PATCH 4/7] Undo dictionary changes --- .../src/System/AppContext.cs | 18 +----------------- .../System/Collections/Generic/Dictionary.cs | 19 +++++++------------ 2 files changed, 8 insertions(+), 29 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/AppContext.cs b/src/libraries/System.Private.CoreLib/src/System/AppContext.cs index 341c2daa515682..23cfae81faf0f9 100644 --- a/src/libraries/System.Private.CoreLib/src/System/AppContext.cs +++ b/src/libraries/System.Private.CoreLib/src/System/AppContext.cs @@ -152,26 +152,10 @@ internal static unsafe void Setup(char** pNames, char** pValues, int count) } } - private sealed class AppContextKeyComparer : IEqualityComparer - { - public bool Equals(string? x, string? y) => string.Equals(x, y); - - public int GetHashCode(string? obj) - { - Debug.Assert(obj != null, "This implementation is only called from first-party collection types that guarantee non-null parameters."); - return obj.GetNonRandomizedHashCode(); - } - } - internal static unsafe void Setup(char** pNames, uint** pNameLengths, char** pValues, uint** pValueLengths, int count) { Debug.Assert(s_dataStore == null, "s_dataStore is not expected to be inited before Setup is called"); - // HACK: Use a special comparer that does not pull in all the dependencies that - // the default BCL string comparers pull in - IEqualityComparer comparer = new AppContextKeyComparer(); - // HACK: Use a special version of the Dictionary constructor that does not pull in a - // large amount of extra code in order to instantiate this dictionary - s_dataStore = new Dictionary(count, comparer, true); + s_dataStore = new Dictionary(count); for (int i = 0; i < count; i++) { s_dataStore.Add(new string(pNames[i], 0, (int)pNameLengths[i]), new string(pValues[i], 0, (int)pValueLengths[i])); diff --git a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs index da6198f909db82..9b18d4a125ed21 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs @@ -43,9 +43,7 @@ public Dictionary(int capacity) : this(capacity, null) { } public Dictionary(IEqualityComparer? comparer) : this(0, comparer) { } - public Dictionary(int capacity, IEqualityComparer? comparer) : this(0, comparer, false) { } - - internal Dictionary(int capacity, IEqualityComparer? comparer, bool useProvidedComparer) + public Dictionary(int capacity, IEqualityComparer? comparer) { if (capacity < 0) { @@ -68,16 +66,13 @@ internal Dictionary(int capacity, IEqualityComparer? comparer, bool usePro { _comparer = comparer ?? EqualityComparer.Default; - if (!useProvidedComparer) + // Special-case EqualityComparer.Default, StringComparer.Ordinal, and StringComparer.OrdinalIgnoreCase. + // We use a non-randomized comparer for improved perf, falling back to a randomized comparer if the + // hash buckets become unbalanced. + if (typeof(TKey) == typeof(string) && + NonRandomizedStringEqualityComparer.GetStringComparer(_comparer!) is IEqualityComparer stringComparer) { - // Special-case EqualityComparer.Default, StringComparer.Ordinal, and StringComparer.OrdinalIgnoreCase. - // We use a non-randomized comparer for improved perf, falling back to a randomized comparer if the - // hash buckets become unbalanced. - if (typeof(TKey) == typeof(string) && - NonRandomizedStringEqualityComparer.GetStringComparer(_comparer!) is IEqualityComparer stringComparer) - { - _comparer = (IEqualityComparer)stringComparer; - } + _comparer = (IEqualityComparer)stringComparer; } } else if (comparer is not null && // first check for null to avoid forcing default comparer instantiation unnecessarily From a2a80ad796cb2cce363c31246a2d202c89587e0e Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Wed, 24 Apr 2024 12:40:58 -0700 Subject: [PATCH 5/7] Fix method signature --- src/libraries/System.Private.CoreLib/src/System/AppContext.cs | 2 +- src/mono/mono/metadata/appdomain.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/AppContext.cs b/src/libraries/System.Private.CoreLib/src/System/AppContext.cs index 23cfae81faf0f9..b7756bb3f6669e 100644 --- a/src/libraries/System.Private.CoreLib/src/System/AppContext.cs +++ b/src/libraries/System.Private.CoreLib/src/System/AppContext.cs @@ -152,7 +152,7 @@ internal static unsafe void Setup(char** pNames, char** pValues, int count) } } - internal static unsafe void Setup(char** pNames, uint** pNameLengths, char** pValues, uint** pValueLengths, int count) + internal static unsafe void Setup(char** pNames, uint* pNameLengths, char** pValues, uint* pValueLengths, int count) { Debug.Assert(s_dataStore == null, "s_dataStore is not expected to be inited before Setup is called"); s_dataStore = new Dictionary(count); diff --git a/src/mono/mono/metadata/appdomain.c b/src/mono/mono/metadata/appdomain.c index 18dca251ea7cde..fc12cc9a21675c 100644 --- a/src/mono/mono/metadata/appdomain.c +++ b/src/mono/mono/metadata/appdomain.c @@ -891,7 +891,7 @@ mono_runtime_install_appctx_properties (void) combined_values + n_appctx_props, combined_value_lengths + n_appctx_props ); - /* internal static unsafe void Setup_Mono(char** pNames, uint** pNameLengths, char** pValues, uint** pValueLengths, int count) */ + /* internal static unsafe void Setup(char** pNames, uint* pNameLengths, char** pValues, uint* pValueLengths, int count) */ args [0] = combined_keys; args [1] = combined_key_lengths; args [2] = combined_values; From 3d652bd2dbc7918c66fb13f7cfddc9298d05408c Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Wed, 24 Apr 2024 14:20:29 -0700 Subject: [PATCH 6/7] Fix appctx property lengths including the null terminator --- src/mono/mono/metadata/appdomain.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/mono/mono/metadata/appdomain.c b/src/mono/mono/metadata/appdomain.c index fc12cc9a21675c..f0a7789108d552 100644 --- a/src/mono/mono/metadata/appdomain.c +++ b/src/mono/mono/metadata/appdomain.c @@ -880,9 +880,10 @@ mono_runtime_install_appctx_properties (void) for (int i = 0; i < n_appctx_props; ++i) { glong num_chars; combined_keys [i] = g_utf8_to_utf16 (appctx_keys [i], -1, NULL, &num_chars, NULL); - combined_key_lengths [i] = GLONG_TO_UINT32 (num_chars); + // HACK: items_written from g_utf8_to_utf16 includes the null terminator unless you pass an explicit length. + combined_key_lengths [i] = GLONG_TO_UINT32 (num_chars ? num_chars - 1 : 0); combined_values [i] = g_utf8_to_utf16 (appctx_values [i], -1, NULL, &num_chars, NULL); - combined_value_lengths [i] = GLONG_TO_UINT32 (num_chars); + combined_value_lengths [i] = GLONG_TO_UINT32 (num_chars ? num_chars - 1 : 0); } runtimeconfig_json_read_props ( From 1ea8882409d45556c6d0260b786e3e0167a8ea03 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Mon, 29 Apr 2024 15:38:15 -0700 Subject: [PATCH 7/7] Address PR feedback --- .../System.Private.CoreLib/src/System/AppContext.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/AppContext.cs b/src/libraries/System.Private.CoreLib/src/System/AppContext.cs index b7756bb3f6669e..d0ab3305bfd4bc 100644 --- a/src/libraries/System.Private.CoreLib/src/System/AppContext.cs +++ b/src/libraries/System.Private.CoreLib/src/System/AppContext.cs @@ -141,24 +141,24 @@ public static void SetSwitch(string switchName, bool isEnabled) } } -#if !NATIVEAOT - internal static unsafe void Setup(char** pNames, char** pValues, int count) +#if MONO + internal static unsafe void Setup(char** pNames, uint* pNameLengths, char** pValues, uint* pValueLengths, int count) { Debug.Assert(s_dataStore == null, "s_dataStore is not expected to be inited before Setup is called"); s_dataStore = new Dictionary(count); for (int i = 0; i < count; i++) { - s_dataStore.Add(new string(pNames[i]), new string(pValues[i])); + s_dataStore.Add(new string(pNames[i], 0, (int)pNameLengths[i]), new string(pValues[i], 0, (int)pValueLengths[i])); } } - - internal static unsafe void Setup(char** pNames, uint* pNameLengths, char** pValues, uint* pValueLengths, int count) +#elif !NATIVEAOT + internal static unsafe void Setup(char** pNames, char** pValues, int count) { Debug.Assert(s_dataStore == null, "s_dataStore is not expected to be inited before Setup is called"); s_dataStore = new Dictionary(count); for (int i = 0; i < count; i++) { - s_dataStore.Add(new string(pNames[i], 0, (int)pNameLengths[i]), new string(pValues[i], 0, (int)pValueLengths[i])); + s_dataStore.Add(new string(pNames[i]), new string(pValues[i])); } } #endif