Targeted perf changes to CommandLineParser #78446
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Will add more details when speedometer numbers come back from test insertion. Not expected to make a large difference, but this codepath is heavily executed during solution load, which is an area whose perf I'm attempting to improve.
ghost
added
Area-Compilers
untriaged
ToddGrun
changed the title
Contributor
Author
|
@dotnet/roslyn-compiler -- This is ready for review. |
Contributor
Author
|
@dotnet/roslyn-compiler -- PTAL, thanks! |
chsienki
approved these changes
May 7, 2025
Contributor
Author
|
@dotnet/roslyn-compiler -- need one more review, thanks! |
jjonescz
approved these changes
May 8, 2025
| } | ||
|
|
||
| if (!inQuotes && separators.IndexOf(c) >= 0) | ||
| else if (!inQuotes && separators.IndexOf(c) >= 0) |
Member
There was a problem hiding this comment.
Wouldn't Contains be even faster than IndexOf >= 0? Perhaps also SearchValues would make sense here.
Contributor
Author
There was a problem hiding this comment.
Switched to Contains in commit 2.
Thanks for the tip about SearchValues, I didn't know about that. I think I'm going to pass on that for now, but I'll keep that in the back pocket for the next time I see something like this.
2) Use Contains(c) instead of IndexOf(c) >= 0
jjonescz
reviewed
May 8, 2025
333fred
added a commit
to 333fred/roslyn
that referenced
this pull request
May 8, 2025
* upstream/main: (415 commits) Use lazy initialization for members in CodeFixService (dotnet#78484) Targeted perf changes to CommandLineParser (dotnet#78446) Exclude VS.ExternalAPIs.Roslyn.Package from source-build Add syntax highlighting of ignored directives (dotnet#78458) Fix unexpected conditional state in nullable analysis of conditional access (dotnet#78439) Update RoslynAnalyzers to use Roslyns version Fix source-build by renaming Assets folder to assets Unlist M.CA.AnalyzerUtilities as an expected DevDivInsertion dependency Use a project reference for M.CA.AnalyzerUtilities Rectify status of dictionary expressions (dotnet#78497) Remove unused field Remove unused field Remove using Fix check Update eng/config/PublishData.json Make internal Remove now unused methods from IWorkspaceProjectContext (dotnet#78479) Reduce allocations during SourceGeneration (dotnet#78403) Update Roslyn.sln Merge EditorFeatures.Wpf entirely into EditorFeatures ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CommandLineParser shows up as surprisingly expensive in the speedometer traces I've been looking at for solution load. This PR attempts a couple small, targeted fixes for a slight improvement.
IsOptionName was doing either 1 or 2 passes in the standard ascii case. Instead, do a length check upfront so that we commonly do 0 passes, and if the lengths match, do 1 pass in the ascii case. In the non-ascii case, fall back to using the ReadOnlySpan's Equals as the code did before. This is a small CPU win reflected by the first two images below.
TryParseOption's calls to IndexOf are showing up in the trace. We can limit the span we search significantly as colon typically is found towards the beginning of arg and arg can be quite long. Small CPU win again, reflected by the 3rd and 4th images below.
ParseSeparatedStrings is a very commonly tread codepath. Small micro-optimizations in the loop to not do the IndexOf call when c is a quote and to check the common condition first.
Test insertion PR: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/634213
*** Without IsOptionName change ***
*** With IsOptionName change ***
*** Without TryParseOption change ***
*** With TryParseOption change ***
