perf: fix dictionary double-lookups, Collection.Contains, and LINQ allocations by Evangelink · Pull Request #15533 · microsoft/vstest

Evangelink · 2026-03-20T10:11:16Z

Summary

Fix several obvious performance issues related to data types and redundant iterations across the codebase.

Changes

1. Dictionary `ContainsKey` + indexer → `TryGetValue`

Eliminates double hash lookups in ~15 locations. Each ContainsKey + dict[key] pair performs two hash operations when TryGetValue suffices with one.

Files changed:

ParallelRunDataAggregator.cs — GetAggregatedRunStats()
TestSessionPool.cs — KillSession(), TryTakeProxy(), ReturnProxy()
ProxyTestSessionManager.cs — DequeueProxy()
InProcDataCollectionSink.cs — AddKeyValuePairToDictionary(), AddOrUpdateData()
PortableSymbolReader.cs — GetNavigationData()
FullSymbolReader.cs — GetTypeSymbol(), GetMethodSymbol()
TestSessionStartArgs.cs — GetPropertyValue<T>()
SessionEvents.cs — GetPropertyValue<T>()
SimpleJSON.cs — indexer getter/setter, Add(), Remove()

2. `Collection<InvokedDataCollector>` → `HashSet<InvokedDataCollector>`

ParallelRunDataAggregator.InvokedDataCollectors was a Collection<T> with O(n) .Contains() called inside a foreach loop (O(n²) behavior). Changed to HashSet<T> for O(1) lookups. The type already implements IEquatable<T> and GetHashCode(). Updated call sites in ParallelRunEventsHandler and ParallelDataCollectionEventsHandler to wrap with .ToList() when passing to APIs expecting Collection<T>.

3. `new[]` allocation inside LINQ predicate → static `HashSet`

In EnableBlameArgumentProcessor.cs, new[] { "CollectAlways", "DumpType" } was allocated per element inside .Where() predicates. Extracted to file-scoped static HashSet<string> fields with StringComparer.OrdinalIgnoreCase.

4. String `+=` → string interpolation

Consolidated two sequential options += calls into a single interpolated string in TestRunnerConnectionInfoExtensions.cs.

Testing

Build: ✅ build.cmd -c Release — 0 errors, 0 warnings
ParallelRunDataAggregator tests: ✅ 44 passed
TestSessionPool tests: ✅ 8 passed
InProcDataCollection tests: ✅ 30 passed
EnableBlame tests: ✅ 26 passed

Copilot

Pull request overview

Performance-focused refactor to reduce redundant dictionary lookups/iterations and cut per-call allocations, with a small concurrency-safety improvement in run stats aggregation.

Changes:

Replaced ContainsKey + indexer patterns with TryGetValue to avoid double lookups.
Switched invoked data collector tracking to a HashSet to avoid O(n²) .Contains() patterns.
Removed a per-element array allocation in LINQ predicates by using static precomputed key sets.

Reviewed changes

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

Show a summary per file

File	Description
test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/Parallel/ParallelRunDataAggregatorTests.cs	Updated assertions for `HashSet` semantics and strengthened thread-safety test by adding a concurrent reader task.
src/vstest.console/Processors/EnableBlameArgumentProcessor.cs	Replaced per-element `new[]` allocations in LINQ filters with static key sets.
src/Microsoft.TestPlatform.ObjectModel/Navigation/PortableSymbolReader.cs	Simplified nested dictionary lookups via `TryGetValue`.
src/Microsoft.TestPlatform.ObjectModel/Navigation/FullSymbolReader.cs	Eliminated redundant dictionary lookups using `TryGetValue`.
src/Microsoft.TestPlatform.ObjectModel/DataCollector/InProcDataCollector/TestSessionStartArgs.cs	Replaced double-lookup dictionary access with `TryGetValue`.
src/Microsoft.TestPlatform.ObjectModel/DataCollector/Events/SessionEvents.cs	Replaced double-lookup dictionary access with `TryGetValue`.
src/Microsoft.TestPlatform.ObjectModel/ConnectionInfo/TestRunnerConnectionInfoExtensions.cs	Reduced string concatenation operations by consolidating into one interpolated assignment.
src/Microsoft.TestPlatform.CrossPlatEngine/TestSession/TestSessionPool.cs	Reduced redundant dictionary lookups in session pool access.
src/Microsoft.TestPlatform.CrossPlatEngine/TestSession/ProxyTestSessionManager.cs	Reduced redundant dictionary lookups when resolving proxy indices.
src/Microsoft.TestPlatform.CrossPlatEngine/DataCollection/ParallelDataCollectionEventsHandler.cs	Adapted to `HashSet` by materializing to `Collection<T>` for downstream APIs.
src/Microsoft.TestPlatform.CrossPlatEngine/DataCollection/InProcDataCollectionSink.cs	Reduced redundant dictionary lookups and simplified update flow.
src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelRunEventsHandler.cs	Adapted to `HashSet` by materializing to `Collection<T>` for downstream APIs.
src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelRunDataAggregator.cs	Switched invoked collectors to `HashSet`, added locking in stats aggregation, and reduced redundant lookups.
src/Microsoft.TestPlatform.Common/Utilities/SimpleJSON.cs	Removed redundant dictionary lookups and simplified add/update/remove logic.

