[efficiency-improver] perf: eliminate redundant ConcurrentDictionary updates in DiscoveryDataAggregator hot path

[nohwnd]

Goal and Rationale

Eliminate O(N) redundant ConcurrentDictionary.AddOrUpdate calls and single-element array allocations on the discovery hot path in DiscoveryDataAggregator.MarkSourcesBasedOnDiscoveredTestCases.

Focus area: Code-Level Efficiency

Problem

The original inner loop called:

if (previousSource is null || previousSource == currentSource)
{
    MarkSourcesWithStatus(new[] { currentSource }, DiscoveryStatus.PartiallyDiscovered);
}

This fires for every test case when consecutive tests share a source assembly (the common case). For a project with N tests in one assembly it means:

• N new[] { currentSource } array allocations (~32 bytes each on 64-bit)
• N ConcurrentDictionary.AddOrUpdate calls with lock acquisition
• N update-factory invocations that return PartiallyDiscovered unchanged

After the first call, the status is already PartiallyDiscovered; all subsequent calls with the same source are pure no-ops.

Approach

1. Skip redundant updates: change the condition from previousSource is null || previousSource == currentSource to just previousSource is null. When the source matches the previous one, the status is already set — skip entirely.

2. Extract MarkSourceWithStatus(string?, DiscoveryStatus): private single-source helper that calls AddOrUpdate directly, eliminating the new[] array, the IEnumerable overhead, and the null guard on sources for the three remaining single-source call sites in the loop.

3. Refactor MarkSourcesWithStatus to delegate to the new helper, removing the duplicated AddOrUpdate logic.

Energy Efficiency Evidence

Proxy metric: Memory allocation (GC pressure) and CPU cycles (lock contention) on the discovery path

Metric	Before	After
Array allocations	O(N) per source — one string[1] per test case	O(1) per source — zero after first encounter
ConcurrentDictionary.AddOrUpdate calls	O(N) per source	O(1) per source

For a solution with 10,000 tests in a single assembly, this eliminates ~10,000 heap allocations and ~10,000 lock acquisitions from the discovery path per dotnet test run.

Green Software Foundation context:

• Hardware Efficiency: fewer lock acquisitions → less bus traffic and cache-line contention; fewer GC pauses → more CPU cycles spent on useful work
• SCI (Software Carbon Intensity): reduced CPU energy per dotnet test invocation, proportional to test count

Trade-offs

None. The semantics are identical: AddOrUpdate with a value that already matches the stored value is a pure no-op. All 45 DiscoveryDataAggregator unit tests (including source-tracking regression tests) pass without modification.

Reproducibility

# Run the specific test suite:
.dotnet/dotnet run --project test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/ \
  -c Release -f net11.0 -- --filter "DiscoveryDataAggregator"

# Verify no new[] single-element calls remain in the loop:
grep -n "new\[\]" src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/DiscoveryDataAggregator.cs

Test Status

• ✅ Build: 0 errors (4 pre-existing IL-trimming warnings, unrelated)
• ✅ DiscoveryDataAggregator tests: 45/45 passed, 0 failures
• i️ 8 pre-existing failures in CrossPlatEngine suite (Windows-path tests, fail on Linux on both main and this branch)

Generated by Efficiency Improver · 1.3K AIC · ⌖ 25.6 AIC · ⊞ 45.6K · ◷

[Copilot]

Pull request overview

This PR optimizes the CrossPlatEngine discovery hot path by avoiding redundant ConcurrentDictionary.AddOrUpdate calls and single-element array allocations while tracking per-source discovery status in DiscoveryDataAggregator.

Changes:

• Refactors source-status updates by extracting a single-source helper (MarkSourceWithStatus) and having MarkSourcesWithStatus delegate to it.
• Updates MarkSourcesBasedOnDiscoveredTestCases to skip redundant “same source” updates and call the single-source helper directly to avoid new[] { ... } allocations.

[nohwnd]

🧠 Expert Review — DiscoveryDataAggregator hot-path optimization

Dimensions activated: Parallel Execution & Scheduling Safety · Algorithmic Correctness · Performance & Allocations · Error Reporting & Diagnostic Clarity

---

Algorithmic Correctness ✅

The condition change from previousSource is null || previousSource == currentSource to previousSource is null is semantically equivalent for all reachable states.

The key invariant being relied on is: the value returned by MarkSourcesBasedOnDiscoveredTestCases is always a source already recorded as PartiallyDiscovered in the dictionary. This holds because:

• Branch 1 (previousSource is null): adds currentSource as PartiallyDiscovered before setting previousSource.
• Branch 2 (currentSource != previousSource): marks currentSource as PartiallyDiscovered before it becomes the new previousSource.
• Skip case (currentSource == previousSource, non-null): the source was already PartiallyDiscovered from a prior iteration or batch, so no update needed.

ConcurrentDictionary has no Remove, so a recorded status can't be erased between batches — the invariant is stable across calls. ✅

Parallel Execution & Scheduling Safety ✅

Multiple proxies share this aggregator but each maintains its own previousSource locally. All shared state is ConcurrentDictionary.AddOrUpdate, which is thread-safe. The optimization skips AddOrUpdate only when the local previousSource equals currentSource, which is purely local state. No new race window is introduced. ✅

The non-volatile read of _isMessageSent is consistent with the pre-existing pattern and was not changed by this PR.

Performance & Allocations ✅

The claimed savings are real. For N tests in a single assembly, the old path allocated N string[1] arrays and called AddOrUpdate N times (all no-ops after the first). The new path does one call on first encounter then zero for the rest — O(1) per source rather than O(N).

The new[] allocation and the IEnumerable<string?> dispatch overhead in the former MarkSourcesWithStatus call are also eliminated at the three loop call sites.

Error Reporting & Diagnostic Clarity ⚠️ (minor, non-blocking)

The trace messages inside MarkSourceWithStatus still reference DiscoveryDataAggregator.MarkSourcesWithStatus: as the method identifier. With the refactoring, this log prefix now points to the public entry point rather than the actual call site. This is a mild diagnostic inconsistency if someone is triaging from traces alone — the message origin will appear to be MarkSourcesWithStatus when the actual frame is the private helper. Not a correctness issue; noting it for completeness.

PR Description Alignment ✅

Title and description accurately describe all three changes in the diff (condition narrowing, private helper extraction, MarkSourcesWithStatus delegation). The before/after table, invariant explanation, and complexity claims all match the code.

---

No blocking issues found. The optimization is correct, the refactoring is clean, and test coverage (45 existing unit tests) validates the unchanged observable semantics.

---

🧠 Reviewed by expert-reviewer

🧠 Reviewed by Expert Code Reviewer 🧠