Uh oh!
There was an error while loading. Please reload this page.
[efficiency-improver] perf: add PropertyBag.FirstOrDefault(T)() to eliminate per-call array allocation - #9488
Conversation
Add a zero-allocation FirstOrDefault<TProperty>() method to PropertyBag that walks the internal linked list directly, returning the first match without materialising a TProperty[] array. Previously, callers used: Properties.OfType<T>().FirstOrDefault() PropertyBag.OfType<T>() allocates a TProperty[] even for the common single-element case, and the subsequent LINQ .FirstOrDefault() iterates it. This results in a heap allocation per call that is immediately discarded. The new method: - Returns _testNodeStateProperty directly (O(1), zero alloc) when T is TestNodeStateProperty or a subtype - Walks the linked list with an early-exit on first match for all other types — no intermediate array, no throw on duplicates VideoRecorderSessionHandler had two call sites on the hot path (once per test state-change message): update.TestNode.Properties.OfType<TestNodeStateProperty>().FirstOrDefault() update.TestNode.Properties.OfType<TimingProperty>().FirstOrDefault() Both are updated to use Properties.FirstOrDefault<T>() directly. Proxy metric: heap allocations eliminated per test update message. GSF principle: Hardware Efficiency — less GC pressure means the CPU spends fewer cycles on collection, reducing energy per functional unit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR introduces a new PropertyBag.FirstOrDefault<TProperty>() API to retrieve the first matching property without throwing when duplicates exist, and updates the video recorder extension to use it instead of LINQ-based enumeration.
Changes:
- Added
PropertyBag.FirstOrDefault<TProperty>()as a public API (tracked in PublicAPI.Unshipped files). - Implemented an allocation-free linked-list walk for first-match lookup in
PropertyBag. - Updated
VideoRecorderSessionHandlerto use the new method forTestNodeStatePropertyandTimingPropertyretrieval.
Show a summary per file
| File | Description |
|---|---|
| src/Platform/Microsoft.Testing.Platform/PublicAPI/net/PublicAPI.Unshipped.txt | Tracks newly added PropertyBag.FirstOrDefault<TProperty>() API for net target. |
| src/Platform/Microsoft.Testing.Platform/PublicAPI/PublicAPI.Unshipped.txt | Tracks newly added PropertyBag.FirstOrDefault<TProperty>() API for general PublicAPI. |
| src/Platform/Microsoft.Testing.Platform/Messages/PropertyBag.cs | Adds FirstOrDefault<TProperty>() implementation with fast-path and linked-list traversal. |
| src/Platform/Microsoft.Testing.Extensions.VideoRecorder/VideoRecorderSessionHandler.cs | Replaces LINQ OfType().FirstOrDefault() with PropertyBag.FirstOrDefault<T>(). |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Low
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Amaury Levé (Evangelink)
commented
Jun 28, 2026
🔍 Build Failure AnalysisSummary — The build fails with Root cause: Duplicate entry in |
| File | Role |
|---|---|
PublicAPI/PublicAPI.Unshipped.txt:3 | Base file — covers all target frameworks |
PublicAPI/net/PublicAPI.Unshipped.txt:2 | TFM-specific file — covers .NET only |
When MSBuild compiles the net target framework of Microsoft.Testing.Platform, the analyzer reads both files and encounters PropertyBag.FirstOrDefault<TProperty>() -> TProperty? in each — triggering RS0025 ("symbol appears more than once in the public API files"). The error fires twice because the project is built for multiple target frameworks and both builds include the .net TFM-specific file alongside the base file.
Affected errors (2)
| Code | File | Line | Message |
|---|---|---|---|
RS0025 | PublicAPI/PublicAPI.Unshipped.txt | 3 | Symbol PropertyBag.FirstOrDefault<TProperty>() -> TProperty? appears more than once |
RS0025 | PublicAPI/PublicAPI.Unshipped.txt | 3 | (same — second TFM build) |
Proposed fix
FirstOrDefault<TProperty>() contains no #if NET-guarded code in PropertyBag.cs, so it is available on all target frameworks. It belongs only in the base PublicAPI/PublicAPI.Unshipped.txt. Remove the duplicate line from the TFM-specific file:
# src/Platform/Microsoft.Testing.Platform/PublicAPI/net/PublicAPI.Unshipped.txt
#nullable enable
- Microsoft.Testing.Platform.Extensions.Messages.PropertyBag.FirstOrDefault<TProperty>() -> TProperty?The base file already has the correct entry at line 3 — no change needed there.
Build overview
Build: FAILED | Duration: 177.2s | MSBuild: 18.7.0-preview
Projects: 48 | Errors: 3 | Warnings: 0
Failed projects:
✗ Build.proj
✗ NonWindowsTests.slnf
✗ Microsoft.Testing.Extensions.CrashDump.csproj
✗ Microsoft.Testing.Extensions.TrxReport.Abstractions.csproj
✗ Microsoft.Testing.Platform.csproj ← root cause here
All MSBuild errors (2)
| Code | Project | File:Line | Message |
|---|---|---|---|
RS0025 | Microsoft.Testing.Platform | PublicAPI/PublicAPI.Unshipped.txt:3 | The symbol Microsoft.Testing.Platform.Extensions.Messages.PropertyBag.FirstOrDefault<TProperty>() -> TProperty? appears more than once in the public API files |
RS0025 | Microsoft.Testing.Platform | PublicAPI/PublicAPI.Unshipped.txt:3 | (same, second TFM evaluation) |
🤖 Generated by the Build Failure Analysis workflow using [binlog-mcp]((dev.azure.com/redacted) · commit 119b52666c9723ac2c09d679b2a2dc1a2dc31998
🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Build Failure Analysis workflow. · 179.5 AIC · ⌖ 14 AIC · ⊞ 47K · [◷]( · ◷)
Amaury Levé (Evangelink)
left a comment
There was a problem hiding this comment.
🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Build Failure Analysis workflow. · 179.5 AIC · ⌖ 14 AIC · ⊞ 47K · ◷
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Amaury Levé (Evangelink)
left a comment
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
| # | Dimension | Verdict |
|---|---|---|
| 13 | Test Completeness & Coverage | 🟠 1 MAJOR |
✅ 21/22 dimensions clean.
- Test Completeness —
PropertyBag.FirstOrDefault<TProperty>()ships with no unit tests; all otherPropertyBagmethods are thoroughly tested inPropertyBagTests.cs. See the inline comment for the exact scenarios needed.
Overall assessment: The implementation is correct, well-structured, and consistent with the existing SingleOrDefault<T> and Any<T> patterns. The algorithm handles all edge cases properly (TestNodeStateProperty fast path, subtype guard via IsAssignableFrom, linked-list walk with early return). The PublicAPI.Unshipped.txt declaration is present, and the VideoRecorder call-site changes are semantically equivalent to the replaced LINQ expressions. The only gap is test coverage for the new public method.
Uh oh!
There was an error while loading. Please reload this page.
# Conflicts: # src/Platform/Microsoft.Testing.Platform/PublicAPI/PublicAPI.Unshipped.txt
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Amaury Levé (Evangelink)
commented
Jun 29, 2026
🧪 Test quality grade — PR #9488
This advisory comment was generated automatically. Grades are heuristic
|
Uh oh!
There was an error while loading. Please reload this page.
Goal
Add a zero-allocation
FirstOrDefault<TProperty>()method toPropertyBagand updateVideoRecorderSessionHandlerto use it, eliminating two heap allocations per test state-change message.Focus Area
Code-Level Efficiency — unnecessary object creation on a hot path.
Problem
PropertyBagexposesOfType<T>()which materialises results into aTProperty[]array. Callers that only want the first match were writing:This allocates a
TProperty[]for every call, even though only the first element is ever used. The array is immediately discarded.VideoRecorderSessionHandlerhas two such call sites, both on the per-test-update hot path.Approach
Add
PropertyBag.FirstOrDefault<TProperty>()modelled after the existingSingleOrDefault<TProperty>():TPropertyisTestNodeStateProperty(or a subtype), check_testNodeStatePropertydirectly — O(1), zero allocation.Property?linked list with an early-exit on the first match — no array, no throw on duplicates.VideoRecorderSessionHandlernow callsProperties.FirstOrDefault<T>()directly at both call sites.Energy Efficiency Evidence
Proxy metric: Heap allocations eliminated per test update message.
VideoRecorderSessionHandlerL128TestNodeStateProperty[]allocated + LINQ enumeration_testNodeStateProperty), O(1), 0 allocVideoRecorderSessionHandlerL480TimingProperty[]allocated + LINQ enumerationEliminating heap allocations directly reduces GC pressure. Less GC means fewer stop-the-world pauses and fewer CPU cycles spent on collection — translating to lower energy per functional unit (test run).
Limitation: We do not have direct energy measurements. The reasoning is:
Green Software Foundation Context
Hardware Efficiency: Making better use of the hardware by avoiding unnecessary memory round-trips. Every array the GC does not have to scan, trace, and collect is CPU time reclaimed for useful work, reducing the energy per test execution.
Trade-offs
None: the new method is semantically equivalent to the previous pattern for the single-match case (which is the only realistic scenario given
PropertyBag's enforcement of uniqueness forTestNodeStateProperty). The only behavioural difference is that this method does not throw when multiple properties of the same type are present — which is exactly the defensive behaviour the code comments already called for.Reproducibility
Test Status
CI will validate. Changes are self-contained: new public API + two call-site updates.
Add this agentic workflows to your repo
To install this agentic workflow, run