Reducing allocations in parsing by adamsitnik · Pull Request #2001 · dotnet/command-line-api

adamsitnik · 2022-12-20T20:18:50Z

While reading parsing logic I found plenty of places where we could remove some small allocations.

… when it's actually null

* don't allocate new string to remove brackets * don't allocate an array of characters (string.Split argument) * don't allocate an array of strings (result of string.Split)

…nternal methods that we control)

…it's creating a copy every time it's called!

adamsitnik · 2022-12-21T12:00:21Z

src/System.CommandLine/ParseResult.cs

        /// </summary>
        /// <remarks>If <see cref="CommandLineConfiguration.EnableDirectives"/> is set to <see langword="false"/>, then this collection will be empty.</remarks>
-        public IReadOnlyDictionary<string, IReadOnlyList<string>> Directives { get; }
+        public IReadOnlyDictionary<string, IReadOnlyList<string>> Directives => _directives ??= new ();


in the future we are going to use IReadOnlyDictionary<TKey, TValue>.Empty introduced very recently in .NET 8: dotnet/runtime#76028

Static Empty properties were not added to the interfaces after all, but ReadOnlyDictionary<TKey, TValue>.Empty was added.

adamsitnik · 2022-12-21T12:01:20Z

src/System.CommandLine/Invocation/TypoCorrection.cs

        public void ProvideSuggestions(ParseResult result, IConsole console)
        {
-            for (var i = 0; i < result.UnmatchedTokens.Count; i++)
+            var unmatchedTokens = result.UnmatchedTokens;


we need to store the result, because currently it's always allocating:

command-line-api/src/System.CommandLine/ParseResult.cs

Line 118 in 905cb0b

public IReadOnlyList<string> UnmatchedTokens => _unmatchedTokens.Select(t => t.Value).ToArray();

ofc I am going to propose a change to this API

adamsitnik · 2022-12-21T12:03:03Z

src/System.CommandLine/Parsing/CommandArgumentNode.cs

-            {
-                throw new ArgumentException($"Incorrect token type: {token}");
-            }
+            Debug.Assert(token.Type == TokenType.Argument, $"Incorrect token type: {token}");


these types are internal and we control the input, so there is no need to the throw condition. Debug.Assert will keep us safe (the CI runs all the tests in debug) with 0 cost for the Release build (less IL to compile, possibility to inline the method)

adamsitnik · 2022-12-21T12:04:22Z

src/System.CommandLine/Parsing/Parser.cs

        /// <returns>A <see cref="ParseResult"/> providing details about the parse operation.</returns>
        public ParseResult Parse(
-            IReadOnlyList<string> arguments,
+            IReadOnlyList<string>? arguments,


there is a test that verifies that the user can pass null here. So I made it more visible.

adamsitnik · 2022-12-21T12:05:14Z

src/System.CommandLine/Parsing/StringExtensions.cs

            var knownTokens = configuration.RootCommand.ValidTokens();

-            for (var i = 0; i < argList.Count; i++)
+            int i = FirstArgumentIsRootCommand(args, configuration.RootCommand, inferRootCommand)


the NormalizeRootCommand was allocating a new list just to ensure that the list of arguments starts with the root command alias.

adamsitnik · 2022-12-21T12:05:38Z

src/System.CommandLine/Parsing/StringExtensions.cs

-                        argList.InsertRange(i + 1, newTokens ?? Array.Empty<string>());
+                        if (newTokens is not null && newTokens.Count > 0)
+                        {
+                            List<string> listWithReplacedTokens = args.ToList();


this should be rare

adamsitnik · 2022-12-21T12:06:06Z

src/System.CommandLine/Parsing/StringExtensions.cs

-            var errorList = new List<string>();
+            const int FirstArgIsNotRootCommand = -1;
+
+            List<string>? errorList = null;


usually there are no errors, so we delay the allocation until first error is found

… the parsing, we take advantage of its mutability and remove the root command token instead of creating a copy of the whole list.

…cates a list)

…allocates a list)

… allocates a list)

…ntResult> duplicate the code rather than introducing new public type like IdentifierSymbolResult

jozkee · 2022-12-21T18:15:20Z

src/System.CommandLine/Command.cs

