[Efficiency Improver] perf: eliminate yield-iterator state machine in PropertyBag.OfType(T)() - #7917

Merged
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
efficiency/propertybag-oftype-no-state-machine-8f9c5bd2284aea80
Apr 29, 2026
Merged

[Efficiency Improver] perf: eliminate yield-iterator state machine in PropertyBag.OfType(T)()#7917
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
efficiency/propertybag-oftype-no-state-machine-8f9c5bd2284aea80

Conversation

@Evangelink

Copy link
Copy Markdown
Member

Replace the yield-based iterator from Property.OfType() with a direct linked-list walk inside PropertyBag.OfType().

Before: _property.OfType() (yield return) allocates one state- machine heap object per call, even when no matching property is found.

After: direct while-loop walk; no state-machine allocated; returns [] for the common case (no matching property) with zero heap allocations beyond the empty array constant.

Fixes#7914

Replace the yield-based iterator from Property.OfType<TProperty>() with
a direct linked-list walk inside PropertyBag.OfType<TProperty>().
Before: _property.OfType<TProperty>() (yield return) allocates one state-
machine heap object per call, even when no matching property is found.
After: direct while-loop walk; no state-machine allocated; returns [] for
the common case (no matching property) with zero heap allocations beyond
the empty array constant.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings April 29, 2026 04:49

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR optimizes a hot-path in Microsoft.Testing.Platform by removing the yield return-based iterator allocation from PropertyBag.OfType<TProperty>() and replacing it with a direct linked-list traversal.

Changes:

  • Replace [.. _property.OfType<TProperty>()] (yield iterator) with a manual while loop over the Property linked list.
  • Preserve existing fast-path behavior for TestNodeStateProperty lookups and the empty/null property-list cases.
Show a summary per file
FileDescription
src/Platform/Microsoft.Testing.Platform/Messages/PropertyBag.csReworks OfType<TProperty>() to avoid iterator state-machine allocations by directly walking the linked list.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 0

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-04-29
Repository: microsoft/testfx

Key Findings

The PR replaces the yield-based OfType<TProperty>() iterator with a direct linked-list walk, introducing three distinct return paths:

  1. _testNodeStateProperty is TProperty[testNodeStateProperty] (unchanged)
  2. New combined early-return: typeof(TestNodeStateProperty).IsAssignableFrom(typeof(TProperty)) || _property is null[]
  3. New while-loop: !foundAny[], single match → [first!], multiple → [.. overflow]

Paths 2 and 3 (both new) have gaps in PropertyBagTests:

GapMissing pathRisk
_property is null early-returnOfType<DummyProperty>() on a bag with only TestNodeStatePropertySilent regression if early-return is dropped
Single-item fast pathOfType<DummyProperty>() with exactly 1 match[first!] returns wrong result/throws undetected
No-match path in walkOfType<DummyProperty2>() on a non-empty bagManual loop doesn't cover LINQ's implicitly empty-on-no-match

The production logic itself looks correct — the overflow initialization overflow ??= [first!] correctly seeds the list with the first match before adding the second. No functional bugs detected.

Recommendations

  1. Add an assertion Assert.IsEmpty(property.OfType<DummyProperty>()) for a bag containing only a TestNodeStateProperty.
  2. Add OfType<DummyProperty>() with exactly 1 DummyProperty in the bag.
  3. Add Assert.IsEmpty(property.OfType<DummyProperty2>()) to the existing OfType_Should_Return_CorrectObject test (one line, costs nothing).

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

return !foundAny
? []
: _property is null ? [] : [.. _property.OfType<TProperty>()];
: overflow is not null ? [.. overflow] : [first!];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Coverage] The single-item fast path ([first!] when overflow is null) is not exercised by any existing test. OfType_Should_Return_CorrectObject only tests with 2 DummyProperty instances, which goes through the overflow branch. The [first!] path needs its own case.

Impact: A regression where a single-result bag returns [] or throws would not be caught.

Suggestion: Extend OfType_Should_Return_CorrectObject or add a dedicated test:

PropertyBagbag=new();DummyPropertysingle=new();bag.Add(single);bag.Add(PassedTestNodeStateProperty.CachedInstance);TProperty[]result=bag.OfType<DummyProperty>();Assert.HasCount(1,result);Assert.AreSame(single,result[0]);


// We don't want to allocate an array if we know that we're looking for a TestNodeStateProperty
return typeof(TestNodeStateProperty).IsAssignableFrom(typeof(TProperty))
if (typeof(TestNodeStateProperty).IsAssignableFrom(typeof(TProperty)) || _property is null)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Coverage] The || _property is null branch is new (the original ternary had _property is null ? [] as its own arm). There's currently no test for calling OfType<SomeDummyProperty>() on a PropertyBag that contains only a TestNodeStateProperty (i.e., _testNodeStateProperty is set but _property is null).

Impact: The early-return path added here goes unverified. If a future refactor accidentally removes it, a silent linear walk over a null reference would produce incorrect results.

Suggestion: Add a test case to PropertyBagTests:

[TestMethod]publicvoidOfType_WithOnlyTestNodeStateProperty_ReturnsEmpty(){PropertyBagproperty=new();property.Add(PassedTestNodeStateProperty.CachedInstance);// _property is null; _testNodeStateProperty is set — exercises the new early-return pathAssert.IsEmpty(property.OfType<DummyProperty>());}

current = current.Next;
}

return !foundAny

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Coverage] The !foundAny ? [] return at the end of the while-loop (no matching property found in a non-empty bag) is not tested. DummyProperty2 is used in Single / SingleOrDefault tests, but OfType<DummyProperty2>() is never called on a bag that has other properties.

Impact: The new manual traversal diverges from the old LINQ-based _property.OfType<TProperty>(). A bug that incorrectly returns null or throws instead of [] when no match is found would go undetected.

Suggestion: Add one assertion to the existing OfType_Should_Return_CorrectObject test:

// No DummyProperty2 in the bag — exercises the "no match found" path in the while-loopAssert.IsEmpty(property.OfType<DummyProperty2>());

@Evangelink

Copy link
Copy Markdown
MemberAuthor

Copilot address review comments

CopilotAI review requested due to automatic review settings April 29, 2026 05:16
@Evangelink
Amaury Levé (Evangelink) removed the request for review from CopilotApril 29, 2026 05:16

CopilotAI commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

Copilot address review comments

Added the three missing test cases in commits 911b697 and 813c705:

  • OfType_Should_Return_CorrectObject — extended with Assert.IsEmpty(property.OfType<DummyProperty2>()) to cover the "no match found" path
  • OfType_WithSingleMatch_ReturnsSingleItemArray — new test covering the single-item fast path ([first!] when overflow is null)
  • OfType_WithOnlyTestNodeStateProperty_ReturnsEmpty — new test covering the _property is null early return when only a TestNodeStateProperty is present

@Evangelink
Amaury Levé (Evangelink) merged commit e309705 into mainApr 29, 2026
24 checks passed
@Evangelink
Amaury Levé (Evangelink) deleted the efficiency/propertybag-oftype-no-state-machine-8f9c5bd2284aea80 branch April 29, 2026 09:14
Amaury Levé (Evangelink) added a commit that referenced this pull request May 12, 2026
… PropertyBag.OfType(T)() (#7917)
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Efficiency Improver] perf: eliminate yield-iterator state machine in PropertyBag.OfType(T)()

3 participants

@Evangelink
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

