This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Fix ServiceController name population perf #32072
Merged
Merged
Changes from 17 commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
4738fbf
Fix ServiceController name population perf
danmoseley 7f169b4
Split tests
danmoseley 34b6d9f
Remove dead field
danmoseley aca1848
Remove new use of DangerousGetHandle
danmoseley 04d0c1a
SafeHandle all the things!
danmoseley bd9db82
VSB #1
danmoseley b83a300
VSB #2
danmoseley e49edb4
Fix GLE
danmoseley b8141de
Initialize machineName in ctor
danmoseley bee632a
Test for empty name ex
danmoseley d7cc299
Null names
danmoseley f336a2d
Inadvertent edit
danmoseley 8051096
Unix build
danmoseley e23c107
Move interop into class
danmoseley 23a0c24
Reverse SafeHandle for HAllocGlobal
danmoseley 711ec3c
Fix tests
danmoseley 2d137b3
Disable test for NETFX
danmoseley 905e4c2
CR feedback
danmoseley 778e3f6
Pattern matching on VSB
danmoseley 8184acf
Direct call
danmoseley 10b2e33
typo
danmoseley File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
19 changes: 19 additions & 0 deletions
19
src/Common/src/Interop/Windows/advapi32/Interop.GetServiceDisplayName.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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); | ||
} | ||
} |
19 changes: 19 additions & 0 deletions
19
src/Common/src/Interop/Windows/advapi32/Interop.GetServiceKeyName.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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> | ||
|
@@ -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> | ||
|
@@ -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> | ||
|
@@ -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> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: extra ' There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Yields something like:
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+).
There was a problem hiding this comment.
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.
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 useMemoryMarshal.GetReference
to pin Span that were introduced before we have proper support for pinning Span. We should move away from usingMemoryMarshal.GetReference
for pinning Span.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
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 ofchar*
have the same impact with the marshaller asMemoryMarshal.GetReference
? I'll validate when I get some time.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.