Fix ServiceController name population perf #32072

danmoseley · 2018-09-02T05:26:52Z

Fix https://github.com/dotnet/corefx/issues/31877

Regression was caused by an unexplained change in Project K in 2014 to populate name and display name by enumerating every service on the machine examining their names. Revert back to use the direct Win32 API calls. Modernize the interop.
Tidy up some strings.
Write tests to get code coverage of various name population codepaths.
Fix several existing issues uncovered by tests, in some cases introduced since .NET Framework.
Inline one method for clarity.

BenchmarkDotNet=v0.11.1, OS=Windows 10.0.17134.228 (1803/April2018Update/Redstone4)
Intel Core i7-2760QM CPU 2.40GHz (Sandy Bridge), 1 CPU, 8 logical and 4 physical cores
Frequency=2336175 Hz, Resolution=428.0501 ns, Timer=TSC
.NET Core SDK=2.1.400
  [Host] : .NET Core 2.1.2 (CoreCLR 4.6.26628.05, CoreFX 4.6.26629.01), 64bit RyuJIT
  Clr    : .NET Framework 4.7.2 (CLR 4.0.30319.42000), 64bit RyuJIT-v4.7.3132.0
  Core   : .NET Core 2.1.2 (CoreCLR 4.6.26628.05, CoreFX 4.6.26629.01), 64bit RyuJIT

Method	Job	Runtime	Mean	Error	StdDev
GetStatus	Clr	Clr	195.6 us	3.8735 us	9.8594 us
GetStatus	Core	Core	2,588.3 us	14.756 us	11.521 us
GetStatusFixed	Core	Core	181.6 us	0.7662 us	0.6398 us

I also noticed that .NET Framework was using raw IntPtr for the SCM handle but apparently that doesn't help it significantly:

Method	Mean	Error	StdDev
IntPtr	186.8 us	2.791 us	2.611 us
SafeServiceHandle	186.2 us	2.077 us	1.842 us

danmoseley · 2018-09-02T05:27:06Z

@PaulHigin

danmoseley · 2018-09-02T05:36:40Z

@SteveL-MSFT

jkotas · 2018-09-02T11:45:43Z