[Efficiency Improver] perf: eliminate yield-iterator state machine in PropertyBag.OfType(T)() - #7917

Merged
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
efficiency/propertybag-oftype-no-state-machine-8f9c5bd2284aea80
Apr 29, 2026
Merged

[Efficiency Improver] perf: eliminate yield-iterator state machine in PropertyBag.OfType(T)()#7917
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
efficiency/propertybag-oftype-no-state-machine-8f9c5bd2284aea80

Conversation

@Evangelink

Copy link
Copy Markdown
Member

Replace the yield-based iterator from Property.OfType() with a direct linked-list walk inside PropertyBag.OfType().

Before: _property.OfType() (yield return) allocates one state- machine heap object per call, even when no matching property is found.

After: direct while-loop walk; no state-machine allocated; returns [] for the common case (no matching property) with zero heap allocations beyond the empty array constant.

Fixes#7914

Replace the yield-based iterator from Property.OfType<TProperty>() with
a direct linked-list walk inside PropertyBag.OfType<TProperty>().
Before: _property.OfType<TProperty>() (yield return) allocates one state-
machine heap object per call, even when no matching property is found.
After: direct while-loop walk; no state-machine allocated; returns [] for
the common case (no matching property) with zero heap allocations beyond
the empty array constant.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings April 29, 2026 04:49

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR optimizes a hot-path in Microsoft.Testing.Platform by removing the yield return-based iterator allocation from PropertyBag.OfType<TProperty>() and replacing it with a direct linked-list traversal.

Changes:

  • Replace [.. _property.OfType<TProperty>()] (yield iterator) with a manual while loop over the Property linked list.
  • Preserve existing fast-path behavior for TestNodeStateProperty lookups and the empty/null property-list cases.
Show a summary per file
FileDescription
src/Platform/Microsoft.Testing.Platform/Messages/PropertyBag.csReworks OfType<TProperty>() to avoid iterator state-machine allocations by directly walking the linked list.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 0

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-04-29
Repository: microsoft/testfx

Key Findings

The PR replaces the yield-based OfType<TProperty>() iterator with a direct linked-list walk, introducing three distinct return paths:

  1. _testNodeStateProperty is TProperty[testNodeStateProperty] (unchanged)
  2. New combined early-return: typeof(TestNodeStateProperty).IsAssignableFrom(typeof(TProperty)) || _property is null[]
  3. New while-loop: !foundAny[], single match → [first!], multiple → [.. overflow]

Paths 2 and 3 (both new) have gaps in PropertyBagTests:

GapMissing pathRisk
_property is null early-returnOfType<DummyProperty>() on a bag with only TestNodeStatePropertySilent regression if early-return is dropped
Single-item fast pathOfType<DummyProperty>() with exactly 1 match[first!] returns wrong result/throws undetected
No-match path in walkOfType<DummyProperty2>() on a non-empty bagManual loop doesn't cover LINQ's implicitly empty-on-no-match

The production logic itself looks correct — the overflow initialization overflow ??= [first!] correctly seeds the list with the first match before adding the second. No functional bugs detected.

Recommendations

  1. Add an assertion Assert.IsEmpty(property.OfType<DummyProperty>()) for a bag containing only a TestNodeStateProperty.
  2. Add OfType<DummyProperty>() with exactly 1 DummyProperty in the bag.
  3. Add Assert.IsEmpty(property.OfType<DummyProperty2>()) to the existing OfType_Should_Return_CorrectObject test (one line, costs nothing).

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

return !foundAny
? []
: _property is null ? [] : [.. _property.OfType<TProperty>()];
: overflow is not null ? [.. overflow] : [first!];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Coverage] The single-item fast path ([first!] when overflow is null) is not exercised by any existing test. OfType_Should_Return_CorrectObject only tests with 2 DummyProperty instances, which goes through the overflow branch. The [first!] path needs its own case.

Impact: A regression where a single-result bag returns [] or throws would not be caught.

Suggestion: Extend OfType_Should_Return_CorrectObject or add a dedicated test:

PropertyBagbag=new();DummyPropertysingle=new();bag.Add(single);bag.Add(PassedTestNodeStateProperty.CachedInstance);TProperty[]result=bag.OfType<DummyProperty>();Assert.HasCount(1,result);Assert.AreSame(single,result[0]);


// We don't want to allocate an array if we know that we're looking for a TestNodeStateProperty
return typeof(TestNodeStateProperty).IsAssignableFrom(typeof(TProperty))
if (typeof(TestNodeStateProperty).IsAssignableFrom(typeof(TProperty)) || _property is null)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Coverage] The || _property is null branch is new (the original ternary had _property is null ? [] as its own arm). There's currently no test for calling OfType<SomeDummyProperty>() on a PropertyBag that contains only a TestNodeStateProperty (i.e., _testNodeStateProperty is set but _property is null).

Impact: The early-return path added here goes unverified. If a future refactor accidentally removes it, a silent linear walk over a null reference would produce incorrect results.

Suggestion: Add a test case to PropertyBagTests:

[TestMethod]publicvoidOfType_WithOnlyTestNodeStateProperty_ReturnsEmpty(){PropertyBagproperty=new();property.Add(PassedTestNodeStateProperty.CachedInstance);// _property is null; _testNodeStateProperty is set — exercises the new early-return pathAssert.IsEmpty(property.OfType<DummyProperty>());}

current = current.Next;
}

return !foundAny

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Coverage] The !foundAny ? [] return at the end of the while-loop (no matching property found in a non-empty bag) is not tested. DummyProperty2 is used in Single / SingleOrDefault tests, but OfType<DummyProperty2>() is never called on a bag that has other properties.

Impact: The new manual traversal diverges from the old LINQ-based _property.OfType<TProperty>(). A bug that incorrectly returns null or throws instead of [] when no match is found would go undetected.

Suggestion: Add one assertion to the existing OfType_Should_Return_CorrectObject test:

// No DummyProperty2 in the bag — exercises the "no match found" path in the while-loopAssert.IsEmpty(property.OfType<DummyProperty2>());

@Evangelink

Copy link
Copy Markdown
MemberAuthor

Copilot address review comments

CopilotAI review requested due to automatic review settings April 29, 2026 05:16
@Evangelink
Amaury Levé (Evangelink) removed the request for review from CopilotApril 29, 2026 05:16

CopilotAI commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

Copilot address review comments

Added the three missing test cases in commits 911b697 and 813c705:

  • OfType_Should_Return_CorrectObject — extended with Assert.IsEmpty(property.OfType<DummyProperty2>()) to cover the "no match found" path
  • OfType_WithSingleMatch_ReturnsSingleItemArray — new test covering the single-item fast path ([first!] when overflow is null)
  • OfType_WithOnlyTestNodeStateProperty_ReturnsEmpty — new test covering the _property is null early return when only a TestNodeStateProperty is present

@Evangelink
Amaury Levé (Evangelink) merged commit e309705 into mainApr 29, 2026
24 checks passed
@Evangelink
Amaury Levé (Evangelink) deleted the efficiency/propertybag-oftype-no-state-machine-8f9c5bd2284aea80 branch April 29, 2026 09:14
Amaury Levé (Evangelink) added a commit that referenced this pull request May 12, 2026
… PropertyBag.OfType(T)() (#7917)
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Efficiency Improver] perf: eliminate yield-iterator state machine in PropertyBag.OfType(T)()

3 participants