src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelRunDataAggregator.cs

...oft.TestPlatform.CrossPlatEngine.UnitTests/Client/Parallel/ParallelRunDataAggregatorTests.cs

src/Microsoft.TestPlatform.CrossPlatEngine/TestSession/TestSessionPool.cs

src/vstest.console/Processors/EnableBlameArgumentProcessor.cs

src/Microsoft.TestPlatform.ObjectModel/Navigation/FullSymbolReader.cs

src/Microsoft.TestPlatform.Common/Utilities/SimpleJSON.cs

src/vstest.console/Processors/EnableBlameArgumentProcessor.cs

nohwnd · 2026-03-20T17:17:56Z

brain is not braining anymore, will have a look on monday, sorry!

nohwnd · 2026-03-23T14:58:02Z

src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelRunDataAggregator.cs

        var testOutcomeMap = new Dictionary<TestOutcome, long>();
        long totalTests = 0;
-        if (_testRunStatsList.Count > 0)
+        lock (_dataUpdateSyncObject)


FYI: This was reverted before, this is called once at the end, not in parallel. Same with part of the test reverted before on the bottom.

Won't hurt most likely just sets unrealistic expectations from the api, that will confuse us if we end up replacing the method with other code.

nohwnd · 2026-03-23T14:58:30Z

...oft.TestPlatform.CrossPlatEngine.UnitTests/Client/Parallel/ParallelRunDataAggregatorTests.cs

+        // Also start a reader thread that calls GetAggregatedRunStats concurrently
+        var readerTask = Task.Run(() =>
+        {
+            barrier.SignalAndWait();
+            for (int i = 0; i < iterationsPerThread; i++)
+            {
+                // This must not throw InvalidOperationException due to collection modification
+                var runStats = aggregator.GetAggregatedRunStats();
+                Assert.IsTrue(runStats.ExecutedTests >= 0, "Executed tests count should be non-negative");
+            }
+        });
+
+        Task.WaitAll(aggregateTasks.Append(readerTask).ToArray());


This is the part I talk about above. Also the barrier.

nohwnd · 2026-03-24T07:14:54Z

looking

Address review comments from PR microsoft#15533: - Revert HashSet<InvokedDataCollector> back to Collection with Contains, as this code runs once at end, not in parallel (nohwnd feedback) - Add clarifying comment to Aggregate method about non-parallel usage - Revert barrier-based parallel test back to simpler sequential version (nohwnd feedback - was reverted before) - Revert test assertions to index-based access (order is deterministic with Collection) - Change BlameParameterNames from string[] to HashSet<string> with OrdinalIgnoreCase comparer for O(1) Contains lookups (Copilot/Youssef) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Address review comments from PR #15533: - Revert HashSet<InvokedDataCollector> back to Collection with Contains, as this code runs once at end, not in parallel (nohwnd feedback) - Add clarifying comment to Aggregate method about non-parallel usage - Revert barrier-based parallel test back to simpler sequential version (nohwnd feedback - was reverted before) - Revert test assertions to index-based access (order is deterministic with Collection) - Change BlameParameterNames from string[] to HashSet<string> with OrdinalIgnoreCase comparer for O(1) Contains lookups (Copilot/Youssef) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

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

Copilot · 2026-03-24T07:55:57Z

src/Microsoft.TestPlatform.ObjectModel/Navigation/FullSymbolReader.cs

        IDiaEnumSymbols? enumSymbols = null;
        IDiaSymbol? methodSymbol;