+                if (HasSubcommands)
                {
-                    AddCompletionsFor(commands[i]);
+                    var commands = Subcommands;


For all the vars that don't comply with dotnet/runtime guidelines, are you planning on addressing them all at once in the future?

Suggested change

var commands = Subcommands;

IList<Command> commands = Subcommands;

We need to discuss it with other contributors. And the usage of is { } instead is not null which drives me crazy ;p

src/System.CommandLine/Invocation/TypoCorrection.cs

src/System.CommandLine/ParseResult.cs

jozkee · 2022-12-21T18:41:18Z

src/System.CommandLine/ParseResult.cs

        /// </summary>
        /// <remarks>If <see cref="CommandLineConfiguration.EnableDirectives"/> is set to <see langword="false"/>, then this collection will be empty.</remarks>
-        public IReadOnlyDictionary<string, IReadOnlyList<string>> Directives { get; }
+        public IReadOnlyDictionary<string, IReadOnlyList<string>> Directives => _directives ??= new ();


Suggested change

public IReadOnlyDictionary<string, IReadOnlyList<string>> Directives => _directives ??= new ();

public IReadOnlyDictionary<string, IReadOnlyList<string>> Directives => _directives ??= new();

I think that this is a matter of personal preference. We should discuss it with others, choose one policy and enforce it with static code analysis tools.

Is there an analyzer for that yet? I don't see one listed at https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/csharp-formatting-options#spacing-options. (There is IDE0090 "Simplify new expression" for target-typed new, though.)

https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1000.md

src/System.CommandLine/Parsing/ParseOperation.cs

…ting a Union of the same hash set

adamsitnik added 14 commits December 20, 2022 17:37

delay error list allocation until it's needed

52af22d

add nullable annotation for Parser.Parse(arguments), avoid allocation…

105e592

… when it's actually null

minor Token cleanup

02c0daf

don't allocate a new list to normalize RootCommand

9741080

don't call EndsWith just to check one character

d6a5026

avoid allocations for parsing directives:

bc087b6

* don't allocate new string to remove brackets * don't allocate an array of characters (string.Split argument) * don't allocate an array of strings (result of string.Split)

delay List<SyntaxNode> allocation

c38ccc1

token type checks can be performed only for debug builds (these are i…

d0e99ee

…nternal methods that we control)

remove unused property that was allocating a list

e919201

delay List<OptionResult> allocation

11d72b0

delay List<ArgumentResult allocation

4a71f26

delay Dictionary<string, IReadOnlyList<string>> allocation

c0e3594

store the results of ParseResult.UnmatchedTokens and re-use it since …

e353656

…it's creating a copy every time it's called!

avoid allocations when there are no unmatched tokens

b9c4d42

adamsitnik requested a review from jozkee December 21, 2022 11:56

adamsitnik commented Dec 21, 2022

View reviewed changes

adamsitnik added 8 commits December 21, 2022 15:35

Since TokenizeResult.Tokens is not public and not used anywhere after…

ac725a3

… the parsing, we take advantage of its mutability and remove the root command token instead of creating a copy of the whole list.

remove TokenizeResult to reduce JIT time and allocations

fea7753

delay List<ParseError> allocation

a31e1ec

don't access Command.Options if there are no options defined (it allo…

1002343

…cates a list)

don't access Command.Arguments if there are no arguments defined (it …

30671e8

…allocates a list)

don't access Command.Subcommands if there are no commands defined (it…

8e096c8

… allocates a list)

delay List<SymbolResult> allocation

7eefbeb

delay List<Token> allocation

8e04f50

not every SymbolResult needs to contain a Dictionary<Argument, Argume…

d6f38a2

…ntResult> duplicate the code rather than introducing new public type like IdentifierSymbolResult

jozkee reviewed Dec 21, 2022

View reviewed changes

KalleOlaviNiemitalo mentioned this pull request Dec 22, 2022

Typo correction suggestions should go to IConsole.Error rather than IConsole.Out #2003

Open

adamsitnik added 3 commits December 23, 2022 14:14

Aliases are internally backed by a HashSet, there is no point in crea…

4afe5b5

…ting a Union of the same hash set

avoid list allocation

96c8e77

address code review feedback

0bb3b51

adamsitnik merged commit 88fb455 into dotnet:main Dec 23, 2022

adamsitnik deleted the allocations branch December 23, 2022 13:41

	var commands = Subcommands;
	IList<Command> commands = Subcommands;

	public IReadOnlyDictionary<string, IReadOnlyList<string>> Directives => _directives ??= new ();
	public IReadOnlyDictionary<string, IReadOnlyList<string>> Directives => _directives ??= new();

Conversation

adamsitnik commented Dec 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsitnik Dec 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adamsitnik Dec 21, 2022 •

edited

Loading