@Evangelink
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[Efficiency Improver] perf: eliminate yield-iterator state machine in PropertyBag.OfType(T)() - #7917

Merged
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
efficiency/propertybag-oftype-no-state-machine-8f9c5bd2284aea80
Apr 29, 2026
Merged

[Efficiency Improver] perf: eliminate yield-iterator state machine in PropertyBag.OfType(T)()#7917
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
efficiency/propertybag-oftype-no-state-machine-8f9c5bd2284aea80

Conversation

@Evangelink

Copy link
Copy Markdown
Member

Replace the yield-based iterator from Property.OfType() with a direct linked-list walk inside PropertyBag.OfType().

Before: _property.OfType() (yield return) allocates one state- machine heap object per call, even when no matching property is found.

After: direct while-loop walk; no state-machine allocated; returns [] for the common case (no matching property) with zero heap allocations beyond the empty array constant.

Fixes#7914

Replace the yield-based iterator from Property.OfType<TProperty>() with
a direct linked-list walk inside PropertyBag.OfType<TProperty>().
Before: _property.OfType<TProperty>() (yield return) allocates one state-
machine heap object per call, even when no matching property is found.
After: direct while-loop walk; no state-machine allocated; returns [] for
the common case (no matching property) with zero heap allocations beyond
the empty array constant.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings April 29, 2026 04:49

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR optimizes a hot-path in Microsoft.Testing.Platform by removing the yield return-based iterator allocation from PropertyBag.OfType<TProperty>() and replacing it with a direct linked-list traversal.

Changes:

  • Replace [.. _property.OfType<TProperty>()] (yield iterator) with a manual while loop over the Property linked list.
  • Preserve existing fast-path behavior for TestNodeStateProperty lookups and the empty/null property-list cases.
Show a summary per file
FileDescription
src/Platform/Microsoft.Testing.Platform/Messages/PropertyBag.csReworks OfType<TProperty>() to avoid iterator state-machine allocations by directly walking the linked list.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 0

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-04-29
Repository: microsoft/testfx

Key Findings

The PR replaces the yield-based OfType<TProperty>() iterator with a direct linked-list walk, introducing three distinct return paths:

  1. _testNodeStateProperty is TProperty[testNodeStateProperty] (unchanged)
  2. New combined early-return: typeof(TestNodeStateProperty).IsAssignableFrom(typeof(TProperty)) || _property is null[]
  3. New while-loop: !foundAny[], single match → [first!], multiple → [.. overflow]

Paths 2 and 3 (both new) have gaps in PropertyBagTests:

GapMissing pathRisk
_property is null early-returnOfType<DummyProperty>() on a bag with only TestNodeStatePropertySilent regression if early-return is dropped
Single-item fast pathOfType<DummyProperty>() with exactly 1 match[first!] returns wrong result/throws undetected
No-match path in walkOfType<DummyProperty2>() on a non-empty bagManual loop doesn't cover LINQ's implicitly empty-on-no-match

The production logic itself looks correct — the overflow initialization overflow ??= [first!] correctly seeds the list with the first match before adding the second. No functional bugs detected.

Recommendations

  1. Add an assertion Assert.IsEmpty(property.OfType<DummyProperty>()) for a bag containing only a TestNodeStateProperty.
  2. Add OfType<DummyProperty>() with exactly 1 DummyProperty in the bag.
  3. Add Assert.IsEmpty(property.OfType<DummyProperty2>()) to the existing OfType_Should_Return_CorrectObject test (one line, costs nothing).

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

return !foundAny
? []
: _property is null ? [] : [.. _property.OfType<TProperty>()];
: overflow is not null ? [.. overflow] : [first!];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Coverage] The single-item fast path ([first!] when overflow is null) is not exercised by any existing test. OfType_Should_Return_CorrectObject only tests with 2 DummyProperty instances, which goes through the overflow branch. The [first!] path needs its own case.

Impact: A regression where a single-result bag returns [] or throws would not be caught.

Suggestion: Extend OfType_Should_Return_CorrectObject or add a dedicated test:

PropertyBagbag=new();DummyPropertysingle=new();bag.Add(single);bag.Add(PassedTestNodeStateProperty.CachedInstance);TProperty[]result=bag.OfType<DummyProperty>();Assert.HasCount(1,result);Assert.AreSame(single,result[0]);


// We don't want to allocate an array if we know that we're looking for a TestNodeStateProperty
return typeof(TestNodeStateProperty).IsAssignableFrom(typeof(TProperty))
if (typeof(TestNodeStateProperty).IsAssignableFrom(typeof(TProperty)) || _property is null)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Coverage] The || _property is null branch is new (the original ternary had _property is null ? [] as its own arm). There's currently no test for calling OfType<SomeDummyProperty>() on a PropertyBag that contains only a TestNodeStateProperty (i.e., _testNodeStateProperty is set but _property is null).

Impact: The early-return path added here goes unverified. If a future refactor accidentally removes it, a silent linear walk over a null reference would produce incorrect results.

Suggestion: Add a test case to PropertyBagTests:

[TestMethod]publicvoidOfType_WithOnlyTestNodeStateProperty_ReturnsEmpty(){PropertyBagproperty=new();property.Add(PassedTestNodeStateProperty.CachedInstance);// _property is null; _testNodeStateProperty is set — exercises the new early-return pathAssert.IsEmpty(property.OfType<DummyProperty>());}

current = current.Next;
}

return !foundAny

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Coverage] The !foundAny ? [] return at the end of the while-loop (no matching property found in a non-empty bag) is not tested. DummyProperty2 is used in Single / SingleOrDefault tests, but OfType<DummyProperty2>() is never called on a bag that has other properties.

Impact: The new manual traversal diverges from the old LINQ-based _property.OfType<TProperty>(). A bug that incorrectly returns null or throws instead of [] when no match is found would go undetected.

Suggestion: Add one assertion to the existing OfType_Should_Return_CorrectObject test:

// No DummyProperty2 in the bag — exercises the "no match found" path in the while-loopAssert.IsEmpty(property.OfType<DummyProperty2>());

@Evangelink

Copy link
Copy Markdown
MemberAuthor

Copilot address review comments

CopilotAI review requested due to automatic review settings April 29, 2026 05:16
@Evangelink
Amaury Levé (Evangelink) removed the request for review from CopilotApril 29, 2026 05:16

CopilotAI commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

Copilot address review comments

Added the three missing test cases in commits 911b697 and 813c705:

  • OfType_Should_Return_CorrectObject — extended with Assert.IsEmpty(property.OfType<DummyProperty2>()) to cover the "no match found" path
  • OfType_WithSingleMatch_ReturnsSingleItemArray — new test covering the single-item fast path ([first!] when overflow is null)
  • OfType_WithOnlyTestNodeStateProperty_ReturnsEmpty — new test covering the _property is null early return when only a TestNodeStateProperty is present

@Evangelink
Amaury Levé (Evangelink) merged commit e309705 into mainApr 29, 2026
24 checks passed
@Evangelink
Amaury Levé (Evangelink) deleted the efficiency/propertybag-oftype-no-state-machine-8f9c5bd2284aea80 branch April 29, 2026 09:14
Amaury Levé (Evangelink) added a commit that referenced this pull request May 12, 2026
… PropertyBag.OfType(T)() (#7917)
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Efficiency Improver] perf: eliminate yield-iterator state machine in PropertyBag.OfType(T)()

3 participants

