From bf302de7f184c6e9dc23382980860af2849219e3 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Sun, 22 Nov 2020 17:59:47 -0500 Subject: [PATCH] Improve throughput of Environment.GetEnvironmentVariables() (#45057) Use IndexOf to search for positions rather than open-coded loops, taking advantage of vectorization to improve throughput. --- src/coreclr/src/vm/ecalllist.h | 4 +- .../Interop.FreeEnvironmentStrings.cs | 4 +- .../Kernel32/Interop.GetEnvironmentStrings.cs | 4 +- .../System/Environment.Variables.Windows.cs | 105 +++++++----------- 4 files changed, 46 insertions(+), 71 deletions(-) diff --git a/src/coreclr/src/vm/ecalllist.h b/src/coreclr/src/vm/ecalllist.h index 87389732f8d2e..f66c599a06209 100644 --- a/src/coreclr/src/vm/ecalllist.h +++ b/src/coreclr/src/vm/ecalllist.h @@ -1050,8 +1050,8 @@ FCFuncStart(gPalKernel32Funcs) QCFuncElement("CreateMutexEx", CreateMutexExW) QCFuncElement("CreateSemaphoreEx", CreateSemaphoreExW) QCFuncElement("FormatMessage", FormatMessageW) - QCFuncElement("FreeEnvironmentStrings", FreeEnvironmentStringsW) - QCFuncElement("GetEnvironmentStrings", GetEnvironmentStringsW) + QCFuncElement("FreeEnvironmentStringsW", FreeEnvironmentStringsW) + QCFuncElement("GetEnvironmentStringsW", GetEnvironmentStringsW) QCFuncElement("GetEnvironmentVariable", GetEnvironmentVariableW) QCFuncElement("OpenEvent", OpenEventW) QCFuncElement("OpenMutex", OpenMutexW) diff --git a/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.FreeEnvironmentStrings.cs b/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.FreeEnvironmentStrings.cs index f9765e66d4d00..5af20c071ca1d 100644 --- a/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.FreeEnvironmentStrings.cs +++ b/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.FreeEnvironmentStrings.cs @@ -7,7 +7,7 @@ internal static partial class Interop { internal static partial class Kernel32 { - [DllImport(Libraries.Kernel32, EntryPoint = "FreeEnvironmentStringsW", SetLastError = true, CharSet = CharSet.Unicode, BestFitMapping = false, ExactSpelling = true)] - internal static extern unsafe bool FreeEnvironmentStrings(char* lpszEnvironmentBlock); + [DllImport(Libraries.Kernel32, ExactSpelling = true)] + internal static extern unsafe BOOL FreeEnvironmentStringsW(char* lpszEnvironmentBlock); } } diff --git a/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.GetEnvironmentStrings.cs b/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.GetEnvironmentStrings.cs index 4fcdbe0c2e767..55452013da027 100644 --- a/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.GetEnvironmentStrings.cs +++ b/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.GetEnvironmentStrings.cs @@ -7,7 +7,7 @@ internal static partial class Interop { internal static partial class Kernel32 { - [DllImport(Libraries.Kernel32, EntryPoint = "GetEnvironmentStringsW", SetLastError = true, CharSet = CharSet.Unicode, BestFitMapping = false, ExactSpelling = true)] - internal static extern unsafe char* GetEnvironmentStrings(); + [DllImport(Libraries.Kernel32, ExactSpelling = true)] + internal static extern unsafe char* GetEnvironmentStringsW(); } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Environment.Variables.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/Environment.Variables.Windows.cs index 15f027c202e65..382021ea055ca 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Environment.Variables.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Environment.Variables.Windows.cs @@ -58,93 +58,68 @@ private static void SetEnvironmentVariableCore(string variable, string? value) public static unsafe IDictionary GetEnvironmentVariables() { - char* pStrings = Interop.Kernel32.GetEnvironmentStrings(); - if (pStrings == null) + // Format for GetEnvironmentStrings is: + // [=HiddenVar=value\0]* [Variable=value\0]* \0 + // See the description of Environment Blocks in MSDN's CreateProcess + // page (null-terminated array of null-terminated strings). Note + // the =HiddenVar's aren't always at the beginning. + + // Copy strings out, parsing into pairs and inserting into the table. + // The first few environment variable entries start with an '='. + // The current working directory of every drive (except for those drives + // you haven't cd'ed into in your DOS window) are stored in the + // environment block (as =C:=pwd) and the program's exit code is + // as well (=ExitCode=00000000). + + char* stringPtr = Interop.Kernel32.GetEnvironmentStringsW(); + if (stringPtr == null) { throw new OutOfMemoryException(); } try { - // Format for GetEnvironmentStrings is: - // [=HiddenVar=value\0]* [Variable=value\0]* \0 - // See the description of Environment Blocks in MSDN's - // CreateProcess page (null-terminated array of null-terminated strings). - - // Search for terminating \0\0 (two unicode \0's). - char* p = pStrings; - while (!(*p == '\0' && *(p + 1) == '\0')) - { - p++; - } - Span block = new Span(pStrings, (int)(p - pStrings + 1)); - - // Format for GetEnvironmentStrings is: - // (=HiddenVar=value\0 | Variable=value\0)* \0 - // See the description of Environment Blocks in MSDN's - // CreateProcess page (null-terminated array of null-terminated strings). - // Note the =HiddenVar's aren't always at the beginning. - - // Copy strings out, parsing into pairs and inserting into the table. - // The first few environment variable entries start with an '='. - // The current working directory of every drive (except for those drives - // you haven't cd'ed into in your DOS window) are stored in the - // environment block (as =C:=pwd) and the program's exit code is - // as well (=ExitCode=00000000). - var results = new Hashtable(); - for (int i = 0; i < block.Length; i++) - { - int startKey = i; - // Skip to key. On some old OS, the environment block can be corrupted. - // Some will not have '=', so we need to check for '\0'. - while (block[i] != '=' && block[i] != '\0') + char* currentPtr = stringPtr; + while (true) + { + int variableLength = string.wcslen(currentPtr); + if (variableLength == 0) { - i++; + break; } - if (block[i] == '\0') - { - continue; - } + var variable = new ReadOnlySpan(currentPtr, variableLength); - // Skip over environment variables starting with '=' - if (i - startKey == 0) + // Find the = separating the key and value. We skip entries that begin with =. We also skip entries that don't + // have =, which can happen on some older OSes when the environment block gets corrupted. + int i = variable.IndexOf('='); + if (i > 0) { - while (block[i] != 0) + // Add the key and value. + string key = new string(variable.Slice(0, i)); + string value = new string(variable.Slice(i + 1)); + try { - i++; + // Add may throw if the environment block was corrupted leading to duplicate entries. + // We allow such throws and eat them (rather than proactively checking for duplication) + // to provide a non-fatal notification about the corruption. + results.Add(key, value); } - - continue; + catch (ArgumentException) { } } - string key = new string(block.Slice(startKey, i - startKey)); - i++; // skip over '=' - - int startValue = i; - while (block[i] != 0) - { - i++; // Read to end of this entry - } - - string value = new string(block.Slice(startValue, i - startValue)); // skip over 0 handled by for loop's i++ - try - { - results.Add(key, value); - } - catch (ArgumentException) - { - // Throw and catch intentionally to provide non-fatal notification about corrupted environment block - } + // Move to the end of this variable, after its terminator. + currentPtr += variableLength + 1; } + return results; } finally { - bool success = Interop.Kernel32.FreeEnvironmentStrings(pStrings); - Debug.Assert(success); + Interop.BOOL success = Interop.Kernel32.FreeEnvironmentStringsW(stringPtr); + Debug.Assert(success != Interop.BOOL.FALSE); } } }