-        Dictionary<string, IDiaSymbol> methodSymbolsForType;
+        Dictionary<string, IDiaSymbol>? methodSymbolsForType;


Making methodSymbolsForType nullable here reduces clarity: it is expected to be non-null in the TryGetValue(...) true-branch, and (based on the structure) likely initialized in the else branch later. To keep nullability accurate and avoid downstream nullability noise, consider using a non-null local inside the if (e.g., out var methodSymbolsForType) and/or ensuring the variable is always assigned a non-null dictionary before any later use.

Suggested change

Dictionary<string, IDiaSymbol>? methodSymbolsForType;

Dictionary<string, IDiaSymbol> methodSymbolsForType;

Copilot · 2026-03-24T07:55:57Z

src/Microsoft.TestPlatform.ObjectModel/Navigation/FullSymbolReader.cs

+            if (_methodSymbols.TryGetValue(symbolName, out methodSymbolsForType))
            {
-                methodSymbolsForType = _methodSymbols[symbolName];
-                if (methodSymbolsForType.ContainsKey(methodName))
+                if (methodSymbolsForType.TryGetValue(methodName, out var cachedMethodSymbol))
                {
-                    return methodSymbolsForType[methodName];
+                    return cachedMethodSymbol;
                }
            }


Making methodSymbolsForType nullable here reduces clarity: it is expected to be non-null in the TryGetValue(...) true-branch, and (based on the structure) likely initialized in the else branch later. To keep nullability accurate and avoid downstream nullability noise, consider using a non-null local inside the if (e.g., out var methodSymbolsForType) and/or ensuring the variable is always assigned a non-null dictionary before any later use.

Copilot · 2026-03-24T07:55:57Z

src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelRunDataAggregator.cs

+    // Note: This method is called once at the end of the test run to aggregate results.
+    // It is NOT called in parallel, so thread-safety optimizations here would be misleading.


The new note contradicts the XML doc immediately above it: the doc states this must be thread-safe and expected to be called by parallel managers, while the note says it is not called in parallel. Please reconcile these (either update the XML doc to match actual usage, or remove/adjust the note) so future changes don’t rely on conflicting guidance.

Suggested change

// Note: This method is called once at the end of the test run to aggregate results.

// It is NOT called in parallel, so thread-safety optimizations here would be misleading.

note belongs to GetAggregatedRunStats

Copilot · 2026-03-24T07:55:58Z

src/Microsoft.TestPlatform.CrossPlatEngine/DataCollection/InProcDataCollectionSink.cs

+            if (CollectionData.ContainsKey(key))
            {
                EqtTrace.Warning("The data for in-proc data collector with key {0} has already been set. Will be reset with new value", key);
-                CollectionData[key] = value;
            }
+            CollectionData[key] = value;


This path still does a double-lookup (ContainsKey then indexer assignment). Since this PR targets dictionary lookup reductions, consider switching to TryAdd (warn+overwrite if it returns false) or TryGetValue to keep the warning behavior while avoiding an extra hash lookup on the common add path.

…locations - Replace ContainsKey + indexer with TryGetValue across 10+ locations (ParallelRunDataAggregator, TestSessionPool, ProxyTestSessionManager, InProcDataCollectionSink, PortableSymbolReader, FullSymbolReader, TestSessionStartArgs, SessionEvents, SimpleJSON) - Change Collection<InvokedDataCollector> to HashSet<InvokedDataCollector> in ParallelRunDataAggregator for O(1) dedup instead of O(n) - Extract static HashSets for blame parameter names instead of allocating new[] per LINQ predicate evaluation - Consolidate string += into string interpolation in TestRunnerConnectionInfoExtensions

Address review comments from PR #15533: - Revert HashSet<InvokedDataCollector> back to Collection with Contains, as this code runs once at end, not in parallel (nohwnd feedback) - Add clarifying comment to Aggregate method about non-parallel usage - Revert barrier-based parallel test back to simpler sequential version (nohwnd feedback - was reverted before) - Revert test assertions to index-based access (order is deterministic with Collection) - Change BlameParameterNames from string[] to HashSet<string> with OrdinalIgnoreCase comparer for O(1) Contains lookups (Copilot/Youssef) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

- Remove lock and non-parallel note from GetAggregatedRunStats (called once at end, not in parallel - lock was reverted before) - Keep TryGetValue and kvp iteration optimizations - Restore non-nullable Dictionary<string, IDiaSymbol> in FullSymbolReader Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Copilot · 2026-03-25T15:47:09Z