@Evangelink
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[Efficiency Improver] perf: eliminate yield-iterator state machine in PropertyBag.OfType(T)() - #7917

Merged
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
efficiency/propertybag-oftype-no-state-machine-8f9c5bd2284aea80
Apr 29, 2026
Merged

[Efficiency Improver] perf: eliminate yield-iterator state machine in PropertyBag.OfType(T)()#7917
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
efficiency/propertybag-oftype-no-state-machine-8f9c5bd2284aea80

Conversation

@Evangelink

Copy link
Copy Markdown
Member

Replace the yield-based iterator from Property.OfType() with a direct linked-list walk inside PropertyBag.OfType().

Before: _property.OfType() (yield return) allocates one state- machine heap object per call, even when no matching property is found.

After: direct while-loop walk; no state-machine allocated; returns [] for the common case (no matching property) with zero heap allocations beyond the empty array constant.

Fixes#7914

Replace the yield-based iterator from Property.OfType<TProperty>() with
a direct linked-list walk inside PropertyBag.OfType<TProperty>().
Before: _property.OfType<TProperty>() (yield return) allocates one state-
machine heap object per call, even when no matching property is found.
After: direct while-loop walk; no state-machine allocated; returns [] for
the common case (no matching property) with zero heap allocations beyond
the empty array constant.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings April 29, 2026 04:49

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR optimizes a hot-path in Microsoft.Testing.Platform by removing the yield return-based iterator allocation from PropertyBag.OfType<TProperty>() and replacing it with a direct linked-list traversal.

Changes:

  • Replace [.. _property.OfType<TProperty>()] (yield iterator) with a manual while loop over the Property linked list.
  • Preserve existing fast-path behavior for TestNodeStateProperty lookups and the empty/null property-list cases.
Show a summary per file
FileDescription
src/Platform/Microsoft.Testing.Platform/Messages/PropertyBag.csReworks OfType<TProperty>() to avoid iterator state-machine allocations by directly walking the linked list.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 0

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-04-29
Repository: microsoft/testfx

Key Findings

The PR replaces the yield-based OfType<TProperty>() iterator with a direct linked-list walk, introducing three distinct return paths:

  1. _testNodeStateProperty is TProperty[testNodeStateProperty] (unchanged)
  2. New combined early-return: typeof(TestNodeStateProperty).IsAssignableFrom(typeof(TProperty)) || _property is null[]
  3. New while-loop: !foundAny[], single match → [first!], multiple → [.. overflow]

Paths 2 and 3 (both new) have gaps in PropertyBagTests:

GapMissing pathRisk
_property is null early-returnOfType<DummyProperty>() on a bag with only TestNodeStatePropertySilent regression if early-return is dropped
Single-item fast pathOfType<DummyProperty>() with exactly 1 match[first!] returns wrong result/throws undetected
No-match path in walkOfType<DummyProperty2>() on a non-empty bagManual loop doesn't cover LINQ's implicitly empty-on-no-match

The production logic itself looks correct — the overflow initialization overflow ??= [first!] correctly seeds the list with the first match before adding the second. No functional bugs detected.

Recommendations

  1. Add an assertion Assert.IsEmpty(property.OfType<DummyProperty>()) for a bag containing only a TestNodeStateProperty.
  2. Add OfType<DummyProperty>() with exactly 1 DummyProperty in the bag.
  3. Add Assert.IsEmpty(property.OfType<DummyProperty2>()) to the existing OfType_Should_Return_CorrectObject test (one line, costs nothing).

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

return !foundAny
? []
: _property is null ? [] : [.. _property.OfType<TProperty>()];
: overflow is not null ? [.. overflow] : [first!];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Coverage] The single-item fast path ([first!] when overflow is null) is not exercised by any existing test. OfType_Should_Return_CorrectObject only tests with 2 DummyProperty instances, which goes through the overflow branch. The [first!] path needs its own case.

Impact: A regression where a single-result bag returns [] or throws would not be caught.

Suggestion: Extend OfType_Should_Return_CorrectObject or add a dedicated test:

PropertyBagbag=new();DummyPropertysingle=new();bag.Add(single);bag.Add(PassedTestNodeStateProperty.CachedInstance);TProperty[]result=bag.OfType<DummyProperty>();Assert.HasCount(1,result);Assert.AreSame(single,result[0]);


// We don't want to allocate an array if we know that we're looking for a TestNodeStateProperty
return typeof(TestNodeStateProperty).IsAssignableFrom(typeof(TProperty))
if (typeof(TestNodeStateProperty).IsAssignableFrom(typeof(TProperty)) || _property is null)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Coverage] The || _property is null branch is new (the original ternary had _property is null ? [] as its own arm). There's currently no test for calling OfType<SomeDummyProperty>() on a PropertyBag that contains only a TestNodeStateProperty (i.e., _testNodeStateProperty is set but _property is null).

Impact: The early-return path added here goes unverified. If a future refactor accidentally removes it, a silent linear walk over a null reference would produce incorrect results.

Suggestion: Add a test case to PropertyBagTests:

[TestMethod]publicvoidOfType_WithOnlyTestNodeStateProperty_ReturnsEmpty(){PropertyBagproperty=new();property.Add(PassedTestNodeStateProperty.CachedInstance);// _property is null; _testNodeStateProperty is set — exercises the new early-return pathAssert.IsEmpty(property.OfType<DummyProperty>());}

current = current.Next;
}

return !foundAny

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Coverage] The !foundAny ? [] return at the end of the while-loop (no matching property found in a non-empty bag) is not tested. DummyProperty2 is used in Single / SingleOrDefault tests, but OfType<DummyProperty2>() is never called on a bag that has other properties.

Impact: The new manual traversal diverges from the old LINQ-based _property.OfType<TProperty>(). A bug that incorrectly returns null or throws instead of [] when no match is found would go undetected.

Suggestion: Add one assertion to the existing OfType_Should_Return_CorrectObject test:

// No DummyProperty2 in the bag — exercises the "no match found" path in the while-loopAssert.IsEmpty(property.OfType<DummyProperty2>());

@Evangelink

Copy link
Copy Markdown
MemberAuthor

Copilot address review comments

CopilotAI review requested due to automatic review settings April 29, 2026 05:16
@Evangelink
Amaury Levé (Evangelink) removed the request for review from CopilotApril 29, 2026 05:16

CopilotAI commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

Copilot address review comments

Added the three missing test cases in commits 911b697 and 813c705:

  • OfType_Should_Return_CorrectObject — extended with Assert.IsEmpty(property.OfType<DummyProperty2>()) to cover the "no match found" path
  • OfType_WithSingleMatch_ReturnsSingleItemArray — new test covering the single-item fast path ([first!] when overflow is null)
  • OfType_WithOnlyTestNodeStateProperty_ReturnsEmpty — new test covering the _property is null early return when only a TestNodeStateProperty is present

@Evangelink
Amaury Levé (Evangelink) merged commit e309705 into mainApr 29, 2026
24 checks passed
@Evangelink
Amaury Levé (Evangelink) deleted the efficiency/propertybag-oftype-no-state-machine-8f9c5bd2284aea80 branch April 29, 2026 09:14
Amaury Levé (Evangelink) added a commit that referenced this pull request May 12, 2026
… PropertyBag.OfType(T)() (#7917)
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Efficiency Improver] perf: eliminate yield-iterator state machine in PropertyBag.OfType(T)()

3 participants

@Evangelink
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

