Replace regex-based metadata expansion with manual scanning - #14116
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors MSBuild’s evaluation-time metadata expansion to remove regex usage and replace it with a manual, allocation-conscious scanner, aiming to reduce per-expansion overhead in the Expander hot path. It also updates related transform parsing and adds new microbenchmarks to measure the impact.
Changes:
- Replaced regex-driven
%(...)metadata expansion with a manual scanner inExpander.MetadataExpander. - Centralized metadata token parsing via
ExpressionShredder.TryParseMetadataExpressionand updated item transform metadata matching to avoid regex. - Added
ExpanderBenchmarkto quantify perf/alloc changes; removed now-unused regex helper files and project includes.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/MSBuild.Benchmarks/ExpanderBenchmark.cs | Adds new benchmarks covering property/item/metadata/mixed expansion scenarios. |
| src/Build/Microsoft.Build.csproj | Removes compile includes for deleted regex-based expander helpers. |
| src/Build/Evaluation/ExpressionShredder.cs | Adds a metadata-expression parser helper and switches to Strings.WeakIntern usage. |
| src/Build/Evaluation/Expander.RegularExpressions.cs | Removes regex infrastructure previously used by metadata expansion. |
| src/Build/Evaluation/Expander.MetadataMatchEvaluator.cs | Removes regex match-evaluator infrastructure used for metadata replacement. |
| src/Build/Evaluation/Expander.MetadataExpander.cs | Rewrites metadata expansion to a manual scanner and gap-handling logic around item vectors. |
| src/Build/Evaluation/Expander.ItemExpander.Transforms.OneOrMultipleMetadataMatches.cs | Updates match container to support manual-scan match info. |
| src/Build/Evaluation/Expander.ItemExpander.Transforms.MetadataMatchType.cs | Minor formatting/structure updates for match-kind enum. |
| src/Build/Evaluation/Expander.ItemExpander.Transforms.MetadataMatch.cs | Replaces regex Match-based storage with index/length storage. |
| src/Build/Evaluation/Expander.ItemExpander.Transforms.cs | Replaces regex matching for quoted expression metadata with manual scanning + TryParseMetadataExpression. |
jankratochvilcz
left a comment
There was a problem hiding this comment.
Expert MSBuild review (automated)
High-quality, faithful refactor. The hand-written ref struct scanner reproduces the previous regex behavior across the edge cases I checked (qualified vs. unqualified metadata, internal/surrounding whitespace, nested %(a%(b)), malformed %()/%(, built-in-vs-custom gating, truncation, and self-reference logging). I verified parity empirically: the project builds with 0 warnings, and Expander_Tests + ExpressionShredder_Tests produce the same pre-existing macOS-platform failures on this branch as on origin/main — i.e. no regressions introduced.
String-comparison correctness is preserved (MSBuildNameIgnoreCaseComparer for the metadata dictionary, IsItemSpecModifier case-insensitive, ordinal for the self-reference item-type check and the %(/@( pre-scans). Name-character validation via XmlUtilities.IsValid*ElementNameCharacter is equivalent to the old regex's [A-Za-z_][A-Za-z_0-9\-]*.
The one substantive item worth addressing before merge is the avoidable double-parse of the item-vector expressions on the mixed @(…%( path (mildly counter to the PR's perf goal). The rest are nits. Nice addition of ExpanderBenchmark to back the perf claims. No blocking issues.
…sion Introduces ExpanderBenchmark to MSBuild.Benchmarks covering: - Property expansion (single, multiple, nested, concatenation) - Item list expansion (simple, transform, custom separator) - Metadata expansion (unqualified, qualified, multiple) - Mixed expressions (property+item, property+metadata, all) - ExpandIntoStringAndUnescape variants for unescape overhead - NoExpansion baseline Parameterized on PropertyCount and ItemCount to measure scaling.
Eliminate all regex usage from metadata expansion by replacing it with a manual span-based scanner that delegates to a shared parsing method. - Extract TryParseMetadataExpression on ExpressionShredder as the single shared metadata parsing implementation, using the existing Sink pattern and Strings.WeakIntern to avoid allocations. Refactor ExpressionShredder's own inline metadata parsing to call the new method. - Refactor MetadataExpander from a static class to a readonly ref struct, capturing expansion context in fields and converting static methods to instance methods. Also, annotate for nullability. - Inline MetadataMatchEvaluator logic into MetadataExpander and delete the type - Rewrite Transforms.GetQuotedExpressionMatches to use manual scanning instead of ItemMetadataRegex - Delete Expander.RegularExpressions.cs (entirely dead code) and Expander.MetadataMatchEvaluator.cs - Replace s_invariantCompareInfo.IndexOf with string.Contains for simple ASCII substring checks
Declare return type of MetadataExpander.ExpandMetadataLeaveEscaped as `string` rather than `string?`.
ScanAndExpandMetadataInGaps re-ran ExpressionShredder.GetReferencedItemExpressions from scratch even though Expand had already shredded the expression to test the single-item-vector fast path. For any expression containing "@(" that isn't a lone full vector (e.g. "prefix@(Compile)" or "@(A);@(B)"), that parsed the first capture and interned its "@(...)" substring twice, throwing away the first pass.
Thread the already-advanced enumerator into ScanAndExpandMetadataInGaps so the shred happens exactly once, restoring the single-pass behavior of the original ExpandMetadataLeaveEscaped. The enumerator is passed by ref and consumed with a do/while since it is positioned at the first capture on entry.
The no-capture case (string contains "@(" but no well-formed item vector, e.g. "%(Culture)@(") is handled explicitly by scanning the whole string for metadata, so malformed-but-benign input keeps working rather than throwing.
The PR that replaced the metadata-expansion regex with a hand-written character scanner added only a benchmark, leaving the scanner's branches without explicit coverage. Add focused Theories over Expander.MetadataExpander that pin the exact expanded result so future edits can't silently regress them:
- ExpandMetadata_ScannerEdgeCases: surrounding/internal whitespace, malformed references (%(, %(), %( ), %(.x)), nested references (%(Culture%(Foo))), qualified vs. unqualified names, and missing metadata.
- ExpandMetadata_ItemVectorGapsAndSeparators: metadata before/after/between item vectors, the lone-vector fast path, metadata embedded in a separator, and a stray "@(" that doesn't form a valid item vector.
- ExpandMetadata_BuiltInVsCustomGating: built-in vs. custom expansion is gated on the matching ExpanderOptions flag (declared internal since ExpanderOptions is internal; xUnit discovers non-public test methods).
Replacing the metadata-expansion regex with a hand-written scanner dropped the "keep in sync with ProjectWriter" note that the deleted Expander.RegularExpressions.cs carried. The scanner validates item type and metadata names via ExpressionShredder.SinkValidName (XmlUtilities.IsValidInitial/SubsequentElementNameCharacter), which is exactly [A-Za-z_][A-Za-z_0-9\-]* — equivalent to ProjectWriter.itemTypeOrMetadataNameSpecification today. Restore the coupling note in three places so the two grammars can't silently diverge: - ExpressionShredder.SinkValidName (the shared validator) - Expander.MetadataExpander.ScanAndExpandMetadata remarks - ProjectWriter.itemTypeOrMetadataNameSpecification (back-reference) Documentation only; no behavior change.
The end parameter was only respected by SinkValidName; SinkWhitespace and the single-char Sink bounded on expression.Length, so the metadata parser could read past end. This was benign for current callers (the gap scan always has '@' at end, which fails the name/'.'/')' checks before any over-read), but it was a latent footgun now that this is a shared, named internal helper with an explicit end. Add end-aware overloads Sink(expression, ref i, end, c) and SinkWhitespace(expression, ref i, end), and route TryParseMetadataExpression through them so no character at or beyond end is read. The existing Length-bounded overloads now delegate to the end-aware versions, removing duplicated bounds logic. Also tidy the surrounding XML doc comments. No behavior change for existing callers.
The de-regexed transform scanner (GetQuotedExpressionMatches) had no test covering its qualified-metadata error path. Add a Theory asserting that a metadata reference qualified with an item name inside a transform throws InvalidProjectFileException, and that the message reports both the offending reference and the suggested unqualified form. Cover the internal-whitespace variant (%( i . Meta0 )) so the scanner's whitespace handling on this path is pinned too.
820a99f to
140da40
Compare
Replace the character-by-character for-loop in ScanAndExpandMetadata with a while loop driven by string.IndexOf("%(", ...), so the search for the next metadata reference benefits from vectorized scanning instead of inspecting each character in managed code. Behavior is unchanged: a "%(" at endIndex - 1 still can't form a reference, failed parses resume past the "%(", and successful expansions resume after the reference.
|
@jankratochvilcz triage: Do you approve? |
Summary
Replaces the regex-based metadata expansion in the Expander with a zero-allocation
ref structscanner that performs manual character parsing. This eliminates theRegex.Replacecall and associatedMatchEvaluatordelegate allocations that occurred on every metadata expression expansion.Changes
Expander.MetadataMatchEvaluator.csandExpander.RegularExpressions.cs— regex infrastructure is no longer neededExpander.MetadataExpander.csusing aref structscanner that walks the expression character-by-character, identifying%(...)patterns without allocatingMetadataMatch,MetadataMatchType, andOneOrMultipleMetadataMatchesstructsExpanderBenchmark.cswith 18 benchmarks covering property, item, metadata, and mixed expansion scenariosBenchmark Results
.NET 10.0 (PropertyCount=10, ItemCount=10)
.NET Framework 4.7.2 (PropertyCount=10, ItemCount=10)
.NET 10.0 Baseline (before changes)
.NET 10.0 (after changes)
.NET Framework 4.7.2 Baseline (before changes)
.NET Framework 4.7.2 (after changes)
Highlights