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

Perf improvements part 6: startup time #1654

Merged
merged 19 commits into from
Mar 1, 2022
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
790646f
extend ICommandHandler with synchronous Invoke method
adamsitnik Feb 22, 2022
7b15e52
don't build invocation chain if there is no middleware
adamsitnik Feb 22, 2022
e084081
_executablePath.Value already uses Environment.GetCommandLineArgs()[0]
adamsitnik Feb 22, 2022
e950bf0
Command.Validators: allocate when needed
adamsitnik Feb 22, 2022
465e150
reduce and simplify ValidateCommandResult size to JIT less code in mo…
adamsitnik Feb 22, 2022
4f7cc72
reduce ParseResultVisitor.Stop size, saves 1 ms of JIT time for simpl…
adamsitnik Feb 22, 2022
59b1941
make Token a class again, so JIT does not need to compile specialized…
adamsitnik Feb 22, 2022
0ed5532
refactor ParseDirectives to JIT less code when directives are not pro…
adamsitnik Feb 22, 2022
fb76e42
refactor GetValueForHandlerParameter to JIT less code for most common…
adamsitnik Feb 22, 2022
bd44fb2
remove static fields from StringExtensions (they need to be compiled …
adamsitnik Feb 22, 2022
0a82102
optimize getting Middleware by using Tuple (class) instead of ValueTu…
adamsitnik Feb 23, 2022
3de63b1
use Nullable.GetUnderlyingType instead of custom implementation (and …
adamsitnik Feb 23, 2022
2127b1e
don't use Lazy, -4 compilations
adamsitnik Feb 23, 2022
1d7aff7
reduce the number of types derived from ArgumentConversionResult to r…
adamsitnik Feb 23, 2022
692f045
remove RootCommandNode to reduce the number of types
adamsitnik Feb 23, 2022
54e26d4
remove another Lazy (the code it uses needed to be compiled the first…
adamsitnik Feb 23, 2022
0d072bd
add net6.0 to benchmarks project
adamsitnik Feb 23, 2022
511d6d0
Merge remote-tracking branch 'upstream/main' into perfImprovements6
adamsitnik Feb 23, 2022
5f475cb
address code review feedback
adamsitnik Feb 24, 2022
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
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@
public class ModelBindingCommandHandler, System.CommandLine.Invocation.ICommandHandler
public System.Void BindParameter(System.Reflection.ParameterInfo param, System.CommandLine.Argument argument)
public System.Void BindParameter(System.Reflection.ParameterInfo param, System.CommandLine.Option option)
public System.Int32 Invoke(System.CommandLine.Invocation.InvocationContext context)
public System.Threading.Tasks.Task<System.Int32> InvokeAsync(System.CommandLine.Invocation.InvocationContext context)
public class ModelDescriptor
public static ModelDescriptor FromType<T>()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,7 @@ System.CommandLine.Help
public System.Int32 GetHashCode()
System.CommandLine.Invocation
public interface ICommandHandler
public System.Int32 Invoke(InvocationContext context)
public System.Threading.Tasks.Task<System.Int32> InvokeAsync(InvocationContext context)
public interface IInvocationResult
public System.Void Apply(InvocationContext context)
Expand Down Expand Up @@ -498,7 +499,7 @@ System.CommandLine.Parsing
public T GetValueForOption<T>(Option<T> option)
public System.Object GetValueForOption(System.CommandLine.Option option)
public System.String ToString()
public struct Token : System.ValueType, System.IEquatable<Token>
public class Token, System.IEquatable<Token>
public static System.Boolean op_Equality(Token left, Token right)
public static System.Boolean op_Inequality(Token left, Token right)
.ctor(System.String value, TokenType type, System.CommandLine.Symbol symbol)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
<UseSharedCompilation>false</UseSharedCompilation>

<!-- Supported target frameworks -->
<TargetFrameworks Condition="'$(TargetFrameworks)' == '' AND '$(OS)' == 'Windows_NT'">net461;net5.0;</TargetFrameworks>
<TargetFrameworks Condition="'$(TargetFrameworks)' == ''">net5.0;</TargetFrameworks>
<TargetFrameworks Condition="'$(TargetFrameworks)' == '' AND '$(OS)' == 'Windows_NT'">net461;net5.0;net6.0;</TargetFrameworks>
<TargetFrameworks Condition="'$(TargetFrameworks)' == ''">net5.0;net6.0;</TargetFrameworks>
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved

<!-- This repo does not produce any libraries, therefore generating docs is disabled -->
<GenerateDocumentationFile>False</GenerateDocumentationFile>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,9 @@ private class GeneratedHandler_{handlerCount} : {ICommandHandlerType}
{propertyDeclaration}");
}

builder.Append($@"
public int Invoke(global::System.CommandLine.Invocation.InvocationContext context) => InvokeAsync(context).GetAwaiter().GetResult();");

builder.Append($@"
public async global::System.Threading.Tasks.Task<int> InvokeAsync(global::System.CommandLine.Invocation.InvocationContext context)
{{");
Expand Down
8 changes: 8 additions & 0 deletions src/System.CommandLine.Hosting.Tests/HostingHandlerTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,12 @@ public MyHandler(MyService service)
public int IntOption { get; set; } // bound from option
public IConsole Console { get; set; } // bound from DI

public int Invoke(InvocationContext context)
{
service.Value = IntOption;
return IntOption;
}

public Task<int> InvokeAsync(InvocationContext context)
{
service.Value = IntOption;
Expand Down Expand Up @@ -162,6 +168,8 @@ public MyHandler(MyService service)

public string One { get; set; }

public int Invoke(InvocationContext context) => InvokeAsync(context).GetAwaiter().GetResult();

public Task<int> InvokeAsync(InvocationContext context)
{
service.Value = IntOption;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,8 @@ public abstract class AbstractTestCommandHandler : ICommandHandler
{
public abstract Task<int> DoJobAsync();

public int Invoke(InvocationContext context) => InvokeAsync(context).GetAwaiter().GetResult();

public Task<int> InvokeAsync(InvocationContext context)
=> DoJobAsync();
}
Expand All @@ -458,6 +460,8 @@ public override Task<int> DoJobAsync()

public class VirtualTestCommandHandler : ICommandHandler
{
public int Invoke(InvocationContext context) => 42;

public virtual Task<int> InvokeAsync(InvocationContext context)
=> Task.FromResult(42);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,4 +128,6 @@ private void BindValueSource(ParameterInfo param, IValueSource valueSource)
: _methodDescriptor.ParameterDescriptors
.FirstOrDefault(x => x.ValueName == param.Name &&
x.ValueType == param.ParameterType);

public int Invoke(InvocationContext context) => InvokeAsync(context).GetAwaiter().GetResult();
}
12 changes: 7 additions & 5 deletions src/System.CommandLine/ArgumentArity.cs
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public bool Equals(ArgumentArity other) =>
public override int GetHashCode()
=> MaximumNumberOfValues ^ MinimumNumberOfValues ^ IsNonDefault.GetHashCode();

internal static FailedArgumentConversionArityResult? Validate(
internal static ArgumentConversionResult? Validate(
SymbolResult symbolResult,
Argument argument,
int minimumNumberOfValues,
Expand All @@ -93,9 +93,10 @@ public override int GetHashCode()
return null;
}

return new MissingArgumentConversionResult(
return ArgumentConversionResult.Failure(
argument,
symbolResult.LocalizationResources.RequiredArgumentMissing(symbolResult));
symbolResult.LocalizationResources.RequiredArgumentMissing(symbolResult),
ArgumentConversionResultType.FailedMissingArgument);
}

if (tokenCount > maximumNumberOfValues)
Expand All @@ -104,9 +105,10 @@ public override int GetHashCode()
{
if (!optionResult.Option.AllowMultipleArgumentsPerToken)
{
return new TooManyArgumentsConversionResult(
return ArgumentConversionResult.Failure(
argument,
symbolResult!.LocalizationResources.ExpectsOneArgument(symbolResult));
symbolResult!.LocalizationResources.ExpectsOneArgument(symbolResult),
ArgumentConversionResultType.FailedTooManyArguments);
}
}
}
Expand Down
64 changes: 57 additions & 7 deletions src/System.CommandLine/Binding/ArgumentConversionResult.cs
Original file line number Diff line number Diff line change
@@ -1,23 +1,73 @@
// Copyright (c) .NET Foundation and contributors. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System.Linq;

namespace System.CommandLine.Binding
{
internal abstract class ArgumentConversionResult
internal sealed class ArgumentConversionResult
{
private protected ArgumentConversionResult(Argument argument)
internal readonly Argument Argument;
internal readonly object? Value;
internal readonly string? ErrorMessage;
internal ArgumentConversionResultType Result;

private ArgumentConversionResult(Argument argument, string error, ArgumentConversionResultType failure)
{
Argument = argument ?? throw new ArgumentNullException(nameof(argument));
ErrorMessage = error ?? throw new ArgumentNullException(nameof(error));
Result = failure;
}

private ArgumentConversionResult(Argument argument, object? value)
{
Argument = argument ?? throw new ArgumentNullException(nameof(argument));
Value = value;
Result = ArgumentConversionResultType.Successful;
}

private ArgumentConversionResult(Argument argument)
{
Argument = argument ?? throw new ArgumentNullException(nameof(argument));
Result = ArgumentConversionResultType.NoArgument;
}

internal ArgumentConversionResult(
Argument argument,
Type expectedType,
string value,
LocalizationResources localizationResources) :
this(argument, FormatErrorMessage(argument, expectedType, value, localizationResources), ArgumentConversionResultType.FailedType)
{
}

public Argument Argument { get; }
internal static ArgumentConversionResult Failure(Argument argument, string error, ArgumentConversionResultType reason) => new(argument, error, reason);

public static ArgumentConversionResult Success(Argument argument, object? value) => new(argument, value);

internal string? ErrorMessage { get; set; }
internal static ArgumentConversionResult None(Argument argument) => new(argument);

internal static FailedArgumentConversionResult Failure(Argument argument, string error) => new(argument, error);
private static string FormatErrorMessage(
Argument argument,
Type expectedType,
string value,
LocalizationResources localizationResources)
{
if (argument.FirstParent?.Symbol is IdentifierSymbol identifierSymbol &&
argument.FirstParent.Next is null)
{
var alias = identifierSymbol.Aliases.First();

public static SuccessfulArgumentConversionResult Success(Argument argument, object? value) => new(argument, value);
switch (identifierSymbol)
{
case Command _:
return localizationResources.ArgumentConversionCannotParseForCommand(value, alias, expectedType);
case Option _:
return localizationResources.ArgumentConversionCannotParseForOption(value, alias, expectedType);
}
}

internal static NoArgumentConversionResult None(Argument argument) => new(argument);
return localizationResources.ArgumentConversionCannotParse(value, expectedType);
}
}
}
16 changes: 16 additions & 0 deletions src/System.CommandLine/Binding/ArgumentConversionResultType.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// Copyright (c) .NET Foundation and contributors. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

namespace System.CommandLine.Binding
{
internal enum ArgumentConversionResultType
{
NoArgument, // NoArgumentConversionResult
Successful, // SuccessfulArgumentConversionResult
Failed, // FailedArgumentConversionResult
FailedArity, // FailedArgumentConversionArityResult
FailedType, // FailedArgumentTypeConversionResult
FailedTooManyArguments, // TooManyArgumentsConversionResult
FailedMissingArgument, // MissingArgumentConversionResult
}
}
16 changes: 10 additions & 6 deletions src/System.CommandLine/Binding/ArgumentConverter.DefaultValues.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,7 @@ namespace System.CommandLine.Binding;
internal static partial class ArgumentConverter
{
#if NET6_0_OR_GREATER
private static readonly Lazy<ConstructorInfo> _listCtor =
new(() => typeof(List<>)
.GetConstructors()
.SingleOrDefault(c => c.GetParameters().Length == 0)!);
private static ConstructorInfo? _listCtor;
#endif

private static Array CreateEmptyArray(Type itemType, int capacity = 0)
Expand All @@ -25,14 +22,21 @@ private static Array CreateEmptyArray(Type itemType, int capacity = 0)
private static IList CreateEmptyList(Type listType)
{
#if NET6_0_OR_GREATER
var ctor = (ConstructorInfo)listType.GetMemberWithSameMetadataDefinitionAs(_listCtor.Value);
ConstructorInfo? listCtor = _listCtor;
Copy link
Member

Choose a reason for hiding this comment

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

Are there reflection-based argument conversions used with the source generator support?

Copy link
Contributor

Choose a reason for hiding this comment

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

Source generator support is still pending. The original source generator work we did was superseded by improvements we've made to support trimming.


if (listCtor is null)
{
_listCtor = listCtor = typeof(List<>).GetConstructors().Single(c => c.GetParameters().Length == 0);
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved
}

var ctor = (ConstructorInfo)listType.GetMemberWithSameMetadataDefinitionAs(listCtor);
#else
var ctor = listType
.GetConstructors()
.SingleOrDefault(c => c.GetParameters().Length == 0);
#endif

return (IList)ctor.Invoke(new object[] { });
return (IList)ctor.Invoke(Array.Empty<object>());
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved
}

private static IList CreateEnumerable(Type type, Type itemType, int capacity = 0)
Expand Down
Loading