[Efficiency Improver] perf: eliminate yield-iterator state machine in PropertyBag.OfType(T)() - #7917

Merged
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
efficiency/propertybag-oftype-no-state-machine-8f9c5bd2284aea80
Apr 29, 2026
Merged

[Efficiency Improver] perf: eliminate yield-iterator state machine in PropertyBag.OfType(T)()#7917
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
efficiency/propertybag-oftype-no-state-machine-8f9c5bd2284aea80

Conversation

@Evangelink

Copy link
Copy Markdown
Member

Replace the yield-based iterator from Property.OfType() with a direct linked-list walk inside PropertyBag.OfType().

Before: _property.OfType() (yield return) allocates one state- machine heap object per call, even when no matching property is found.

After: direct while-loop walk; no state-machine allocated; returns [] for the common case (no matching property) with zero heap allocations beyond the empty array constant.

Fixes#7914

Replace the yield-based iterator from Property.OfType<TProperty>() with
a direct linked-list walk inside PropertyBag.OfType<TProperty>().
Before: _property.OfType<TProperty>() (yield return) allocates one state-
machine heap object per call, even when no matching property is found.
After: direct while-loop walk; no state-machine allocated; returns [] for
the common case (no matching property) with zero heap allocations beyond
the empty array constant.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings April 29, 2026 04:49

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR optimizes a hot-path in Microsoft.Testing.Platform by removing the yield return-based iterator allocation from PropertyBag.OfType<TProperty>() and replacing it with a direct linked-list traversal.

Changes:

  • Replace [.. _property.OfType<TProperty>()] (yield iterator) with a manual while loop over the Property linked list.
  • Preserve existing fast-path behavior for TestNodeStateProperty lookups and the empty/null property-list cases.
Show a summary per file
FileDescription
src/Platform/Microsoft.Testing.Platform/Messages/PropertyBag.csReworks OfType<TProperty>() to avoid iterator state-machine allocations by directly walking the linked list.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 0

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-04-29
Repository: microsoft/testfx

Key Findings

The PR replaces the yield-based OfType<TProperty>() iterator with a direct linked-list walk, introducing three distinct return paths:

  1. _testNodeStateProperty is TProperty[testNodeStateProperty] (unchanged)
  2. New combined early-return: typeof(TestNodeStateProperty).IsAssignableFrom(typeof(TProperty)) || _property is null[]
  3. New while-loop: !foundAny[], single match → [first!], multiple → [.. overflow]

Paths 2 and 3 (both new) have gaps in PropertyBagTests:

GapMissing pathRisk
_property is null early-returnOfType<DummyProperty>() on a bag with only TestNodeStatePropertySilent regression if early-return is dropped
Single-item fast pathOfType<DummyProperty>() with exactly 1 match[first!] returns wrong result/throws undetected
No-match path in walkOfType<DummyProperty2>() on a non-empty bagManual loop doesn't cover LINQ's implicitly empty-on-no-match

The production logic itself looks correct — the overflow initialization overflow ??= [first!] correctly seeds the list with the first match before adding the second. No functional bugs detected.

Recommendations

  1. Add an assertion Assert.IsEmpty(property.OfType<DummyProperty>()) for a bag containing only a TestNodeStateProperty.
  2. Add OfType<DummyProperty>() with exactly 1 DummyProperty in the bag.
  3. Add Assert.IsEmpty(property.OfType<DummyProperty2>()) to the existing OfType_Should_Return_CorrectObject test (one line, costs nothing).

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

return !foundAny
? []
: _property is null ? [] : [.. _property.OfType<TProperty>()];
: overflow is not null ? [.. overflow] : [first!];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Coverage] The single-item fast path ([first!] when overflow is null) is not exercised by any existing test. OfType_Should_Return_CorrectObject only tests with 2 DummyProperty instances, which goes through the overflow branch. The [first!] path needs its own case.

Impact: A regression where a single-result bag returns [] or throws would not be caught.

Suggestion: Extend OfType_Should_Return_CorrectObject or add a dedicated test:

PropertyBagbag=new();DummyPropertysingle=new();bag.Add(single);bag.Add(PassedTestNodeStateProperty.CachedInstance);TProperty[]result=bag.OfType<DummyProperty>();Assert.HasCount(1,result);Assert.AreSame(single,result[0]);


// We don't want to allocate an array if we know that we're looking for a TestNodeStateProperty
return typeof(TestNodeStateProperty).IsAssignableFrom(typeof(TProperty))
if (typeof(TestNodeStateProperty).IsAssignableFrom(typeof(TProperty)) || _property is null)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Coverage] The || _property is null branch is new (the original ternary had _property is null ? [] as its own arm). There's currently no test for calling OfType<SomeDummyProperty>() on a PropertyBag that contains only a TestNodeStateProperty (i.e., _testNodeStateProperty is set but _property is null).

Impact: The early-return path added here goes unverified. If a future refactor accidentally removes it, a silent linear walk over a null reference would produce incorrect results.

Suggestion: Add a test case to PropertyBagTests:

[TestMethod]publicvoidOfType_WithOnlyTestNodeStateProperty_ReturnsEmpty(){PropertyBagproperty=new();property.Add(PassedTestNodeStateProperty.CachedInstance);// _property is null; _testNodeStateProperty is set — exercises the new early-return pathAssert.IsEmpty(property.OfType<DummyProperty>());}

current = current.Next;
}

return !foundAny

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Coverage] The !foundAny ? [] return at the end of the while-loop (no matching property found in a non-empty bag) is not tested. DummyProperty2 is used in Single / SingleOrDefault tests, but OfType<DummyProperty2>() is never called on a bag that has other properties.

Impact: The new manual traversal diverges from the old LINQ-based _property.OfType<TProperty>(). A bug that incorrectly returns null or throws instead of [] when no match is found would go undetected.

Suggestion: Add one assertion to the existing OfType_Should_Return_CorrectObject test:

// No DummyProperty2 in the bag — exercises the "no match found" path in the while-loopAssert.IsEmpty(property.OfType<DummyProperty2>());

@Evangelink

Copy link
Copy Markdown
MemberAuthor

Copilot address review comments

CopilotAI review requested due to automatic review settings April 29, 2026 05:16
@Evangelink
Amaury Levé (Evangelink) removed the request for review from CopilotApril 29, 2026 05:16

CopilotAI commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

Copilot address review comments

Added the three missing test cases in commits 911b697 and 813c705:

  • OfType_Should_Return_CorrectObject — extended with Assert.IsEmpty(property.OfType<DummyProperty2>()) to cover the "no match found" path
  • OfType_WithSingleMatch_ReturnsSingleItemArray — new test covering the single-item fast path ([first!] when overflow is null)
  • OfType_WithOnlyTestNodeStateProperty_ReturnsEmpty — new test covering the _property is null early return when only a TestNodeStateProperty is present

@Evangelink
Amaury Levé (Evangelink) merged commit e309705 into mainApr 29, 2026
24 checks passed
@Evangelink
Amaury Levé (Evangelink) deleted the efficiency/propertybag-oftype-no-state-machine-8f9c5bd2284aea80 branch April 29, 2026 09:14
Amaury Levé (Evangelink) added a commit that referenced this pull request May 12, 2026
… PropertyBag.OfType(T)() (#7917)
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Efficiency Improver] perf: eliminate yield-iterator state machine in PropertyBag.OfType(T)()

3 participants

@Evangelink
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[Efficiency Improver] perf: eliminate yield-iterator state machine in PropertyBag.OfType(T)() - #7917

