Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove some overhead from Process.Start #44691

Merged
merged 1 commit into from
Nov 17, 2020

Conversation

stephentoub
Copy link
Member

@stephentoub stephentoub commented Nov 14, 2020

  • Avoid StringBuilder marshaling
  • Let CreateProcess{WithLogonW} determine the current working directory
  • Avoid creating SafeHandles for GetCurrentProcess()
  • Avoid forcing ProcessStartInfo.ArgumentList into existence just to check if it contains anything
  • Prefer using ProcessStartInfo.Arguments in Start(string, IEnumerable) if there's only one string in the enumerable
  • Use ValueStringBuilder instead of StringBuilder to build up arguments, so as to use stack space / pooled char[]s and avoid actually needing to produce strings.
  • Avoid unnecessary SafeHandle for PROCESS_INFORMATION.hThread

Test that launches a thousand processes, no redirection, one argument (execution time is dominated by the actual native call and remote process, so this has a negligible impact on serial execution time):

Allocations before
image

Allocations after
image

Contributes to #44598

@ghost
Copy link

ghost commented Nov 14, 2020

Tagging subscribers to this area: @eiriktsarpalis, @jeffhandley
See info in area-owners.md if you want to be subscribed.

Issue Details
Description:
  • Avoid StringBuilder marshaling
  • Let CreateProcess{WithLogonW} determine the current working directory
  • Avoid creating SafeHandles for GetCurrentProcess()
  • Avoid forcing ProcessStartInfo.ArgumentList into existence just to check if it contains anything
  • Prefer using ProcessStartInfo.Arguments in Start(string, IEnumerable) if there's only one string in the enumerable
  • Use ValueStringBuilder instead of StringBuilder to build up arguments, so as to use stack space / pooled char[]s and avoid actually needing to produce strings.
  • Avoid unnecessary SafeHandle for PROCESS_INFORMATION.hThread

Test that launches a thousand processes, no redirection, one argument (execution time is dominated by the actual native call and remote process, so this has a negligible impact on serial execution time):

Allocations before
image

Allocations after
image

Author: stephentoub
Assignees: -
Labels:

area-System.Diagnostics.Process

Milestone: -

Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

LGTM.

@eiriktsarpalis
Copy link
Member

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@jkotas
Copy link
Member

jkotas commented Nov 17, 2020

Build break

[F:\workspace\_work\1\s\src\coreclr\src\System.Private.CoreLib\System.Private.CoreLib.csproj]
Source file 'F:\workspace\_work\1\s\src\libraries\Common\src\Interop\Windows\Kernel32\Interop.GetCurrentProcess_IntPtr.cs' could not be found.

- Avoid StringBuilder marshaling
- Let CreateProcess{WithLogonW} determine the current working directory
- Avoid creating SafeHandles for GetCurrentProcess()
- Avoid forcing ProcessStartInfo.ArgumentList into existence just to check if it contains anything
- Prefer using ProcessStartInfo.Arguments in Start(string, IEnumerable<string>) if there's only one string in the enumerable
- Use ValueStringBuilder instead of StringBuilder to build up arguments, so as to use stack space / pooled char[]s and avoid actually needing to produce strings.
- Avoid unnecessary SafeHandle for PROCESS_INFORMATION.hThread
@stephentoub
Copy link
Member Author

Build break

What happens when you search within *.csproj and forget about *.projitems ;-)

@stephentoub stephentoub merged commit ef2a187 into dotnet:master Nov 17, 2020
@stephentoub stephentoub deleted the processalloc branch November 17, 2020 22:48
@ghost ghost locked as resolved and limited conversation to collaborators Dec 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants