[efficiency-improver] perf: avoid redundant dictionary lookups in FastFilter.Evaluate

[Jakub Jareš (nohwnd)]

Goal and Rationale

FastFilter.Evaluate is called once per test case whenever a test filter is active (e.g. --filter "TestCategory=UnitTests"). The previous implementation iterated over FilterProperties.Keys and then called FilterProperties[name] 2–3 times per property per call, resulting in repeated hash-compute-and-compare dictionary lookups.

For a test suite with 10,000 tests and a single-property filter, this is ~20,000–30,000 extra dictionary lookups that can be eliminated with no behaviour change.

Focus Area

Code-Level Efficiency — eliminating redundant computation in a tight per-test-case loop.

Approach

Two changes in FastFilter.cs:

1. Evaluate — changed foreach (var name in FilterProperties.Keys) to foreach (var kvp in FilterProperties), obtaining the value set once per property (kvp.Value) instead of looking it up via FilterProperties[name] on each access.

2. ValidForProperties — replaced the double-scan pattern

   FilterProperties.Keys.All(name => properties.Contains(name))
       ? null
       : FilterProperties.Keys.Where(name => !properties.Contains(name)).ToArray();

   with a single-pass:

   string[] invalid = FilterProperties.Keys.Where(name => !properties.Contains(name)).ToArray();
   return invalid.Length == 0 ? null : invalid;

   ValidForProperties is called once per test source (not per test case), so the impact here is smaller, but the simplification is clear.

Energy Efficiency Evidence

Proxy metric: CPU instruction count / execution time (faster code = fewer CPU cycles = less energy drawn).

ImmutableDictionary<TKey, TValue> lookup involves:

1. Computing the key hash (string hashing — O(n) over key length)
2. Bucket lookup
3. Key equality comparison

For the common single-string-value case the previous code did 2 lookups per property per Evaluate call; the new code does 0. For the multi-value case it did 3 lookups; now 0. Given Evaluate is called O(tests) times per run, and typical filters have 1–3 properties, savings compound across large test suites.

Reproducibility: Run dotnet test --filter TestCategory=... against a large test suite and compare wall-clock time. With 10,000 tests, expect a small but measurable reduction in filter-evaluation overhead.

Green Software Foundation Context

Hardware Efficiency: Eliminating unnecessary work makes better use of the available CPU cycles, reducing energy drawn per functional unit (test execution).

SCI: Reducing the instruction count per dotnet test run lowers the energy term of the SCI equation for CI/CD pipelines that run vstest millions of times per day across the .NET ecosystem.

Trade-offs

None. The change is purely mechanical — no algorithmic change, no new dependencies, no readability degradation. Using kvp.Key / kvp.Value from a foreach over a dictionary is idiomatic C# and already used throughout the codebase (e.g. JsoniteConvert.cs).

Test Status

• Microsoft.TestPlatform.Common.UnitTests (Filtering namespace): ✅ 108/108 passed
• Microsoft.TestPlatform.Filter.Source.UnitTests: ✅ 45/45 passed
• Build: ✅ 0 warnings, 0 errors (net462 + netstandard2.0)

Generated by Efficiency Improver · 1.6K AIC · ⌖ 25.9 AIC · ⊞ 45.5K · ◷

[Copilot]

Pull request overview

This PR optimizes FastFilter evaluation in the filter engine by reducing redundant dictionary lookups in a per-test-case hot path (FastFilter.Evaluate), aligning with the goal of lowering overhead when --filter is used.

Changes:

• Updated FastFilter.Evaluate to iterate FilterProperties as key/value pairs and reuse the value set instead of repeatedly indexing the dictionary by key.
• Simplified ValidForProperties to a single-pass invalid-property computation (but see performance note in comments re: allocation behavior).

[Jakub Jareš (nohwnd)]

Review Summary

Scope: Single file, FastFilter.cs — two method changes totalling 17 lines.

Evaluate — ✅ Correct and beneficial

The core optimization holds: iterating foreach (var kvp in FilterProperties) and capturing kvp.Value once per property eliminates 2–3 ImmutableDictionary hash-compute-and-compare lookups per property per test-case evaluation. For an ImmutableDictionary backed by a sorted tree, each [name] access was O(log n) over the number of distinct filter properties — eliminating them in a per-test-case hot path is legitimate.

The lambda closure in the multiValues path previously re-entered FilterProperties[name] for every value tested; now it closes over the already-resolved filterValues reference, which is also correct.

No correctness, thread-safety, null-safety, or IPC concerns in Evaluate.

ValidForProperties — ⚠️ Minor allocation tradeoff (not blocking)

See inline comment on line 71. The refactoring eliminates the double-scan in the failure path but introduces an empty string[] allocation in the success path where the old .All() guard had none. Given this method is called once per test source, the real-world impact is immeasurable. The PR description's framing as a pure single-pass optimization is slightly imprecise.

Checklist

• Algorithmic correctness: ✅ Functionally equivalent in all branches
• Thread safety: ✅ FilterProperties is ImmutableDictionary; values are ImmutableHashSet — both inherently thread-safe
• Null safety: ✅ kvp.Key and kvp.Value cannot be null for this dictionary's construction path
• API surface: ✅ All changed methods are internal — no public surface impact
• IPC/protocol: ✅ No wire-format changes
• Cross-TFM: ✅ #if IS_VSTEST_REPO guards handle both vstest and Roslyn-analyzer contexts correctly
• Binding redirects: ✅ No new dependencies
• Description alignment: Minor — title scopes to Evaluate while the change also includes ValidForProperties; the body covers both

🧠 Reviewed by expert-reviewing workflow · Dimensions: Algorithmic Correctness, Performance & Allocations, Null Safety & Boundary Validation, Backward Compatibility & Rollback Safety

🧠 Reviewed by Expert Code Reviewer 🧠

[Jakub Jareš (nohwnd)]

[Performance & Allocations] Minor allocation tradeoff worth noting.

The previous code:

return FilterProperties.Keys.All(name => properties.Contains(name))
    ? null
    : FilterProperties.Keys.Where(name => !properties.Contains(name)).ToArray();

had no allocation in the success path (all properties valid): .All() iterates and returns true without calling .ToArray().

The new code:

string[] invalid = FilterProperties.Keys.Where(name => !properties.Contains(name)).ToArray();
return invalid.Length == 0 ? null : invalid;

always calls .ToArray(), allocating an empty string[0] in the success path before immediately discarding it.

Since ValidForProperties is called once per test source (not per test case), the practical impact is zero. But the PR description frames this as a pure "single-pass optimization" — it is single-pass, but it trades the double-scan in the rare failure path for an allocation in the common success path. The PR's energy-efficiency claim for this method should be noted with that nuance.

Not blocking — just flagging for accuracy against the PR description.

[Jakub Jareš (nohwnd)]

[Performance] Any(lambda) still allocates a Closure object per outer-loop iteration that reaches this branch. The closure previously captured FilterProperties + name (two fields); it now captures only filterValues (one field)—smaller, but still a heap allocation on every multi-value test evaluation.

With filterValues already a local, a foreach eliminates the closure entirely:

Suggested change

	matched = values?.Any(result => result != null && filterValues.Contains(result)) == true;
	matched = false;
	if (values != null)
	{
	foreach (var result in values)
	{
	if (result != null && filterValues.Contains(result))
	{
	matched = true;
	break;
	}
	}
	}

Also removes the == true nullable-bool coercion, which is defensive but unreachable here — values is always non-null in this branch (multiValues was just proven { Length: > 0 }, and Select on a non-null source returns a non-null iterator).

[Jakub Jareš (nohwnd)]

nit: [Performance] On the happy path (all properties valid) the old All() short-circuited and returned null without any allocation. The new Where().ToArray() always runs to completion — on .NET 6+ this returns Array.Empty<string>() (no heap cost), but on net462 Buffer<T>.ToArray() emits new string[0].

ValidForProperties is cold (once per test source), so the practical impact is zero. But since the stated goal of the PR is eliminating redundant work, worth being aware that this path trades a short-circuit win for avoiding the double-pass in the unhappy case.

[Jakub Jareš (nohwnd)]

looks not worth it with the other allocations it introduces.=