Merged
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
efficiency/propertybag-oftype-no-state-machine-8f9c5bd2284aea80
Apr 29, 2026
Merged

[Efficiency Improver] perf: eliminate yield-iterator state machine in PropertyBag.OfType(T)()#7917
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
efficiency/propertybag-oftype-no-state-machine-8f9c5bd2284aea80

Conversation

@Evangelink

Copy link
Copy Markdown
Member

Replace the yield-based iterator from Property.OfType() with a direct linked-list walk inside PropertyBag.OfType().

Before: _property.OfType() (yield return) allocates one state- machine heap object per call, even when no matching property is found.

After: direct while-loop walk; no state-machine allocated; returns [] for the common case (no matching property) with zero heap allocations beyond the empty array constant.

Fixes#7914

Replace the yield-based iterator from Property.OfType<TProperty>() with
a direct linked-list walk inside PropertyBag.OfType<TProperty>().
Before: _property.OfType<TProperty>() (yield return) allocates one state-
machine heap object per call, even when no matching property is found.
After: direct while-loop walk; no state-machine allocated; returns [] for
the common case (no matching property) with zero heap allocations beyond
the empty array constant.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings April 29, 2026 04:49

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR optimizes a hot-path in Microsoft.Testing.Platform by removing the yield return-based iterator allocation from PropertyBag.OfType<TProperty>() and replacing it with a direct linked-list traversal.

Changes:

  • Replace [.. _property.OfType<TProperty>()] (yield iterator) with a manual while loop over the Property linked list.
  • Preserve existing fast-path behavior for TestNodeStateProperty lookups and the empty/null property-list cases.
Show a summary per file
FileDescription
src/Platform/Microsoft.Testing.Platform/Messages/PropertyBag.csReworks OfType<TProperty>() to avoid iterator state-machine allocations by directly walking the linked list.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 0

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-04-29
Repository: microsoft/testfx

Key Findings

The PR replaces the yield-based OfType<TProperty>() iterator with a direct linked-list walk, introducing three distinct return paths:

  1. _testNodeStateProperty is TProperty[testNodeStateProperty] (unchanged)
  2. New combined early-return: typeof(TestNodeStateProperty).IsAssignableFrom(typeof(TProperty)) || _property is null[]
  3. New while-loop: !foundAny[], single match → [first!], multiple → [.. overflow]

Paths 2 and 3 (both new) have gaps in PropertyBagTests:

GapMissing pathRisk
_property is null early-returnOfType<DummyProperty>() on a bag with only TestNodeStatePropertySilent regression if early-return is dropped
Single-item fast pathOfType<DummyProperty>() with exactly 1 match[first!] returns wrong result/throws undetected
No-match path in walkOfType<DummyProperty2>() on a non-empty bagManual loop doesn't cover LINQ's implicitly empty-on-no-match

The production logic itself looks correct — the overflow initialization overflow ??= [first!] correctly seeds the list with the first match before adding the second. No functional bugs detected.

Recommendations

  1. Add an assertion Assert.IsEmpty(property.OfType<DummyProperty>()) for a bag containing only a TestNodeStateProperty.
  2. Add OfType<DummyProperty>() with exactly 1 DummyProperty in the bag.
  3. Add Assert.IsEmpty(property.OfType<DummyProperty2>()) to the existing OfType_Should_Return_CorrectObject test (one line, costs nothing).

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

return !foundAny
? []
: _property is null ? [] : [.. _property.OfType<TProperty>()];
: overflow is not null ? [.. overflow] : [first!];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Coverage] The single-item fast path ([first!] when overflow is null) is not exercised by any existing test. OfType_Should_Return_CorrectObject only tests with 2 DummyProperty instances, which goes through the overflow branch. The [first!] path needs its own case.

Impact: A regression where a single-result bag returns [] or throws would not be caught.

Suggestion: Extend OfType_Should_Return_CorrectObject or add a dedicated test:

PropertyBagbag=new();DummyPropertysingle=new();bag.Add(single);bag.Add(PassedTestNodeStateProperty.CachedInstance);TProperty[]result=bag.OfType<DummyProperty>();Assert.HasCount(1,result);Assert.AreSame(single,result[0]);


// We don't want to allocate an array if we know that we're looking for a TestNodeStateProperty
return typeof(TestNodeStateProperty).IsAssignableFrom(typeof(TProperty))
if (typeof(TestNodeStateProperty).IsAssignableFrom(typeof(TProperty)) || _property is null)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Coverage] The || _property is null branch is new (the original ternary had _property is null ? [] as its own arm). There's currently no test for calling OfType<SomeDummyProperty>() on a PropertyBag that contains only a TestNodeStateProperty (i.e., _testNodeStateProperty is set but _property is null).

Impact: The early-return path added here goes unverified. If a future refactor accidentally removes it, a silent linear walk over a null reference would produce incorrect results.

Suggestion: Add a test case to PropertyBagTests:

[TestMethod]publicvoidOfType_WithOnlyTestNodeStateProperty_ReturnsEmpty(){PropertyBagproperty=new();property.Add(PassedTestNodeStateProperty.CachedInstance);// _property is null; _testNodeStateProperty is set — exercises the new early-return pathAssert.IsEmpty(property.OfType<DummyProperty>());}

current = current.Next;
}

return !foundAny

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Coverage] The !foundAny ? [] return at the end of the while-loop (no matching property found in a non-empty bag) is not tested. DummyProperty2 is used in Single / SingleOrDefault tests, but OfType<DummyProperty2>() is never called on a bag that has other properties.

Impact: The new manual traversal diverges from the old LINQ-based _property.OfType<TProperty>(). A bug that incorrectly returns null or throws instead of [] when no match is found would go undetected.

Suggestion: Add one assertion to the existing OfType_Should_Return_CorrectObject test:

// No DummyProperty2 in the bag — exercises the "no match found" path in the while-loopAssert.IsEmpty(property.OfType<DummyProperty2>());

@Evangelink

Copy link
Copy Markdown
MemberAuthor

Copilot address review comments

CopilotAI review requested due to automatic review settings April 29, 2026 05:16
@Evangelink
Amaury Levé (Evangelink) removed the request for review from CopilotApril 29, 2026 05:16

CopilotAI commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

Copilot address review comments

Added the three missing test cases in commits 911b697 and 813c705:

  • OfType_Should_Return_CorrectObject — extended with Assert.IsEmpty(property.OfType<DummyProperty2>()) to cover the "no match found" path
  • OfType_WithSingleMatch_ReturnsSingleItemArray — new test covering the single-item fast path ([first!] when overflow is null)
  • OfType_WithOnlyTestNodeStateProperty_ReturnsEmpty — new test covering the _property is null early return when only a TestNodeStateProperty is present

@Evangelink
Amaury Levé (Evangelink) merged commit e309705 into mainApr 29, 2026
24 checks passed
@Evangelink
Amaury Levé (Evangelink) deleted the efficiency/propertybag-oftype-no-state-machine-8f9c5bd2284aea80 branch April 29, 2026 09:14
Amaury Levé (Evangelink) added a commit that referenced this pull request May 12, 2026
… PropertyBag.OfType(T)() (#7917)
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Efficiency Improver] perf: eliminate yield-iterator state machine in PropertyBag.OfType(T)()

3 participants

@Evangelink
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[Efficiency Improver] perf: eliminate yield-iterator state machine in PropertyBag.OfType(T)() - #7917

