Reduce allocations in CSharp command line parsing by jaredpar · Pull Request #54675 · dotnet/roslyn

jaredpar · 2021-07-08T00:28:48Z

The C# parser is used by the project system in scenarios like solution
open. That means the allocations in command line parsing can contribute
significantly to Visual Studio performance. This PR reduces the
allocations significantly.

Note: in the below explanations when I refer to the "99% case" I am
referring to how command lines are structured when created by
MSBuild based builds. That is the overwhelming case for the compiler
and is advantageous because it normalizes many items like lower casing
all option names, having one reference per option, etc ... The remaining
1% are hand authored build files and these should not impact Visual
Studio scenarios.

The first fix is to simply avoid iterators in the parse hot paths. Most
of the iterators in the parser return a single element in the 99% case
hence the iterator is wasted allocations. These were switched to take
pooled builder arguments.

The next, and more siginificant fix, is to use ReadOnlyMemory<char>
instead of string on our hot parsing paths. This allows us to parse
the command line arguments without actually allocating memory for the
values until actually needed by lower level APIs. This allowed for APIs
like RemoveQuotesAndSlashes to become allocation free in the 99% case
(it's just a slicing operation now, not a string allocation).

The hot paths in our parsing were updated to employ these techniques.

The one downside of the change is that I had to touch virtually every
case "someOption" where the option value was used. The value is now
held in a ReadOnlyMemory<char> until it's actually needed as a
string. Hence every one of these cases either needed to use the new
ReadOnlyMemory<char> APIs or force the allocation of the string.
That made the change longer than I would've liked but it's also fairly
mechanical.

To test out the changes I created a benchmark which parsed a couple of
command lines:

Command line for building Microsoft.CodeAnalysis.CSharp
Command line for a simple razor app

The results are below and represent a significant win in both
allocations and in simple performance. There are a few more places we
could get some wins but getting into the realm of diminishing returns at
this point.

Before

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
ParseCSharpCompilerCommandLine	1,101.9 us	16.05 us	14.23 us	242.1875	80.0781	-	876 KB
ParseCSharpCompilerResponseFile	1,916.7 us	18.26 us	14.26 us	353.5156	175.7813	-	2,082 KB
ParseRazorCompilerCommandLine	467.3 us	2.27 us	2.02 us	350.5859	53.7109	-	571 KB
ParseRazorCompilerResponseFile	850.6 us	8.09 us	7.17 us	327.1484	102.5391	-	953 KB

After

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
ParseCSharpCompilerCommandLine	833.7 us	16.53 us	31.04 us	111.3281	18.5547	-	200 KB
ParseCSharpCompilerResponseFile	1,631.5 us	11.42 us	10.69 us	167.9688	78.1250	1.9531	891 KB
ParseRazorCompilerCommandLine	257.6 us	3.44 us	3.05 us	80.0781	14.6484	-	136 KB
ParseRazorCompilerResponseFile	600.0 us	5.59 us	5.23 us	119.1406	33.2031	-	364 KB

There is a Gen2 increase here but I believe that is just the new usage of pooled builders.

Closes #53570

Context dotnet#53570 The C# parser is used by the project system in scenarios like solution open. That means the allocations in command line parsing can contribute significantly to Visual Studio performance. This PR reduces the allocations significantly. Note: in the below explanations when I refer to the "99% case" I am referring to how command lines are structured when created by MSBuild based builds. That is the **overwhelming** case for the compiler and is advantageous because it normalizes many items like lower casing all option names, having one reference per option, etc ... The remaining 1% are hand authored build files and these should not impact Visual Studio scenarios. The first fix is to simply avoid iterators in the parse hot paths. Most of the iterators in the parser return a single element in the 99% case hence the iterator is wasted allocations. These were switched to take pooled builder arguments. The next, and more siginificant fix, is to use `ReadOnlyMemory<char>` instead of `string` on our hot parsing paths. This allows us to parse the command line arguments without actually allocating memory for the values until actually needed by lower level APIs. This allowed for APIs like `RemoveQuotesAndSlashes` to become allocation free in the 99% case (it's just a slicing operation now, not a string allocation). The hot paths in our parsing were updated to employ these techniques. The one downside of the change is that I had to touch virtually every `case "someOption"` where the option value was used. The value is now held in a `ReadOnlyMemory<char>` until it's actually needed as a `string`. Hence every one of these cases either needed to use the new `ReadOnlyMemory<char>` APIs or force the allocation of the `string`. That made the change longer than I would've liked but it's also fairly mechanical. To test out the changes I created a benchmark which parsed a couple of command lines: - Command line for building Microsoft.CodeAnalysis.CSharp - Command line for a simple razor app The results are below and represent a significant win in both allocations and in simple performance. There are a few more places we could get some wins but getting into the realm of diminishing returns at this point. Before | Method | Mean | Error | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated | |------------------------------- |-----------:|---------:|---------:|---------:|--------:|------:|----------:| | ParseCSharpCompilerCommandLine | 1,127.3 us | 20.52 us | 24.42 us | 250.0000 | 80.0781 | - | 876 KB | | ParseRazorCompilerCommandLine | 486.1 us | 7.19 us | 6.38 us | 350.0977 | 56.6406 | - | 571 KB | After | Method | Mean | Error | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated | |------------------------------- |---------:|--------:|--------:|---------:|--------:|------:|----------:| | ParseCSharpCompilerCommandLine | 630.3 us | 8.54 us | 7.57 us | 147.4609 | 14.6484 | - | 223 KB | | ParseRazorCompilerCommandLine | 232.4 us | 0.73 us | 0.61 us | 86.1816 | 14.1602 | - | 149 KB | closses dotnet#53570

jaredpar · 2021-07-08T00:30:06Z

@davkean FYI

jaredpar · 2021-07-08T23:31:53Z

@dotnet/roslyn-compiler PTAL

src/Compilers/CSharp/Portable/CommandLine/CSharpCommandLineParser.cs

chsienki · 2021-07-08T23:55:35Z

src/Compilers/CSharp/Portable/CommandLine/CSharpCommandLineParser.cs

            }

