Skip to content

Conversation

@Erarndt
Copy link
Contributor

@Erarndt Erarndt commented Jul 1, 2025

Context

The usage patterns in ExtractFunctionArguments() cause significant allocations due to repeated, small, intermediate allocations. Luckily, we can update SpanBasedStringBuilder to avoid a majority of these allocations.

Before:
image

After:
image

Changes Made

Testing

Notes

Copilot AI review requested due to automatic review settings July 1, 2025 17:37
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR optimizes SpanBasedStringBuilder by adding direct indexing, enumeration, equality, and character-based trim operations to reduce intermediate allocations, and updates Expander to use these new features for argument parsing.

  • Added an indexer, GetEnumerator, Equals overloads, and Trim(char) methods in SpanBasedStringBuilder
  • Refactored Expander to remove temporary quote-character arrays and leverage SpanBasedStringBuilder methods

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/StringTools/SpanBasedStringBuilder.cs Enhanced with an indexer, enumerator, span- and string-based Equals, and Trim(char) overloads to avoid allocations
src/Build/Evaluation/Expander.cs Updated argument parsing to call SpanBasedStringBuilder APIs directly, removed static quote-char arrays
Comments suppressed due to low confidence (3)

src/StringTools/SpanBasedStringBuilder.cs:123

  • [nitpick] The public indexer is missing XML documentation. Consider adding a tag explaining how indexing works and which exceptions are thrown.
        public char this[int index]

src/StringTools/SpanBasedStringBuilder.cs:312

  • [nitpick] This overload is missing XML documentation. Add a description matching the existing parameterless TrimStart for consistency.
        public void TrimStart(char c)

src/StringTools/SpanBasedStringBuilder.cs:123

  • Introduce unit tests covering the new indexer, all Equals overloads, and Trim(char) methods to validate behavior and prevent regressions.
        public char this[int index]

@YuliiaKovalova YuliiaKovalova merged commit 04e602d into dotnet:main Jul 2, 2025
9 checks passed
@Erarndt Erarndt deleted the dev/erarndt/spanBasedStringBuilder branch July 2, 2025 15:51
rainersigwald added a commit to rainersigwald/MSBuildLocator that referenced this pull request Nov 4, 2025
An internal tool recently had trouble updating to .NET 10 because
it deployed a stale local copy of `Microsoft.NET.StringTools.dll` that
was working (by coincidence) throughout .NET 9 but failed after
dotnet/msbuild#12100 added some API surface to StringTools and used it.
YuliiaKovalova pushed a commit to microsoft/MSBuildLocator that referenced this pull request Nov 6, 2025
* Add StringTools to do-not-deploy list

An internal tool recently had trouble updating to .NET 10 because
it deployed a stale local copy of `Microsoft.NET.StringTools.dll` that
was working (by coincidence) throughout .NET 9 but failed after
dotnet/msbuild#12100 added some API surface to StringTools and used it.

* Add nuget dependency as well

---------

Co-authored-by: Chet Husk <[email protected]>
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.

4 participants