-
Notifications
You must be signed in to change notification settings - Fork 97
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
Startup performance measure tool #851
Conversation
…d to any DotVVM project
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.
nice :)
} | ||
return true; | ||
} | ||
catch (Exception) |
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 should only catch DirectoryNotFoundException
. If there is another problem (like, file with the same name exists, runtime error, ... it should still fail)
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.
The catch was here actually for IOException
and UnauthorizedAccessException
- I am using it only to delete a temp folder and in general, it's not a big deal if it fails.
I've added a retry logic - sometimes it failed to delete it because some files were still locked.
for (var i = 0; i < repeat; i++) | ||
{ | ||
TraceOutput($"Attempt #{i + 1}"); | ||
measuredTime += RunProcessAndWaitForHealthCheck(@"dotnet", $@"""./{Path.GetFileNameWithoutExtension(projectPath)}.dll"" --urls {urlToTest}", tempDir, urlToTest); |
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.
Unfortunately, the .NET API for starting processes is terrible, I'd recommend using https://github.com/madelson/MedallionShell, it handles the argument escaping well.
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.
Thanks for the tip, it is a nice library.
catch (WebException ex) when (ex.InnerException is HttpRequestException hrex && (hrex.InnerException is IOException || hrex.InnerException is SocketException)) | ||
{ | ||
Thread.Sleep(100); | ||
goto retry; |
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.
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.
Added the timeout option (with a default value of 10 seconds). If the HTTP port doesn't open within the timeout, an exception is thrown.
|
||
if (process.HasExited) | ||
{ | ||
throw new Exception("The process has exited!"); |
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.
The check should be inside the retry
loop
…ed MedallionShell to work correctly with command line args
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 this tool and measuring performance overall is a great idea.
Still, do we need so many console projects? Couldn't we maybe consolidate them all into one dotnet-dotvvm tool?
@@ -0,0 +1,23 @@ | |||
<Project Sdk="Microsoft.NET.Sdk"> | |||
<PropertyGroup> | |||
<TargetFramework>netcoreapp3.0</TargetFramework> |
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.
Is there any particular reason for not targeting .NET Core 3.1, which is an LTS release?
<PackageLicenseExpression>Apache-2.0</PackageLicenseExpression> | ||
</PropertyGroup> | ||
<ItemGroup> | ||
<PackageReference Include="MedallionShell" Version="1.6.1" /> |
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.
CSC would rather have you reference the MedallionShell.StrongName package.
<PackageTags>dotvvm;asp.net;mvvm;owin;dotnetcore;dnx;cli</PackageTags> | ||
<PackageIconUrl>https://dotvvm.com/Content/images/icons/icon-blue-64x64.png</PackageIconUrl> | ||
<PackageLicenseExpression>Apache-2.0</PackageLicenseExpression> | ||
</PropertyGroup> |
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.
Consider setting the AssemblyName
property to create a shorter name for the executable than DotVVM.Tools.StartupPerfTester
.
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.
Especially since PackAsTool
is set to true
.
</PropertyGroup> | ||
<ItemGroup> | ||
<PackageReference Include="MedallionShell" Version="1.6.1" /> | ||
<PackageReference Include="system.commandline" Version="2.0.0-beta1.20253.1" /> |
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.
There's a newer version of System.CommandLine
. :P
) { Arity = ArgumentArity.ExactlyOne }, | ||
new Option<TestTarget>( | ||
new [] { "-t", "--type" }, | ||
"Type of the project - use 'owin' or 'aspnetcore'" |
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.
The "use 'owin' or 'aspnetcore'" part is unnecessary. System.CommandLine already shows the options:
Options:
-t, --type <AspNetCore|Owin> (REQUIRED) Type of the project - use 'owin' or 'aspnetcore'
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.
Also, can't we figure out what type the project is based on its project file?
Console.WriteLine(message); | ||
} | ||
|
||
public void TraceOutput(string message) |
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.
Consider using Microsoft.Extensions.Logging.Console instead.
{ | ||
// OWIN | ||
TraceOutput($"Publishing..."); | ||
RunProcessAndWait(@"c:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\MSBuild\Current\Bin\MSBuild.exe", |
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.
Can't we use vswhere instead of hard-coding the path?
To measure startup performance improvements in #812, I've added a simple tool that can publish a DotVVM app in Release mode (using MSBuild DeployOnBuild target for OWIN and dotnet publish for ASP.NET Core), run it repeatedly and measure how long does it take until a first HTTP response is generated.