-            foreach (string path in ParseSeparatedPaths(switchValue))
+            foreach (string path in ParseSeparatedPaths(switchValue.Value.ToString()))


Do we want to get rid of the iterator allocation here also?

We certainly could but this is not really a hot path, or even a common path, for C# compilation. To keep the churn down I'd been focusing on areas of the argument parsing that hit hot paths. This particular argument doesn't get hit at all in modern MSBuild driven scenarios that I'm aware of.

I'm certainly happy to push further here if you think it's worth the code churn.

src/Compilers/CSharp/Portable/CommandLine/CSharpCommandLineParser.cs

src/Compilers/Core/CodeAnalysisTest/CommonCommandLineParserTests.cs

src/Compilers/Core/Portable/CommandLine/CommandLineParser.cs

jaredpar · 2021-07-09T06:02:39Z

@chsienki thanks! Responded to the feedback.

333fred

Done review pass (commit 7)

src/Compilers/CSharp/Portable/CommandLine/CSharpCommandLineParser.cs

333fred · 2021-07-09T17:39:09Z

src/Compilers/CSharp/Portable/CommandLine/CSharpCommandLineParser.cs

@@ -192,7 +217,7 @@ internal sealed override CommandLineArguments CommonParse(IEnumerable<string> ar
                        continue;

                    case "langversion":


IMO, it feels like langversion could be common enough to special case as well. Did you have a reason for not doing this one?

Mostly I was going for option names that were likely to appear multiple times in standard compilation requests. For example /reference was an easy win because there are hundreds of those per compilation. The next one after that was /analyzer. After that though pretty much every other option is used only once. Wasn't making big impact.

Don't have any objections to pushing more up to the fast path though.

That makes sense and seems like a reasonable cutoff.

src/Compilers/CSharp/Portable/CommandLine/CSharpCommandLineParser.cs

333fred · 2021-07-09T17:42:43Z

src/Compilers/CSharp/Portable/CommandLine/CSharpCommandLineParser.cs

                            checkOverflow = false;
                            continue;

                        case "nullable":


Nullable also feels like it would be a common option. It's a bit more complex, but did you consider doing this one ahead of time as well?

Similar to "langversion".

333fred · 2021-07-09T17:44:13Z

src/Compilers/CSharp/Portable/CommandLine/CSharpCommandLineParser.cs


                        case "nowarn":
-                            if (value == null)
+                            if (valueMemory is null)


nowarn will also be super common, as the SDK sets this for every C# program in existance.

src/Compilers/Core/Portable/CommandLine/CommandLineParser.cs

jaredpar · 2021-07-09T23:20:12Z

After reading through a few of the comments I did some more profiling and found I could save a lot more allocations by avoiding the TextReader.ReadLine calls. Under the hood that uses a lot of StringBuilder and there was really no reason for that. The majority of lines can be parsed into a stack based Span<char> and parsed out directly from there. That produced a significant allocation savings

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
ParseCSharpCompilerCommandLine	771.7 us	15.09 us	19.62 us	109.3750	20.5078	-	200 KB
ParseCSharpCompilerResponseFile	2,094.4 us	20.04 us	18.74 us	105.4688	39.0625	-	551 KB
ParseRazorCompilerCommandLine	249.5 us	2.49 us	2.33 us	77.6367	14.6484	-	136 KB
ParseRazorCompilerResponseFile	710.1 us	5.97 us	5.29 us	88.8672	24.4141	-	251 KB

src/Compilers/Core/Portable/CommandLine/CommandLineParser.cs

Co-authored-by: Fred Silberberg <fred@silberberg.xyz>

jaredpar · 2021-07-13T23:06:56Z

@chsienki any other feedback here that I missed?

jaredpar added the Area-Compilers label Jul 8, 2021

jaredpar requested a review from a team as a code owner July 8, 2021 00:28

jaredpar marked this pull request as draft July 8, 2021 00:33

Progress

d56736b

jaredpar force-pushed the cl branch from a0c5206 to d56736b Compare July 8, 2021 22:48

Response file parsing

eb7b339

jaredpar marked this pull request as ready for review July 8, 2021 23:31

Fix build error

554ce33

chsienki reviewed Jul 8, 2021

View reviewed changes

src/Compilers/CSharp/Portable/CommandLine/CSharpCommandLineParser.cs Outdated Show resolved Hide resolved

chsienki reviewed Jul 8, 2021

View reviewed changes

src/Compilers/CSharp/Portable/CommandLine/CSharpCommandLineParser.cs Outdated Show resolved Hide resolved

chsienki reviewed Jul 8, 2021

View reviewed changes

src/Compilers/Core/CodeAnalysisTest/CommonCommandLineParserTests.cs Outdated Show resolved Hide resolved

chsienki reviewed Jul 9, 2021

View reviewed changes

src/Compilers/Core/Portable/CommandLine/CommandLineParser.cs Outdated Show resolved Hide resolved

PR feedback

f9bce81

Linux test failure

34a3e54

jaredpar mentioned this pull request Jul 9, 2021

Unescape interpolated string literal components #54706

Merged

Fix test crash

b6cad76

333fred reviewed Jul 9, 2021

View reviewed changes

PR feedback

beeb92b

Parse response file wins

01a4b0e

333fred approved these changes Jul 9, 2021

View reviewed changes

src/Compilers/Core/Portable/CommandLine/CommandLineParser.cs Outdated Show resolved Hide resolved

jaredpar and others added 2 commits July 9, 2021 20:52

Update src/Compilers/Core/Portable/CommandLine/CommandLineParser.cs

2a910d7

Co-authored-by: Fred Silberberg <fred@silberberg.xyz>

Fixed build correctness issue

e90e01f

jaredpar requested a review from a team as a code owner July 13, 2021 23:06

chsienki approved these changes Jul 13, 2021

View reviewed changes

jaredpar enabled auto-merge (squash) July 13, 2021 23:52

jaredpar merged commit 043b60e into dotnet:main Jul 14, 2021

ghost added this to the Next milestone Jul 14, 2021

jaredpar deleted the cl branch July 14, 2021 15:06

RikkiGibson mentioned this pull request Jul 26, 2021

Support Pattern Matching ReadOnlySpan<char> on constant string #44388

Merged

allisonchou modified the milestones: Next, 17.0.P3 Jul 27, 2021

KirillOsenkov mentioned this pull request Dec 23, 2022

[Responsiveness] Further improve CSharpCommandLineParser.Parse() perf #66110

Closed

		@@ -192,7 +217,7 @@ internal sealed override CommandLineArguments CommonParse(IEnumerable<string> ar
		continue;

		case "langversion":

Conversation

jaredpar commented Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaredpar commented Jul 8, 2021

Uh oh!

jaredpar commented Jul 8, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jaredpar commented Jul 9, 2021

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

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!

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!

Uh oh!

Uh oh!

Uh oh!

jaredpar commented Jul 9, 2021

Uh oh!

Uh oh!

jaredpar commented Jul 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jaredpar commented Jul 8, 2021 •

edited

Loading