Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Fix ServiceController name population perf #32072

Merged
merged 21 commits into from
Sep 4, 2018
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Common/src/Interop/Windows/Interop.Errors.cs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ internal partial class Errors
internal const int ERROR_IO_INCOMPLETE = 0x3E4;
internal const int ERROR_IO_PENDING = 0x3E5;
internal const int ERROR_NO_TOKEN = 0x3f0;
internal const int ERROR_SERVICE_DOES_NOT_EXIST = 0x424;
internal const int ERROR_DLL_INIT_FAILED = 0x45A;
internal const int ERROR_COUNTER_TIMEOUT = 0x461;
internal const int ERROR_NO_ASSOCIATION = 0x483;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using Microsoft.Win32.SafeHandles;
using System;
using System.Runtime.InteropServices;

Expand All @@ -10,9 +11,9 @@ internal partial class Interop
internal partial class Advapi32
{
[DllImport(Libraries.Advapi32, CharSet = CharSet.Unicode, SetLastError = true)]
public static extern bool ChangeServiceConfig2(IntPtr serviceHandle, uint infoLevel, ref SERVICE_DESCRIPTION serviceDesc);
public static extern bool ChangeServiceConfig2(SafeServiceHandle serviceHandle, uint infoLevel, ref SERVICE_DESCRIPTION serviceDesc);

[DllImport(Libraries.Advapi32, CharSet = CharSet.Unicode, SetLastError = true)]
public static extern bool ChangeServiceConfig2(IntPtr serviceHandle, uint infoLevel, ref SERVICE_DELAYED_AUTOSTART_INFO serviceDesc);
public static extern bool ChangeServiceConfig2(SafeServiceHandle serviceHandle, uint infoLevel, ref SERVICE_DELAYED_AUTOSTART_INFO serviceDesc);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using Microsoft.Win32.SafeHandles;
using System;
using System.Runtime.InteropServices;

Expand All @@ -10,7 +11,7 @@ internal partial class Interop
internal partial class Advapi32
{
[DllImport(Libraries.Advapi32, CharSet = CharSet.Unicode, SetLastError = true)]
internal extern unsafe static bool ControlService(IntPtr serviceHandle, int control, SERVICE_STATUS* pStatus);
internal extern unsafe static bool ControlService(SafeServiceHandle serviceHandle, int control, SERVICE_STATUS* pStatus);

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using Microsoft.Win32.SafeHandles;
using System;
using System.Runtime.InteropServices;

Expand All @@ -10,7 +11,7 @@ internal partial class Interop
internal partial class Advapi32
{
[DllImport(Libraries.Advapi32, CharSet = CharSet.Unicode, SetLastError = true)]
public extern static IntPtr CreateService(IntPtr databaseHandle, string serviceName, string displayName, int access, int serviceType,
public extern static IntPtr CreateService(SafeServiceHandle databaseHandle, string serviceName, string displayName, int access, int serviceType,
int startType, int errorControl, string binaryPath, string loadOrderGroup, IntPtr pTagId, string dependencies,
string servicesStartName, string password);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using Microsoft.Win32.SafeHandles;
using System;
using System.Runtime.InteropServices;

Expand All @@ -10,6 +11,6 @@ internal partial class Interop
internal partial class Advapi32
{
[DllImport(Libraries.Advapi32, CharSet = CharSet.Unicode, SetLastError = true)]
public extern static bool DeleteService(IntPtr serviceHandle);
public extern static bool DeleteService(SafeServiceHandle serviceHandle);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using Microsoft.Win32.SafeHandles;
using System;
using System.Runtime.InteropServices;

Expand All @@ -11,7 +12,7 @@ internal partial class Advapi32
{
[DllImport(Libraries.Advapi32, EntryPoint = "EnumDependentServicesW", CharSet = CharSet.Unicode, SetLastError = true)]
internal extern static bool EnumDependentServices(
IntPtr serviceHandle,
SafeServiceHandle serviceHandle,
int serviceState,
IntPtr bufferOfENUM_SERVICE_STATUS,
int bufSize,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using Microsoft.Win32.SafeHandles;
using System;
using System.Runtime.InteropServices;

Expand All @@ -11,7 +12,7 @@ internal partial class Advapi32
{
[DllImport(Libraries.Advapi32, EntryPoint = "EnumServicesStatusExW", CharSet = CharSet.Unicode, SetLastError = true)]
internal extern static bool EnumServicesStatusEx(
IntPtr databaseHandle,
SafeServiceHandle databaseHandle,
int infolevel,
int serviceType,
int serviceState,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using Microsoft.Win32.SafeHandles;
using System;
using System.Buffers;
using System.ComponentModel;
using System.Runtime.InteropServices;
using System.Text;

internal partial class Interop
{
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are not consistent with this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @JeremyKuhne - we should probably be consistent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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+).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using Microsoft.Win32.SafeHandles;
using System;
using System.Buffers;
using System.ComponentModel;
using System.Runtime.InteropServices;
using System.Text;

internal partial class Interop
{
internal partial class Advapi32
{
[DllImport(Libraries.Advapi32, EntryPoint = "GetServiceKeyNameW", CharSet = System.Runtime.InteropServices.CharSet.Unicode, SetLastError = true)]
internal static extern bool GetServiceKeyName(SafeServiceHandle SCMHandle, string displayName, ref char KeyName, ref int KeyNameLength);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using Microsoft.Win32.SafeHandles;
using System;
using System.Runtime.InteropServices;

Expand All @@ -10,6 +11,6 @@ internal partial class Interop
internal partial class Advapi32
{
[DllImport(Libraries.Advapi32, EntryPoint = "OpenServiceW", CharSet = CharSet.Unicode, SetLastError = true)]
internal extern static IntPtr OpenService(IntPtr databaseHandle, string serviceName, int access);
internal extern static IntPtr OpenService(SafeServiceHandle databaseHandle, string serviceName, int access);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using Microsoft.Win32.SafeHandles;
using System;
using System.Runtime.InteropServices;

Expand All @@ -10,7 +11,7 @@ internal partial class Interop
internal partial class Advapi32
{
[DllImport(Libraries.Advapi32, EntryPoint = "QueryServiceConfigW", CharSet = CharSet.Unicode, SetLastError = true)]
internal extern static bool QueryServiceConfig(IntPtr serviceHandle, IntPtr queryServiceConfigPtr, int bufferSize, out int bytesNeeded);
internal extern static bool QueryServiceConfig(SafeServiceHandle serviceHandle, IntPtr queryServiceConfigPtr, int bufferSize, out int bytesNeeded);

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using Microsoft.Win32.SafeHandles;
using System;
using System.Runtime.InteropServices;

Expand All @@ -10,7 +11,7 @@ internal partial class Interop
internal partial class Advapi32
{
[DllImport(Libraries.Advapi32, CharSet = CharSet.Unicode, SetLastError = true)]
internal static extern unsafe bool QueryServiceStatus(IntPtr serviceHandle, SERVICE_STATUS* pStatus);
internal static extern unsafe bool QueryServiceStatus(SafeServiceHandle serviceHandle, SERVICE_STATUS* pStatus);

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using Microsoft.Win32.SafeHandles;
using System;
using System.Runtime.InteropServices;

Expand All @@ -10,6 +11,6 @@ internal partial class Interop
internal partial class Advapi32
{
[DllImport(Libraries.Advapi32, EntryPoint = "StartServiceW", CharSet = CharSet.Unicode, SetLastError = true)]
internal extern static bool StartService(IntPtr serviceHandle, int argNum, IntPtr argPtrs);
internal extern static bool StartService(SafeServiceHandle serviceHandle, int argNum, IntPtr argPtrs);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@

namespace Microsoft.Win32.SafeHandles
{
/// <summary>
/// Used to wrap handles gotten from OpenSCManager or OpenService
/// </summary>
internal class SafeServiceHandle : SafeHandle
{
internal SafeServiceHandle(IntPtr handle) : base(IntPtr.Zero, true)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,10 @@
<value>Arguments within the 'args' array passed to Start cannot be null.</value>
</data>
<data name="BadMachineName" xml:space="preserve">
<value>MachineName value {0} is invalid.</value>
<value>MachineName '{0}' is invalid.</value>
</data>
<data name="CannotStart" xml:space="preserve">
<value>Cannot start service {0} on computer '{1}'.</value>
<value>Cannot start service '{0}' on computer '{1}'.</value>
</data>
<data name="InvalidEnumArgument" xml:space="preserve">
<value>The value of argument '{0}' ({1}) is invalid for Enum type '{2}'.</value>
Expand All @@ -77,22 +77,22 @@
<value>MachineName was not set.</value>
</data>
<data name="NoService" xml:space="preserve">
<value>Service {0} was not found on computer '{1}'.</value>
<value>Service '{0}' was not found on computer '{1}'.</value>
</data>
<data name="OpenSC" xml:space="preserve">
<value>Cannot open Service Control Manager on computer '{0}'. This operation might require other privileges.</value>
</data>
<data name="OpenService" xml:space="preserve">
<value>Cannot open {0} service on computer '{1}'.</value>
<value>Cannot open '{0}' service on computer '{1}'.</value>
</data>
<data name="PauseService" xml:space="preserve">
<value>Cannot pause {0} service on computer '{1}'.</value>
<value>Cannot pause '{0}' service on computer '{1}'.</value>
</data>
<data name="ResumeService" xml:space="preserve">
<value>Cannot resume {0} service on computer '{1}'.</value>
<value>Cannot resume '{0}' service on computer '{1}'.</value>
</data>
<data name="StopService" xml:space="preserve">
<value>Cannot stop {0} service on computer '{1}'.</value>
<value>Cannot stop '{0}' service on computer '{1}'.</value>
</data>
<data name="Timeout" xml:space="preserve">
<value>Time out has expired and the operation has not been completed.</value>
Expand All @@ -107,7 +107,7 @@
<value>Cannot change service name when the service is running.</value>
</data>
<data name="ServiceName" xml:space="preserve">
<value>Service name {0} contains invalid characters, is empty, or is too long (max length = {1}).</value>
<value>Service name '{0}' contains invalid characters, is empty, or is too long (max length = {1}).</value>
</data>
<data name="NoServices" xml:space="preserve">
<value>Service has not been supplied. At least one object derived from ServiceBase is required in order to run.</value>
Expand Down Expand Up @@ -179,18 +179,18 @@
<value>Failed in handling the PowerEvent. The error that occurred was: {0}.</value>
</data>
<data name="InstallOK" xml:space="preserve">
<value>Service {0} has been successfully installed.</value>
<value>Service '{0}' has been successfully installed.</value>
</data>
<data name="TryToStop" xml:space="preserve">
<value>Attempt to stop service {0}.</value>
<value>Attempt to stop service '{0}'.</value>
</data>
<data name="ServiceRemoving" xml:space="preserve">
<value>Service {0} is being removed from the system...</value>
<value>Service '{0}' is being removed from the system...</value>
</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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: extra '

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good eye, thanks!

</data>
<data name="ControlService" xml:space="preserve">
<value>Cannot control {0} service on computer '{1}'.</value>
<value>Cannot control '{0}' service on computer '{1}'.</value>
</data>
</root>
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
<Configurations>net461-Windows_NT-Debug;net461-Windows_NT-Release;netcoreapp-Windows_NT-Debug;netcoreapp-Windows_NT-Release;netfx-Windows_NT-Debug;netfx-Windows_NT-Release;netstandard-Debug;netstandard-Release;netstandard-Windows_NT-Debug;netstandard-Windows_NT-Release</Configurations>
</PropertyGroup>
<ItemGroup Condition="$(TargetGroup.StartsWith('netcoreapp')) OR ('$(TargetGroup)' == 'netstandard' AND '$(TargetsWindows)' == 'true')">
<Compile Include="$(CommonPath)\CoreLib\System\Text\ValueStringBuilder.cs">
<Link>Common\CoreLib\System\Text\ValueStringBuilder.cs</Link>
</Compile>
<Compile Include="$(CommonPath)\Interop\Windows\Interop.Libraries.cs">
<Link>Common\Interop\Windows\Interop.Libraries.cs</Link>
</Compile>
Expand All @@ -33,6 +36,12 @@
<Compile Include="$(CommonPath)\Interop\Windows\advapi32\Interop.EnumServicesStatusEx.cs">
<Link>Common\Interop\Windows\Interop.EnumServicesStatusEx.cs</Link>
</Compile>
<Compile Include="$(CommonPath)\Interop\Windows\advapi32\Interop.GetServiceDisplayName.cs">
<Link>Common\Interop\Windows\Interop.GetServiceDisplayName.cs</Link>
</Compile>
<Compile Include="$(CommonPath)\Interop\Windows\advapi32\Interop.GetServiceKeyName.cs">
<Link>Common\Interop\Windows\Interop.GetServiceKeyName.cs</Link>
</Compile>
<Compile Include="$(CommonPath)\Interop\Windows\advapi32\Interop.OpenSCManager.cs">
<Link>Common\Interop\Windows\Interop.OpenSCManager.cs</Link>
</Compile>
Expand Down Expand Up @@ -93,13 +102,16 @@
</ItemGroup>
<ItemGroup Condition="$(TargetGroup.StartsWith('netcoreapp')) OR ('$(TargetGroup)' == 'netstandard' AND '$(TargetsWindows)' == 'true')">
<Reference Include="Microsoft.Win32.Primitives" />
<Reference Include="System.Buffers" />
<Reference Include="System.Collections" />
<Reference Include="System.Console" />
<Reference Include="System.ComponentModel.Primitives" />
<Reference Include="System.Diagnostics.Debug" />
<Reference Include="System.Diagnostics.Tools" />
<Reference Include="System.Memory" />
<Reference Include="System.Resources.ResourceManager" />
<Reference Include="System.Runtime" />
<Reference Include="System.Runtime.Extensions" />
<Reference Include="System.Runtime.InteropServices" />
<Reference Include="System.Threading" />
<Reference Include="System.Threading.Thread" />
Expand Down
Loading