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 setting of CreateNoWindow to reduce overhead #142

Merged
merged 8 commits into from
Mar 14, 2022

Conversation

ian-g-holm-intel
Copy link
Contributor

I've observed a 30-40ms overhead added to the execution time of any process when you set ProcessStartInfo.CreateNoWindow to anything, true or false. I'm not sure why exactly this is, but you can see the difference in the benchmarks really easily. CliWrap sets CreateNoWindow = true and UseShellExecute = false. But as far as I can tell, UseShellExecute = false is the default and CreateNoWindow doesn't work unless UseShellExecute = true. So removing those two settings from the ProcessStartInfo is functionally equivalent to the way they are currently, but it allows processes to run without the 30ms overhead.

@Tyrrrz Tyrrrz changed the title Removing setting of CreateNoWindow to reduce overhead Remove setting of CreateNoWindow to reduce overhead Mar 14, 2022
@ian-g-holm-intel
Copy link
Contributor Author

I ran a benchmark executing "cmd.exe /c echo this is a test" with CreateNoWindow set to true vs unset.
CreateNoWindow = true
before
CreateNoWindow unset
after

@ian-g-holm-intel
Copy link
Contributor Author

This proposed fix will check if the current application has a console or not. If it does not, it will set ProcessStartInfo.CreateNoWindow = true. If it does have a console, CreateNoWindow will not be set, avoiding the added overhead of setting it.

CliWrap/Command.cs Outdated Show resolved Hide resolved
CliWrap/Command.cs Outdated Show resolved Hide resolved
CliWrap/Command.cs Outdated Show resolved Hide resolved
@Tyrrrz
Copy link
Owner

Tyrrrz commented Mar 14, 2022

@retik okay, just to make sure, you tested all previously scenarios with this and it works correctly, right?
desktop/console/batch file, etc

@Tyrrrz Tyrrrz merged commit c55bba6 into Tyrrrz:master Mar 14, 2022
@what-the-diff
Copy link

what-the-diff bot commented Nov 10, 2022

  • The diff above is a change to the CliWrap/Command.cs file and adds a new file called NativeMethods.cs
  • In Command, we are checking if there is no console window on Windows platforms before setting CreateNoWindow = true in ProcessStartInfo
  • We added an external method GetConsoleWindow() from kernel32 that returns IntPtr (pointer) for use in our check of whether or not there's a console window open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants