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

Conversation

adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Feb 23, 2022

The best way to review this PR is most probably to look at each commit separately. The changes belong to few categories:

  • extend ICommandHandlerwith synchronous Invoke method: this allows to avoid the need of loading and jitting all async machinery for users who don't want to use async overloads (I would imagine that most users don't need it as async should be used for potentially blocking IO operations)
  • not using static Lazy<T> fields, which get jitted when given type is referenced for the firs time. I've just moved the logic to getters and it's compiled when it's used. In worst case scenario users who for some reason use multiple threads to parse command line args are going to duplicate the initialization work.
  • reducing the number of types (to load and compile less). We could do more of that.
  • moving large, less-frequently used logic to separate methods, so it does not get compiled for common cases
  • not using structs. Every struct is of a different size and because of that, the JIT compiler needs to prepare a dedicated version of each generic collection (or other generic type that uses struct and type argument) for each unique value type. Changing Token from struct to class reduced the number of JIT compilations reported by Perfview by 20!

For the following app that mimics @jkotas example:

using System.CommandLine;
using System.CommandLine.Builder;
using System.CommandLine.Parsing;

public class Program
{
    static void Run(bool boolArg, string stringArg)
    {
        // not doing anything on purpose to focus on argument parsing
    }

    private static int Main(string[] args)
    {
        Option<bool> boolOption = new Option<bool>(new[] { "--bool", "-b" }, "Bool option");
        Option<string> stringOption = new Option<string>(new[] { "--string", "-s" }, "String option");

        RootCommand command = new RootCommand()
        {
            boolOption,
            stringOption
        };

        command.SetHandler<bool, string>(Run, boolOption, stringOption);

        return new CommandLineBuilder(command).Build().Invoke(args);
    }
}

the total execution time measured with Measure-Command for --bool true -s test arguments went down from 73 ms to 60-62 ms (Windows)

…educe the number of compilations and type loading
… time ArgumentConverter was used, no matter if we wanted to create a list or not)
# Conflicts:
#	src/System.CommandLine/Binding/ArgumentConverter.cs
@@ -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.

argument,
argument.Arity.MinimumNumberOfValues,
argument.Arity.MaximumNumberOfValues) is ArgumentConversionResult failed &&
failed is not null) // returns null on success
Copy link
Contributor

Choose a reason for hiding this comment

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

failed is not null check isn't needed. I think it's redundant with is ArgumentConversionResult failed which won't match if the return value of Validate is null.

@jonsequitur jonsequitur merged commit 3cb430c into dotnet:main Mar 1, 2022
@adamsitnik adamsitnik deleted the perfImprovements6 branch March 1, 2022 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants