[efficiency-improver] perf: avoid string[1] allocation in Condition.Evaluate for single-string properties - #16179
Conversation
…ing properties When a test property value is a plain string (the common case for FullyQualifiedName, DisplayName, Source, etc.), Condition.Evaluate previously wrapped it in a new string[1] array before dispatching to EvaluateEqualOperation / EvaluateContainsOperation. This allocation happened on every test-case evaluation in the slow filter path (filters using '~', '!~', or mixed operators). Add a fast path that handles the string case inline, eliminating the transient string[1] per evaluated test case. The null and string[] cases are unchanged; non-string/non-array types retain the ToString() fallback for backward compatibility. Proxy metric: heap allocation count in the slow filter path. Expected reduction: ~1 string[1] (~24 bytes) per test case evaluated when a Contains/NotContains filter is active. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR improves the performance of the test filter evaluation slow path by avoiding a per-evaluation string[1] allocation when a filter property value is already a single string (common for properties like FullyQualifiedName, DisplayName, Source).
Changes:
- Adds a fast path in
Condition.EvaluateforpropertyValue is string, performing the operation directly without wrapping into a one-element array. - Keeps existing behavior for
nullandstring[]values, and preserves theToString()fallback for other types for backward compatibility. - Removes the now-unneeded
GetPropertyValuehelper.
Jakub Jareš (nohwnd)
left a comment
There was a problem hiding this comment.
Code Review — Condition.Evaluate fast path
Dimensions checked: Algorithmic Correctness · Performance & Allocations · Null Safety & Boundary Validation · Public API Surface Protection
Overall: ✅ Clean
The fast path is semantically equivalent to the removed GetPropertyValue helper across all cases. Verified against each code path:
| Property type | Old path | New path | Verdict |
|---|---|---|---|
string |
new string[1] { value } → EvaluateEqual/ContainsOperation([value]) |
Direct string.Equals / IndexOf |
Equivalent — single-element iteration is identical to direct comparison |
null |
Returns null from helper |
propertyValue switch { null => null, ... } |
Equivalent |
string[] |
Returns array as-is | string[] arr => arr |
Equivalent |
| other (ToString coercion) | new string[1] { x.ToString()! } |
new[] { x.ToString()! } |
Equivalent |
NoneFilterValue semantics preserved: EvaluateEqualOperation's null or { Length: 0 } guard is only reachable for null/empty-array property values. A string-typed property value never satisfies those conditions in either old or new code — the fast path skips that method entirely, which is safe.
All four operators verified: Equal, NotEqual, Contains, NotContains in the fast path produce identical boolean results to the indirect path for any single string value.
Value null-safety: Value is annotated string (non-nullable), constructed via FilterHelper.Unescape which returns string. IndexOf(Value, ...) and string.Equals(..., Value, ...) are safe. ✓
No public API changes: Condition is internal sealed. No PublicAPI.Unshipped.txt changes needed. ✓
🧠 Reviewed by expert-reviewer — Algorithmic Correctness, Performance & Allocations, Null Safety, Public API Surface
🧠 Reviewed by Expert Code Reviewer 🧠
Jakub Jareš (nohwnd)
left a comment
There was a problem hiding this comment.
Code Review — Condition.Evaluate fast path
Dimensions checked: Algorithmic Correctness · Backward Compatibility & Rollback Safety · Null Safety & Boundary Validation · Performance & Allocations · Cross-TFM Compatibility · Public API Surface Protection
Overall: ✅ Clean
The fast path is semantically equivalent to the removed GetPropertyValue helper across all property value types and all operators. Verified:
| Property type | Old path | New path | Verdict |
|---|---|---|---|
string |
new string[1] { value } → EvaluateEqual/ContainsOperation([value]) |
Direct string.Equals / IndexOf |
Equivalent |
null |
Returns null from helper |
propertyValue switch { null => null, ... } |
Equivalent |
string[] |
Returns array as-is | string[] arr => arr |
Equivalent |
| other (ToString coercion) | new string[1] { x.ToString()! } |
new[] { x.ToString()! } |
Equivalent |
NoneFilterValue semantics preserved: The null or { Length: 0 } guard in EvaluateEqualOperation is only reachable for null or empty-array property values. A non-null string typed property value never satisfies that guard in either the old or new code — the fast path is safe to skip EvaluateEqualOperation entirely for this case.
All four operators verified: Equal, NotEqual, Contains, NotContains in the fast path produce identical boolean results for any single string value.
Null safety: singleValue is non-null by pattern match precondition. Value is annotated string (non-nullable) and constructed via FilterHelper.Unescape. IndexOf and string.Equals calls are safe.
Cross-TFM: string.Equals(s1, s2, StringComparison) and string.IndexOf(s, StringComparison) are available on all targeted TFMs (net462, netstandard2.0, net8.0+). Switch expressions require C# 8+, already in use.
No public API changes: Condition is internal sealed. No PublicAPI.Unshipped.txt changes needed.
No binding redirect implications: No package changes; this is a pure logic optimization.
Description alignment: Title and description accurately describe the change. The approach mirrors FastFilter.TryGetPropertyValue from #16160 as stated.
🧠 Reviewed by Expert Code Reviewer 🧠
🧠 Reviewed by Expert Code Reviewer 🧠
Goal and Rationale
When a test property value is a plain
string(the common case forFullyQualifiedName,DisplayName,Source, etc.),Condition.Evaluatepreviously wrapped it in anew string[1]array before dispatching toEvaluateEqualOperation/EvaluateContainsOperation. This allocation occurred on every test-case evaluation in the slow filter path — i.e., filters using~(Contains),!~(NotContains), or mixed boolean operators.The
~operator is the most commonly used developer filter (--filter "FullyQualifiedName~MyTest"), making this a high-frequency allocation.Focus Area
Code-Level Efficiency — unnecessary object creation per evaluated test case.
Approach
Add a fast path in
Condition.Evaluatethat handles thestringcase directly, without wrapping:The
nullandstring[]cases continue throughEvaluateEqualOperation/EvaluateContainsOperationunchanged. Non-string/non-array types retain theToString()fallback for backward compatibility. The now-unused privateGetPropertyValuehelper is removed.This mirrors the approach already taken in
FastFilter.TryGetPropertyValue(merged in #16160), which also avoids allocation for single-valued properties.Energy Efficiency Evidence
Proxy metric: heap allocation count in the slow filter path (less memory churn → less GC pressure → less CPU energy for collection).
Before:
GetPropertyValueallocatesnew string[1]+ assigns[0]for every non-string[]property value.After:
stringproperties go through the fast path with zero allocation.Estimated reduction: ~1
string[1](~24 bytes) per test case evaluated when a Contains/NotContains filter is active. For a 10 K-test run with--filter "FullyQualifiedName~Test":Green Software Foundation context — Hardware Efficiency: reducing allocations makes better use of DRAM bandwidth and CPU cache by reducing GC scan pressure proportional to the working set size.
Trade-offs
GetPropertyValue.Reproducibility
Test Status
Microsoft.TestPlatform.Common.UnitTests,net11.0)Microsoft.TestPlatform.Filter.Source.UnitTests,net11.0)