Merged
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
efficiency/propertybag-oftype-no-state-machine-8f9c5bd2284aea80
Apr 29, 2026
Merged

[Efficiency Improver] perf: eliminate yield-iterator state machine in PropertyBag.OfType(T)()#7917
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
efficiency/propertybag-oftype-no-state-machine-8f9c5bd2284aea80

Conversation

@Evangelink

Copy link
Copy Markdown
Member

Replace the yield-based iterator from Property.OfType() with a direct linked-list walk inside PropertyBag.OfType().

Before: _property.OfType() (yield return) allocates one state- machine heap object per call, even when no matching property is found.

After: direct while-loop walk; no state-machine allocated; returns [] for the common case (no matching property) with zero heap allocations beyond the empty array constant.

Fixes#7914

Replace the yield-based iterator from Property.OfType<TProperty>() with
a direct linked-list walk inside PropertyBag.OfType<TProperty>().
Before: _property.OfType<TProperty>() (yield return) allocates one state-
machine heap object per call, even when no matching property is found.
After: direct while-loop walk; no state-machine allocated; returns [] for
the common case (no matching property) with zero heap allocations beyond
the empty array constant.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings April 29, 2026 04:49

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR optimizes a hot-path in Microsoft.Testing.Platform by removing the yield return-based iterator allocation from PropertyBag.OfType<TProperty>() and replacing it with a direct linked-list traversal.

Changes:

  • Replace [.. _property.OfType<TProperty>()] (yield iterator) with a manual while loop over the Property linked list.
  • Preserve existing fast-path behavior for TestNodeStateProperty lookups and the empty/null property-list cases.
Show a summary per file
FileDescription
src/Platform/Microsoft.Testing.Platform/Messages/PropertyBag.csReworks OfType<TProperty>() to avoid iterator state-machine allocations by directly walking the linked list.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 0

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-04-29
Repository: microsoft/testfx

Key Findings

The PR replaces the yield-based OfType<TProperty>() iterator with a direct linked-list walk, introducing three distinct return paths:

  1. _testNodeStateProperty is TProperty[testNodeStateProperty] (unchanged)
  2. New combined early-return: typeof(TestNodeStateProperty).IsAssignableFrom(typeof(TProperty)) || _property is null[]
  3. New while-loop: !foundAny[], single match → [first!], multiple → [.. overflow]

Paths 2 and 3 (both new) have gaps in PropertyBagTests:

GapMissing pathRisk
_property is null early-returnOfType<DummyProperty>() on a bag with only TestNodeStatePropertySilent regression if early-return is dropped
Single-item fast pathOfType<DummyProperty>() with exactly 1 match[first!] returns wrong result/throws undetected
No-match path in walkOfType<DummyProperty2>() on a non-empty bagManual loop doesn't cover LINQ's implicitly empty-on-no-match

The production logic itself looks correct — the overflow initialization overflow ??= [first!] correctly seeds the list with the first match before adding the second. No functional bugs detected.

Recommendations

  1. Add an assertion Assert.IsEmpty(property.OfType<DummyProperty>()) for a bag containing only a TestNodeStateProperty.
  2. Add OfType<DummyProperty>() with exactly 1 DummyProperty in the bag.
  3. Add Assert.IsEmpty(property.OfType<DummyProperty2>()) to the existing OfType_Should_Return_CorrectObject test (one line, costs nothing).

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

return !foundAny
? []
: _property is null ? [] : [.. _property.OfType<TProperty>()];
: overflow is not null ? [.. overflow] : [first!];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Coverage] The single-item fast path ([first!] when overflow is null) is not exercised by any existing test. OfType_Should_Return_CorrectObject only tests with 2 DummyProperty instances, which goes through the overflow branch. The [first!] path needs its own case.

Impact: A regression where a single-result bag returns [] or throws would not be caught.

Suggestion: Extend OfType_Should_Return_CorrectObject or add a dedicated test:

PropertyBagbag=new();DummyPropertysingle=new();bag.Add(single);bag.Add(PassedTestNodeStateProperty.CachedInstance);TProperty[]result=bag.OfType<DummyProperty>();Assert.HasCount(1,result);Assert.AreSame(single,result[0]);


// We don't want to allocate an array if we know that we're looking for a TestNodeStateProperty
return typeof(TestNodeStateProperty).IsAssignableFrom(typeof(TProperty))
if (typeof(TestNodeStateProperty).IsAssignableFrom(typeof(TProperty)) || _property is null)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Coverage] The || _property is null branch is new (the original ternary had _property is null ? [] as its own arm). There's currently no test for calling OfType<SomeDummyProperty>() on a PropertyBag that contains only a TestNodeStateProperty (i.e., _testNodeStateProperty is set but _property is null).

Impact: The early-return path added here goes unverified. If a future refactor accidentally removes it, a silent linear walk over a null reference would produce incorrect results.

Suggestion: Add a test case to PropertyBagTests:

[TestMethod]publicvoidOfType_WithOnlyTestNodeStateProperty_ReturnsEmpty(){PropertyBagproperty=new();property.Add(PassedTestNodeStateProperty.CachedInstance);// _property is null; _testNodeStateProperty is set — exercises the new early-return pathAssert.IsEmpty(property.OfType<DummyProperty>());}

current = current.Next;
}

return !foundAny

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Coverage] The !foundAny ? [] return at the end of the while-loop (no matching property found in a non-empty bag) is not tested. DummyProperty2 is used in Single / SingleOrDefault tests, but OfType<DummyProperty2>() is never called on a bag that has other properties.

Impact: The new manual traversal diverges from the old LINQ-based _property.OfType<TProperty>(). A bug that incorrectly returns null or throws instead of [] when no match is found would go undetected.

Suggestion: Add one assertion to the existing OfType_Should_Return_CorrectObject test:

// No DummyProperty2 in the bag — exercises the "no match found" path in the while-loopAssert.IsEmpty(property.OfType<DummyProperty2>());

@Evangelink

Copy link
Copy Markdown
MemberAuthor

Copilot address review comments

CopilotAI review requested due to automatic review settings April 29, 2026 05:16
@Evangelink
Amaury Levé (Evangelink) removed the request for review from CopilotApril 29, 2026 05:16

CopilotAI commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

Copilot address review comments

Added the three missing test cases in commits 911b697 and 813c705:

  • OfType_Should_Return_CorrectObject — extended with Assert.IsEmpty(property.OfType<DummyProperty2>()) to cover the "no match found" path
  • OfType_WithSingleMatch_ReturnsSingleItemArray — new test covering the single-item fast path ([first!] when overflow is null)
  • OfType_WithOnlyTestNodeStateProperty_ReturnsEmpty — new test covering the _property is null early return when only a TestNodeStateProperty is present

@Evangelink
Amaury Levé (Evangelink) merged commit e309705 into mainApr 29, 2026
24 checks passed
@Evangelink
Amaury Levé (Evangelink) deleted the efficiency/propertybag-oftype-no-state-machine-8f9c5bd2284aea80 branch April 29, 2026 09:14
Amaury Levé (Evangelink) added a commit that referenced this pull request May 12, 2026
… PropertyBag.OfType(T)() (#7917)
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Efficiency Improver] perf: eliminate yield-iterator state machine in PropertyBag.OfType(T)()

3 participants

@Evangelink
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

[Efficiency Improver] perf: eliminate yield-iterator state machine in PropertyBag.OfType(T)() - #7917

