Slight allocation reduction in CommandLineParser.FlattenArgs #79139
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
This code previously created an ArrayBuilder over the rawArguments given and did some funky manipulations in support of response file handling. This PR removes that extra ArrayBuilder and it's associated allocations (these lists end up being large enough to exceed the pooling threshold). I think the new code is also a bit easier to understand, particularly in the non-response file section of the code. (No copying into a list, reversing that list, walking it backwards, walking another list backwards, copying into an unused section of the first list, making that section used again, etc) This only reduces allocations by ~5 MB when opening the roslyn sln, but it also seems simpler and easier to understand.
AlekseyTs
reviewed
Jun 26, 2025
| args.ReverseContents(); | ||
| var argsIndex = args.Count - 1; | ||
| while (argsIndex >= 0) | ||
| var argsEnumerator = rawArguments.GetEnumerator(); |
There was a problem hiding this comment.
Not needed with recursive simplificiation
AlekseyTs
reviewed
Jun 26, 2025
| while (argsEnumerator.MoveNext()) | ||
| notProcessedSplitList.Add(argsEnumerator.Current); | ||
|
|
||
| argsEnumerator = notProcessedSplitList.GetEnumerator(); |
There was a problem hiding this comment.
Not needed with recursive simplificiation
AlekseyTs
reviewed
Jun 26, 2025
| // Ignores /noconfig option specified in a response file | ||
| if (!string.Equals(newArg, "/noconfig", StringComparison.OrdinalIgnoreCase) && !string.Equals(newArg, "-noconfig", StringComparison.OrdinalIgnoreCase)) | ||
| if (string.Equals(newArg, "/noconfig", StringComparison.OrdinalIgnoreCase) || string.Equals(newArg, "-noconfig", StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
AlekseyTs
reviewed
Jun 26, 2025
| var argsIndex = args.Count - 1; | ||
| while (argsIndex >= 0) | ||
| var argsEnumerator = rawArguments.GetEnumerator(); | ||
| while (argsEnumerator.MoveNext()) |
There was a problem hiding this comment.
I would like to suggest an alternative approach.
- Extract the body of this loop into a local function, let's call it "void processArg(string arg)". It should start with trimming the
arg, and do what the body of the loop does, assuming that acontinueis replaced with areturn. - Change this loop to plain
foreachoverrawArguments, calling the new local function as the body. - Inside
parseResponseFile, instead of doingnotProcessedSplitList.Add(newArg);, callprocessArg(newArg). #Closed
There was a problem hiding this comment.
I like this idea, implemented in commit 2. Makes the code even easier to understand.
|
Done with review pass (commit 1) |
333fred
approved these changes
Jun 26, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This code previously created an ArrayBuilder over the rawArguments given and did some funky manipulations in support of response file handling.
This PR removes that extra ArrayBuilder and it's associated allocations (these lists end up being large enough to exceed the pooling threshold). I think the new code is also a bit easier to understand, particularly in the non-response file section of the code. (No copying into a list, reversing that list, walking it backwards, walking another list backwards, copying into an unused section of the first list, making that section used again, etc)
This only reduces allocations by ~5 MB when opening the roslyn sln, but it also seems simpler and easier to understand.