src/System.ServiceProcess.ServiceController/src/System/ServiceProcess/ServiceController.cs

                {
-                    Interop.Advapi32.CloseServiceHandle(databaseHandle);
+                    Exception inner = new Win32Exception();


There is a bunch of code between GetServiceDisplayName and this place. It may corrupt the last error. Last error should be captured right after the PInvoke returns. (It may look better to have the buffer marshaling here and not under Common.)

jkotas · 2018-09-02T11:46:07Z

src/System.ServiceProcess.ServiceController/src/System/ServiceProcess/ServiceController.cs

-                Marshal.FreeHGlobal(memory);
-                if (databaseHandle != IntPtr.Zero)
+                // We must have _name
+                string result = Interop.Advapi32.GetServiceDisplayName(_serviceManagerHandle.DangerousGetHandle(), _name);


Is this really so performance critical that you have to use DangerousGetHandle here?

No, was following existing pattern. I will SafeHandle all the things.

jkotas · 2018-09-02T11:57:55Z

src/Common/src/Interop/Windows/advapi32/Interop.GetServiceDisplayName.cs

+        {
+            // Get the size of buffer required
+            int bufLen = 0;
+            bool success = GetServiceDisplayNamePrivate(SCMHandle, serviceName, null, ref bufLen);


The display name can change. Do we need to re-loop to handle races?

It may look better with ValueStringBuilder:

var builder = new ValueStringBuilder(4096); int bufLen; for (;;) { fixed (char * pBuffer = builder.GetPinnableReference()) { bufLen = builder.Capacity; if (GetServiceDisplayNamePrivate(SCMHandle, serviceName, pBuffer, ref int bufLen)) break; } int lastError = Marshal.GetLastWin32Error(); if (lastError != Interop.Errors.ERROR_INSUFFICIENT_BUFFER) ... throw exception ... builder.EnsureCapacity(bufLen + 1); // Does not include null } builder.Length = bufLen; return builder.ToString();

Notice that it is calling GetServiceDisplayName just once in the typical case, so it should be more efficient as well.

jkotas · 2018-09-02T12:08:37Z

src/System.ServiceProcess.ServiceController/src/System/ServiceProcess/ServiceController.cs

@@ -38,7 +39,6 @@ public class ServiceController : Component
        private ServiceStartMode _startType;

        private const int SERVICENAMEMAXLENGTH = 80;


This constant is unused as well. Delete?

jkotas · 2018-09-02T12:12:58Z

src/System.ServiceProcess.ServiceController/src/System/ServiceProcess/ServiceController.cs

@@ -449,12 +449,6 @@ private static bool CheckMachineName(string value)
        }

        public void Close()


This change is going to change the behavior with user-overiden Dispose methods. Implementing Close by calling Dispose is fine - not sure why you have changed it.

To match NETFX. It calls Close() to reset state, without setting disposed, for example when names need to be regenerated. Once disposed is set, the class can no longer be used. Do you still want me to change it? I suppose I could just rename it Reset().

It violates design guidelines for Close method. From https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/dispose-pattern:

CONSIDER providing method Close(), in addition to the Dispose(), if close is standard terminology in the area. When doing so, it is important that you make the Close implementation identical to Dispose

I am sure what the right thing to do here is ... but it would be nice to add a comment about design guidelines violation at least if you keep it as it used to be in netfx.

jkotas · 2018-09-02T12:23:04Z

src/System.ServiceProcess.ServiceController/src/System/ServiceProcess/ServiceController.cs

-        private string _displayName;
+        private string _name = ""; // Never null
+        private string _eitherName = ""; // Never null
+        private string _displayName = ""; // Never null


This has potential to create corner case bugs when the service name is empty string. Using null to mean not-yet-computed looks better.

jkotas · 2018-09-02T12:34:31Z

src/System.ServiceProcess.ServiceController/src/System/ServiceProcess/ServiceController.cs

@@ -19,13 +19,14 @@ namespace System.ServiceProcess
    /// and manipulate it or get information about it.
    public class ServiceController : Component
    {
-        private string _machineName;
+        private string _machineName = DefaultMachineName; // Never null, always a valid name


var n = new ServiceController().DisplayName; would throw NullReferenceException before this change, but succeeds after this change. Do we need a test for this?

Actually it throws IOE now, but I'll add a test.

jkotas · 2018-09-02T12:35:18Z

src/System.ServiceProcess.ServiceController/src/System/ServiceProcess/ServiceController.cs

@@ -19,13 +19,14 @@ namespace System.ServiceProcess
    /// and manipulate it or get information about it.
    public class ServiceController : Component
    {
-        private string _machineName;
+        private string _machineName = DefaultMachineName; // Never null, always a valid name


Since the _machineName is set in all but one constructor - it may be better to just fix the one constructor that is not setting it to set it.

danmoseley · 2018-09-03T00:15:06Z

Updated.

danmoseley · 2018-09-03T00:18:27Z

src/System.ServiceProcess.ServiceController/src/System/ServiceProcess/ServiceBase.cs

-            IntPtr structPtr;
-
-            for (int index = 0; index < services.Length; ++index)
+            using (var entriesPointer = new SafeAllocHHandle(Marshal.AllocHGlobal(checked((services.Length + 1) * sizeOfSERVICE_TABLE_ENTRY))))


Seems like it might be nice to add SafeAllocHHandle.CreateNewAlloc(int size)

jkotas · 2018-09-03T15:53:45Z

src/Common/src/Interop/Windows/advapi32/Interop.GetServiceKeyName.cs

+                    if (throwOnError)
+                        throw new Win32Exception(lastError);
+
+                    return null; // Caller may want to try something else


If this is expected to be a "common" path, this should dispose the buffer.

jkotas · 2018-09-03T15:55:47Z

src/Common/src/Interop/Windows/advapi32/Interop.GetServiceKeyName.cs

+        [DllImport(Libraries.Advapi32, EntryPoint = "GetServiceKeyNameW", CharSet = System.Runtime.InteropServices.CharSet.Unicode, SetLastError = true)]
+        private static extern bool GetServiceKeyNamePrivate(SafeServiceHandle SCMHandle, string displayName, ref char KeyName, ref int KeyNameLength);
+
+        public static unsafe string GetServiceKeyName(SafeServiceHandle SCMHandle, string displayName, bool throwOnError)


I think the throwOnError is over-engineered since this is always used with the same value.

It is why we typically have the wrapper code like this in implementation, and not under Common.

jkotas · 2018-09-03T16:16:30Z

src/System.ServiceProcess.ServiceController/src/System/ServiceProcess/ServiceController.cs

-                        IntPtr enumBuffer = Marshal.AllocHGlobal((IntPtr)bytesNeeded);
-
-                        try
+                        using (var enumBuffer = new SafeAllocHHandle(Marshal.AllocHGlobal((IntPtr)bytesNeeded)))


The pattern where both alloc and free is done within same function and the thing does not escape anywhere is fine without safe handles. Safe handles just add overhead, without really improving safety.

Safe handles are worth it for the extra safety when the handle is object state (stored in a field). Safe handles prevent situation where some other thread frees it while it is being used.

Reversed, but if not a SafeHandle a disposable holder seems like it might have helped avoid the leak in Run() in ServiceBase.

danmoseley · 2018-09-03T17:57:47Z

Updated.

danmoseley · 2018-09-03T19:34:53Z

@dotnet-bot test UWP CoreCLR x64 Debug Build

jkotas · 2018-09-04T00:12:23Z

src/Common/src/Interop/Windows/advapi32/Interop.GetServiceDisplayName.cs

+    internal partial class Advapi32
+    {
+        [DllImport(Libraries.Advapi32, EntryPoint = "GetServiceDisplayNameW", CharSet = System.Runtime.InteropServices.CharSet.Unicode, SetLastError = true)]
+        internal static extern bool GetServiceDisplayName(SafeServiceHandle SCMHandle, string serviceName, ref char displayName, ref int displayNameLength);


I think it is better to use char* for string pointers in PInvoke signatures. ref char obfuscates the intent. It is hard to see that it is actually pointer to string.

Changed, but I was following the pattern used in Environment and Path. It avoids the fixed statement?

We are not consistent with this.

cc @JeremyKuhne - we should probably be consistent.

I think we had settled on not using ref char instead of char* to potentially avoid an extra frame in addition to clarity. In this case we're stuck with extra frames no matter what due to the safe handle.

On a side note: one weird gotcha we have in this space is that pinning an empty array behaves differently than pinning an empty span.

static unsafe void PinMe() { int[] emptyArray = new int[0]; Span<int> emptySpan = new Span<int>(emptyArray); fixed (int* ea = emptyArray) fixed (int* es = emptySpan) fixed (int* esr = &MemoryMarshal.GetReference(emptySpan)) { Console.WriteLine($"Array: 0x{(ulong)ea:X}, Span: 0x{(ulong)es:X}, Address of ref span: 0x{(ulong)esr:X}"); } }

Yields something like:

Array: 0x0, Span: 0x0, Address of ref span: 0x27D97CD1588

If the source array actually had length, these would all return the same pointer. This actually generated regressions in System.Drawing as GDI+ cared about the pointer in APIs when we sent the pointer and a length of zero (user passed us no points and we passed no points along to GDI+).

pinning an empty array behaves differently than pinning an empty span.

Your example shows that pinning empty array works same way as pinning empty span.

It is only when you use the low-level MemoryMarshal.GetReference API that you will see the difference. We have a number of places in CoreFX that use MemoryMarshal.GetReference to pin Span that were introduced before we have proper support for pinning Span. We should move away from using MemoryMarshal.GetReference for pinning Span.

Your example shows that pinning empty array works same way as pinning empty span.

Yes, I should have been clearer. &MemoryMarshal.GetReference was the only "pinning" option prior to C# 7.3. I'll open a tracking issue to pull usages.

@jkotas Incidentally, does using ref char instead of char* have the same impact with the marshaller as MemoryMarshal.GetReference? I'll validate when I get some time.

Should we make a breaking change to MemoryMarshal.GetReference to change to the ideal behavior? I imagine not much code has been written to use it on spans yet.

MemoryMarshal.GetReference was designed to give you the Span pointer with zero overhead and without any normalization.

If we added normalization to it, it would make it impossible to write low-level high perf code that works on Span. I am pretty sure number of our micro-benchmarks would regress.

jkotas · 2018-09-04T00:20:33Z

src/System.ServiceProcess.ServiceController/src/System/ServiceProcess/ServiceController.cs

-            try
-            {
+            using (var serviceHandle = GetServiceHandle(Interop.Advapi32.ServiceOptions.SERVICE_PAUSE_CONTINUE))
+                {


Nit: Indentation

danmoseley · 2018-09-04T03:19:55Z

Updated.

jkotas · 2018-09-04T06:03:45Z

src/System.ServiceProcess.ServiceController/src/System/ServiceProcess/ServiceController.cs

+            while (true)
+            {
+                bufLen = builder.Capacity;
+                fixed (char* c = &builder.GetPinnableReference())


I think this can be just fixed (char* c = builder) with the latest C# compiler.

I had to add a parameterless overload to ValueStringBuilder to achieve this useful trick.

danmoseley · 2018-09-04T16:11:55Z

Thanks for all the reviews!

safern · 2018-09-04T17:23:34Z

src/System.ServiceProcess.ServiceController/src/Resources/Strings.resx

  </data>
  <data name="ServiceRemoved" xml:space="preserve">
-    <value>Service {0} was successfully removed from the system.</value>
+    <value>Service '{0}'' was successfully removed from the system.</value>


Nit: extra '

Good eye, thanks!

* Fix ServiceController name population perf * Split tests * Remove dead field * Remove new use of DangerousGetHandle * SafeHandle all the things! * VSB dotnet#1 * VSB dotnet#2 * Fix GLE * Initialize machineName in ctor * Test for empty name ex * Null names * Inadvertent edit * Unix build * Move interop into class * Reverse SafeHandle for HAllocGlobal * Fix tests * Disable test for NETFX * CR feedback * Pattern matching on VSB * Direct call * typo Signed-off-by: dotnet-bot <[email protected]>

* Fix ServiceController name population perf * Split tests * Remove dead field * Remove new use of DangerousGetHandle * SafeHandle all the things! * VSB #1 * VSB dotnet#2 * Fix GLE * Initialize machineName in ctor * Test for empty name ex * Null names * Inadvertent edit * Unix build * Move interop into class * Reverse SafeHandle for HAllocGlobal * Fix tests * Disable test for NETFX * CR feedback * Pattern matching on VSB * Direct call * typo Signed-off-by: dotnet-bot <[email protected]>

* Fix ServiceController name population perf * Split tests * Remove dead field * Remove new use of DangerousGetHandle * SafeHandle all the things! * VSB #1 * VSB #2 * Fix GLE * Initialize machineName in ctor * Test for empty name ex * Null names * Inadvertent edit * Unix build * Move interop into class * Reverse SafeHandle for HAllocGlobal * Fix tests * Disable test for NETFX * CR feedback * Pattern matching on VSB * Direct call * typo Signed-off-by: dotnet-bot <[email protected]>

* Fix ServiceController name population perf * Split tests * Remove dead field * Remove new use of DangerousGetHandle * SafeHandle all the things! * VSB dotnet/corefx#1 * VSB dotnet/corefx#2 * Fix GLE * Initialize machineName in ctor * Test for empty name ex * Null names * Inadvertent edit * Unix build * Move interop into class * Reverse SafeHandle for HAllocGlobal * Fix tests * Disable test for NETFX * CR feedback * Pattern matching on VSB * Direct call * typo Commit migrated from dotnet/corefx@76c8587

Fix ServiceController name population perf

4738fbf

Split tests

7f169b4

jkotas suggested changes Sep 2, 2018

View reviewed changes

danmoseley added 10 commits September 2, 2018 14:46

Remove dead field

34b6d9f

Remove new use of DangerousGetHandle

aca1848

SafeHandle all the things!

04d0c1a

VSB #1

bd9db82

VSB dotnet#2

b83a300

Fix GLE

e49edb4

Initialize machineName in ctor

b8141de

Test for empty name ex

bee632a

Null names

d7cc299

Inadvertent edit

f336a2d

danmoseley commented Sep 3, 2018

View reviewed changes

Unix build

8051096

jkotas reviewed Sep 3, 2018

View reviewed changes

danmoseley added 3 commits September 3, 2018 10:17

Move interop into class

e23c107

Reverse SafeHandle for HAllocGlobal

23a0c24

Fix tests

711ec3c

Disable test for NETFX

2d137b3

jkotas reviewed Sep 4, 2018

View reviewed changes

CR feedback

905e4c2

jkotas reviewed Sep 4, 2018

View reviewed changes

jkotas approved these changes Sep 4, 2018

View reviewed changes

danmoseley added 2 commits September 4, 2018 09:23

Pattern matching on VSB

778e3f6

Direct call

8184acf

safern reviewed Sep 4, 2018

View reviewed changes

typo

10b2e33

danmoseley merged commit 76c8587 into dotnet:master Sep 4, 2018

danmoseley deleted the servicecontroller branch September 4, 2018 22:21

karelz added this to the 3.0 milestone Sep 6, 2018

JeremyKuhne mentioned this pull request Oct 13, 2018

Removed unnecessary MemoryMarshal.GetReference #32669

Closed

lewurm mentioned this pull request Feb 1, 2019

[2018-08] Bump corert mono/mono#12721

Merged

daxian-dbw mentioned this pull request Aug 2, 2019

Updating System.ServiceProcess.ServiceController to its .Net Core 3-preview6 version breaks 2 tests PowerShell/PowerShell#10067

Closed

JeremyKuhne mentioned this pull request Jan 31, 2020

Need to pull pinning via &MemoryMarshal.GetReference dotnet/runtime#27308

Open

		@@ -38,7 +39,6 @@ public class ServiceController : Component
		private ServiceStartMode _startType;

		private const int SERVICENAMEMAXLENGTH = 80;

		@@ -449,12 +449,6 @@ private static bool CheckMachineName(string value)
		}

		public void Close()

Fix ServiceController name population perf #32072

Fix ServiceController name population perf #32072

Conversation

danmoseley commented Sep 2, 2018

danmoseley commented Sep 2, 2018

danmoseley commented Sep 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Sep 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented Sep 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented Sep 3, 2018

danmoseley commented Sep 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented Sep 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented Sep 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Sep 3, 2018 •

edited

Loading