Merged
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
efficiency/propertybag-oftype-no-state-machine-8f9c5bd2284aea80
Apr 29, 2026
Merged

[Efficiency Improver] perf: eliminate yield-iterator state machine in PropertyBag.OfType(T)()#7917
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
efficiency/propertybag-oftype-no-state-machine-8f9c5bd2284aea80

Conversation

@Evangelink

Copy link
Copy Markdown
Member

Replace the yield-based iterator from Property.OfType() with a direct linked-list walk inside PropertyBag.OfType().

Before: _property.OfType() (yield return) allocates one state- machine heap object per call, even when no matching property is found.

After: direct while-loop walk; no state-machine allocated; returns [] for the common case (no matching property) with zero heap allocations beyond the empty array constant.

Fixes#7914

Replace the yield-based iterator from Property.OfType<TProperty>() with
a direct linked-list walk inside PropertyBag.OfType<TProperty>().
Before: _property.OfType<TProperty>() (yield return) allocates one state-
machine heap object per call, even when no matching property is found.
After: direct while-loop walk; no state-machine allocated; returns [] for
the common case (no matching property) with zero heap allocations beyond
the empty array constant.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings April 29, 2026 04:49

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR optimizes a hot-path in Microsoft.Testing.Platform by removing the yield return-based iterator allocation from PropertyBag.OfType<TProperty>() and replacing it with a direct linked-list traversal.

Changes:

  • Replace [.. _property.OfType<TProperty>()] (yield iterator) with a manual while loop over the Property linked list.
  • Preserve existing fast-path behavior for TestNodeStateProperty lookups and the empty/null property-list cases.
Show a summary per file
FileDescription
src/Platform/Microsoft.Testing.Platform/Messages/PropertyBag.csReworks OfType<TProperty>() to avoid iterator state-machine allocations by directly walking the linked list.

Copilot's findings

  • Files reviewed: 1/1 changed files
  • Comments generated: 0

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-04-29
Repository: microsoft/testfx

Key Findings

The PR replaces the yield-based OfType<TProperty>() iterator with a direct linked-list walk, introducing three distinct return paths:

  1. _testNodeStateProperty is TProperty[testNodeStateProperty] (unchanged)
  2. New combined early-return: typeof(TestNodeStateProperty).IsAssignableFrom(typeof(TProperty)) || _property is null[]
  3. New while-loop: !foundAny[], single match → [first!], multiple → [.. overflow]

Paths 2 and 3 (both new) have gaps in PropertyBagTests:

GapMissing pathRisk
_property is null early-returnOfType<DummyProperty>() on a bag with only TestNodeStatePropertySilent regression if early-return is dropped
Single-item fast pathOfType<DummyProperty>() with exactly 1 match[first!] returns wrong result/throws undetected
No-match path in walkOfType<DummyProperty2>() on a non-empty bagManual loop doesn't cover LINQ's implicitly empty-on-no-match

The production logic itself looks correct — the overflow initialization overflow ??= [first!] correctly seeds the list with the first match before adding the second. No functional bugs detected.

Recommendations

  1. Add an assertion Assert.IsEmpty(property.OfType<DummyProperty>()) for a bag containing only a TestNodeStateProperty.
  2. Add OfType<DummyProperty>() with exactly 1 DummyProperty in the bag.
  3. Add Assert.IsEmpty(property.OfType<DummyProperty2>()) to the existing OfType_Should_Return_CorrectObject test (one line, costs nothing).

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

return !foundAny
? []
: _property is null ? [] : [.. _property.OfType<TProperty>()];
: overflow is not null ? [.. overflow] : [first!];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Coverage] The single-item fast path ([first!] when overflow is null) is not exercised by any existing test. OfType_Should_Return_CorrectObject only tests with 2 DummyProperty instances, which goes through the overflow branch. The [first!] path needs its own case.

Impact: A regression where a single-result bag returns [] or throws would not be caught.

Suggestion: Extend OfType_Should_Return_CorrectObject or add a dedicated test:

PropertyBagbag=new();DummyPropertysingle=new();bag.Add(single);bag.Add(PassedTestNodeStateProperty.CachedInstance);TProperty[]result=bag.OfType<DummyProperty>();Assert.HasCount(1,result);Assert.AreSame(single,result[0]);


// We don't want to allocate an array if we know that we're looking for a TestNodeStateProperty
return typeof(TestNodeStateProperty).IsAssignableFrom(typeof(TProperty))
if (typeof(TestNodeStateProperty).IsAssignableFrom(typeof(TProperty)) || _property is null)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Coverage] The || _property is null branch is new (the original ternary had _property is null ? [] as its own arm). There's currently no test for calling OfType<SomeDummyProperty>() on a PropertyBag that contains only a TestNodeStateProperty (i.e., _testNodeStateProperty is set but _property is null).

Impact: The early-return path added here goes unverified. If a future refactor accidentally removes it, a silent linear walk over a null reference would produce incorrect results.

Suggestion: Add a test case to PropertyBagTests:

[TestMethod]publicvoidOfType_WithOnlyTestNodeStateProperty_ReturnsEmpty(){PropertyBagproperty=new();property.Add(PassedTestNodeStateProperty.CachedInstance);// _property is null; _testNodeStateProperty is set — exercises the new early-return pathAssert.IsEmpty(property.OfType<DummyProperty>());}

current = current.Next;
}

return !foundAny

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Coverage] The !foundAny ? [] return at the end of the while-loop (no matching property found in a non-empty bag) is not tested. DummyProperty2 is used in Single / SingleOrDefault tests, but OfType<DummyProperty2>() is never called on a bag that has other properties.

Impact: The new manual traversal diverges from the old LINQ-based _property.OfType<TProperty>(). A bug that incorrectly returns null or throws instead of [] when no match is found would go undetected.

Suggestion: Add one assertion to the existing OfType_Should_Return_CorrectObject test:

// No DummyProperty2 in the bag — exercises the "no match found" path in the while-loopAssert.IsEmpty(property.OfType<DummyProperty2>());

@Evangelink

Copy link
Copy Markdown
MemberAuthor

Copilot address review comments

CopilotAI review requested due to automatic review settings April 29, 2026 05:16
@Evangelink
Amaury Levé (Evangelink) removed the request for review from CopilotApril 29, 2026 05:16

CopilotAI commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

Copilot address review comments

Added the three missing test cases in commits 911b697 and 813c705:

  • OfType_Should_Return_CorrectObject — extended with Assert.IsEmpty(property.OfType<DummyProperty2>()) to cover the "no match found" path
  • OfType_WithSingleMatch_ReturnsSingleItemArray — new test covering the single-item fast path ([first!] when overflow is null)
  • OfType_WithOnlyTestNodeStateProperty_ReturnsEmpty — new test covering the _property is null early return when only a TestNodeStateProperty is present

@Evangelink
Amaury Levé (Evangelink) merged commit e309705 into mainApr 29, 2026
24 checks passed
@Evangelink
Amaury Levé (Evangelink) deleted the efficiency/propertybag-oftype-no-state-machine-8f9c5bd2284aea80 branch April 29, 2026 09:14
Amaury Levé (Evangelink) added a commit that referenced this pull request May 12, 2026
… PropertyBag.OfType(T)() (#7917)
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Efficiency Improver] perf: eliminate yield-iterator state machine in PropertyBag.OfType(T)()

3 participants

@Evangelink