...Microsoft.TestPlatform.CrossPlatEngine/DataCollection/ParallelDataCollectionEventsHandler.cs

            _runDataAggregator.GetAggregatedException(),
            _runDataAggregator.RunContextAttachments,
-            _runDataAggregator.InvokedDataCollectors,
+            new Collection<InvokedDataCollector>(_runDataAggregator.InvokedDataCollectors.ToList()),


This materializes a new List<T> via ToList() and then wraps it in a new Collection<T>, adding allocations on the completion path. If the downstream API can be adjusted, prefer accepting IReadOnlyCollection<T> / IEnumerable<T> to avoid forcing a copy. If the signature cannot change, consider passing a one-time snapshot that is created earlier (or reusing a cached snapshot) so this call site doesn't allocate on every completion.

Suggested change

new Collection<InvokedDataCollector>(_runDataAggregator.InvokedDataCollectors.ToList()),

_runDataAggregator.InvokedDataCollectors.ToList(),

nohwnd · 2026-03-26T16:29:01Z

blocked on flakiness from main

Copilot AI review requested due to automatic review settings March 20, 2026 10:11

Evangelink enabled auto-merge (squash) March 20, 2026 10:12

Copilot AI reviewed Mar 20, 2026

View reviewed changes

Youssef1313 reviewed Mar 20, 2026

View reviewed changes

src/Microsoft.TestPlatform.Common/Utilities/SimpleJSON.cs Show resolved Hide resolved

Youssef1313 reviewed Mar 20, 2026

View reviewed changes

src/vstest.console/Processors/EnableBlameArgumentProcessor.cs Outdated Show resolved Hide resolved

nohwnd reviewed Mar 23, 2026

View reviewed changes

nohwnd mentioned this pull request Mar 24, 2026

Apply review feedback to perf dictionary double-lookups PR #15563

Closed

Copilot AI review requested due to automatic review settings March 24, 2026 07:49

Copilot AI reviewed Mar 24, 2026

View reviewed changes

Evangelink and others added 5 commits March 25, 2026 16:35

Address review comments

597b533

Move non-parallel note to GetAggregatedRunStats

b4af0f9

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings March 25, 2026 15:40

nohwnd force-pushed the dev/amauryleve/perf-fix-dictionary-double-lookups branch from bc0b5f6 to 038b6c5 Compare March 25, 2026 15:40

Copilot AI reviewed Mar 25, 2026

View reviewed changes

nohwnd approved these changes Mar 26, 2026

View reviewed changes

nohwnd disabled auto-merge March 26, 2026 16:29

nohwnd merged commit f427180 into main Mar 26, 2026
2 of 4 checks passed

nohwnd deleted the dev/amauryleve/perf-fix-dictionary-double-lookups branch March 26, 2026 16:29

This was referenced Mar 27, 2026

[main] Source code updates from microsoft/vstest dotnet/dotnet#5511

Merged

[release/11.0.1xx-preview3] Source code updates from microsoft/vstest dotnet/dotnet#5743

Open

	Dictionary<string, IDiaSymbol>? methodSymbolsForType;
	Dictionary<string, IDiaSymbol> methodSymbolsForType;

		// Note: This method is called once at the end of the test run to aggregate results.
		// It is NOT called in parallel, so thread-safety optimizations here would be misleading.

	new Collection<InvokedDataCollector>(_runDataAggregator.InvokedDataCollectors.ToList()),
	_runDataAggregator.InvokedDataCollectors.ToList(),

Conversation

Evangelink commented Mar 20, 2026

Summary

Changes

1. Dictionary ContainsKey + indexer → TryGetValue

2. Collection<InvokedDataCollector> → HashSet<InvokedDataCollector>

3. new[] allocation inside LINQ predicate → static HashSet

4. String += → string interpolation

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nohwnd commented Mar 20, 2026

Uh oh!

nohwnd Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

nohwnd Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

nohwnd commented Mar 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

nohwnd Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

nohwnd commented Mar 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

1. Dictionary `ContainsKey` + indexer → `TryGetValue`

2. `Collection<InvokedDataCollector>` → `HashSet<InvokedDataCollector>`

3. `new[]` allocation inside LINQ predicate → static `HashSet`

4. String `+=` → string interpolation