GetEnvironmentVariable: Avoid StringBuilder marshaling overhead #14502

justinvp · 2017-10-14T00:30:37Z

Try a stack allocated buffer first, then fallback to using the shared array pool.

Results on my machine:

Method	Mean	Error	StdDev	Gen 0	Allocated
`"TMP"` Before	156.51 ns	2.0132 ns	1.8831 ns	0.0875	368 B
`"TMP"` After	92.88 ns	0.5002 ns	0.4434 ns	0.0209	88 B
`"PATH"` Before	621.59 ns	2.2668 ns	2.1204 ns	0.9565	4016 B
`"PATH"` After	287.02 ns	1.0377 ns	0.8666 ns	0.2704	1136 B

"TMP" is under the stack limit and "PATH" is over the limit (uses the pool).

Try a stack allocated buffer first, then fallback to using the shared array pool.

danmoseley · 2017-10-15T06:04:23Z

src/mscorlib/src/System/Environment.cs

        {
-            StringBuilder sb = StringBuilderCache.Acquire(128); // A somewhat reasonable default size
-            int requiredSize = Win32Native.GetEnvironmentVariable(variable, sb, sb.Capacity);
+            Span<char> buffer = stackalloc char[128]; // A somewhat reasonable default size


is it reasonable? since this is now a stack allocation, maybe it's cheap to be more generous? although, this is probably fine for most variables except path, include, libpath, etc.

@stephentoub do we have a rule of thumb for reasonable stackalloc max? In the CoreFX code I see mostly a few bytes, a cutoff of 100, 200, one of 256 is the largest.

I had the same question. 128 chars (256 bytes) seemed like a reasonable limit... but I could see going a little higher. Not sure how far we want to push it, though.

We don't have a hard and fast rule, but we've generally stayed at <= 256 bytes.

danmoseley · 2017-10-15T06:06:58Z

src/mscorlib/src/System/Environment.cs

+            int requiredSize = Win32Native.GetEnvironmentVariable(variable, buffer);

-            if (requiredSize == 0 && Marshal.GetLastWin32Error() == Win32Native.ERROR_ENVVAR_NOT_FOUND)
+            if (requiredSize > buffer.Length)


confirmed that GetEnvironmentVariable returns the length including terminating null. StringBuilder.Capacity does not include terminating null so that would not have mattered before.

danmoseley · 2017-10-15T06:08:59Z

src/mscorlib/src/Microsoft/Win32/Win32Native.cs


        [DllImport(Interop.Libraries.Kernel32, CharSet = CharSet.Auto, SetLastError = true, BestFitMapping = false)]
-        internal static extern int GetEnvironmentVariable(string lpName, [Out]StringBuilder lpValue, int size);
+        private static extern unsafe int GetEnvironmentVariable(string lpName, char* lpValue, int size);


you've also eliminated the StringBuilder marshaling it was doing before, which forced a copy.
https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/interop-pinvokes.md

danmoseley

Looks good to me - thanks! I'll let @stephentoub have a quick look in case I missed some subtlety.

danmoseley · 2017-10-15T06:14:29Z

@dotnet-bot test Windows_NT x86 full_opt legacy_backend CoreCLR Perf Tests Correctness please (nolog)

jkotas · 2017-10-16T16:52:43Z

src/mscorlib/src/System/Environment.cs

-                sb.Capacity = requiredSize;
-                sb.Length = 0;
-                requiredSize = Win32Native.GetEnvironmentVariable(variable, sb, sb.Capacity);
+                return Marshal.GetLastWin32Error() == Win32Native.ERROR_ENVVAR_NOT_FOUND ? null : string.Empty;


Nit: I would move the check for error condition right after the PInvoke to make it easier to see that there is nothing modifying Marshal.GetLastWin32Error().

Done. I also changed the condition back to the way it was as the string ctor will take care of returning string.Empty for empty spans (when requiredSize == 0 and the error isn’t “not found”).

justinvp · 2017-10-16T18:55:08Z

I had added clearArray: true when returning the array to the pool out of caution and it didn't appear to significantly impact the perf, but re-running the microbenchmarks again does show a slowdown.

clearArray: true:

Method	Mean	Error	StdDev	Gen 0	Allocated
Before	702.23 ns	2.0626 ns	1.8284 ns	1.1282	4736 B
After	723.63 ns	2.8812 ns	2.5541 ns	0.3271	1376 B

clearArray: false:

Method	Mean	Error	StdDev	Gen 0	Allocated
Before	684.89 ns	3.7051 ns	3.2845 ns	1.1282	4736 B
After	316.44 ns	1.9137 ns	1.7901 ns	0.3276	1376 B

Thus, I've added a commit that removes clearArray: true since:

Any code in the process can already read environment variables whenever it wants
Clearing the array before returning it to the pool is not done anywhere else in corelib
CoreRT's ExpandEnvironmentVariables implementation uses the pool and doesn't clear the array

Let me know if you have any concerns

GetEnvironmentVariable: Avoid StringBuilder marshaling overhead

630f362

Try a stack allocated buffer first, then fallback to using the shared array pool.

dnfclas added the cla-already-signed label Oct 14, 2017

danmoseley reviewed Oct 15, 2017

View reviewed changes

danmoseley approved these changes Oct 15, 2017

View reviewed changes

jkotas reviewed Oct 16, 2017

View reviewed changes

Address feedback

05bf177

justinvp force-pushed the getenvironmentvariable branch from 22146e1 to 05bf177 Compare October 16, 2017 18:06

Don't clear the array when returning it to the pool

a8ba2f4

jkotas approved these changes Oct 17, 2017

View reviewed changes

jkotas merged commit 2f01c28 into dotnet:master Oct 17, 2017

justinvp deleted the getenvironmentvariable branch October 17, 2017 21:39

Ruikuan mentioned this pull request Nov 28, 2018

.Net Core 2.1 性能改进 Ruikuan/blog#26

Closed

GetEnvironmentVariable: Avoid StringBuilder marshaling overhead #14502

GetEnvironmentVariable: Avoid StringBuilder marshaling overhead #14502

Uh oh!

Conversation

justinvp commented Oct 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmoseley left a comment

Choose a reason for hiding this comment

Uh oh!

danmoseley commented Oct 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinvp commented Oct 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants