Avoid boxing the struct enumerator in PropertyDictionary<T>.GetEnumerator()

[nareshjo]

🤖 AI-Generated Pull Request 🤖

This pull request was generated by the VS Perf Rel AI Agent. Please review this AI-generated PR with extra care! For more information, visit our wiki.

---

• Issue:
  PropertyDictionary<T>.GetEnumerator() enumerates its backing set through the IEnumerable<T> interface — it iterates _properties.Values, and RetrievableEntryHashSet<T>.Values returns this typed as ICollection<T>. Because the backing RetrievableEntryHashSet<T> exposes a value-type Enumerator, dispatching through the interface boxes that struct onto the heap on every enumeration: IEnumerable<T>.GetEnumerator() does return new Enumerator(this), and the struct is boxed to satisfy the interface return type.
  This runs once per project item that carries direct metadata during ProjectInstance construction (VS design-time / project-snapshot builds), making it a meaningful source of GC pressure during solution load.

  Allocation sites (TypeAllocated!Enumerator[Microsoft.Build.Evaluation.ProjectMetadata], ~97% of sampled allocations; a further ~3% as Enumerator[…Execution.ProjectPropertyInstance]):

  PropertyDictionary<T>.GetEnumerator   (yield iterator)
   → foreach over _properties.Values          ← ICollection<T>, forces interface dispatch
     → RetrievableEntryHashSet<T>.IEnumerable<T>.GetEnumerator   ← return new Enumerator(this)
       → clr.dll!JIT_New → boxed Enumerator[ProjectMetadata]     ← discarded after enumeration
  

  Callers on hot path as per Perfwatson traces (all funnel through PropertyDictionary<T>.GetEnumerator):

  InstantiateProjectItemInstance → ImmutableDictionaryExtensions.SetItems → PropertyDictionary.GetEnumerator          (~67%)
  InstantiateProjectItemInstance → CopyOnWritePropertyDictionary.ImportProperties → PropertyDictionary.GetEnumerator  (~31%)
  InstantiateProjectItemInstance → PropertyDictionary..ctor(IEnumerable<T>) → PropertyDictionary.GetEnumerator          (~3%)
  

  All variants converge on the same boxed value-type-enumerator leaf at RetrievableEntryHashSet<T>.IEnumerable<T>.GetEnumerator, rooted at ProjectSnapshotService.GenerateProjectInstance → ProjectInstance..ctor → CreateItemsSnapshot and attributed to GC pause time.

  See related failure in PRISM

• Issue type: Avoid boxing a value-type enumerator when an allocation-free struct enumerator exists on the same concrete type.

• Proposed fix: In PropertyDictionary<T>.GetEnumerator(), pattern-match the backing field to its concrete RetrievableValuedEntryHashSet<T> type and foreach over it, so the C# foreach binds to the struct-returning public Enumerator GetEnumerator() — no boxing. An else branch preserves the original interface enumeration for the non-deriving IRetrievableValuedEntryHashSet<T> implementers (ImmutableGlobalPropertiesCollectionConverter, ImmutableProjectPropertyCollectionConverter). The non-generic IEnumerable.GetEnumerator() now delegates to the generic method, removing a second boxing site. Because every hot-path caller routes through this single sink, one change covers all of them. The change mirrors the allocation-free pattern already used by PropertyDictionary<T>.Filter, is internal-only, and is behavior-preserving — identical enumeration order, version/concurrent-modification check, and read-lock scope (both paths drive the same RetrievableEntryHashSet<T>.Enumerator). Net effect: −1 heap allocation per enumeration on the concrete path, 0 new allocations introduced.

Best practices wiki
See related failure in PRISM
ADO work item

[Copilot]

Pull request overview

This PR reduces GC pressure during hot-path PropertyDictionary<T> enumeration by avoiding boxing of the struct enumerator from RetrievableEntryHashSet<T> when _properties is backed by the common concrete RetrievableValuedEntryHashSet<T> implementation.

Changes:

• Add a fast path in PropertyDictionary<T>.GetEnumerator() that enumerates RetrievableValuedEntryHashSet<T> directly so foreach binds to the struct-returning GetEnumerator() (no interface boxing).
• Preserve the existing interface-based enumeration behavior for non-RetrievableValuedEntryHashSet<T> implementations of IRetrievableValuedEntryHashSet<T>.
• Make non-generic IEnumerable.GetEnumerator() delegate to the generic GetEnumerator() to eliminate duplicate enumeration logic (and an extra boxing site).

[ViktorHofer]

/review

[github-actions]

✅ Expert Code Review (command) completed successfully!

Caution

agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.

Details

The threat detection results could not be parsed.

Review the workflow run logs for details.

Code review of PR #14272 complete — three dimensions evaluated against the actual diff and source.

[github-actions]

Caution

agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.

Details

The threat detection results could not be parsed.

Review the workflow run logs for details.

MSBuild Expert Review — 24-Dimension Analysis

✅ 24/24 dimensions clean — no findings.

Summary

This PR applies a well-scoped micro-optimization to PropertyDictionary<T>.GetEnumerator(). The change is correct, safe, and follows an established pattern already present in the same file (Filter() at line 609 uses the identical is RetrievableValuedEntryHashSet<T> hashSet fast path).

---

What the PR does

Hot path (common case): When _properties is a RetrievableValuedEntryHashSet<T> (all normally-constructed dictionaries), the new code calls the concrete type's public Enumerator GetEnumerator() directly. The compiler resolves this via pattern-based foreach, storing the struct Enumerator as a field of the iterator state machine — no boxing. The old code called _properties.Values.GetEnumerator() via IEnumerable<T>.GetEnumerator(), which returned new Enumerator(this) as IEnumerator<T> — one heap allocation per enumeration (boxing).

Else path (rare case): ImmutableGlobalPropertiesCollectionConverter and ImmutableProjectPropertyCollectionConverter fall back to _properties.Values, preserving the original behaviour.

IEnumerable.GetEnumerator() simplification: The old 10-line duplicate iterator (own lock + own yield return) is replaced by a 1-line delegation => GetEnumerator(). The non-generic path now also gets the struct-enumerator fast path for free.

---

Dimension results

Group	Dimensions	Result
Blocking	Backwards Compatibility, ChangeWave, Concurrency, Evaluation Model, Security	✅ All LGTM
Major	Performance, Test Coverage, API Surface, Design, Cross-Platform, Correctness	✅ All LGTM
Moderate	Code Simplification, Documentation, Scope, Logging, Build Infrastructure	✅ All LGTM
Nit	String Comparison, Naming, Idiomatic C#, SDK Integration, Dependency Mgmt, Error Messages	✅ All LGTM

Concurrency: Lock semantics are preserved. The using (_lock.EnterDisposableReadLock()) block wraps both new branches identically to the old single branch. The iterator state machine's Dispose() correctly releases the lock on early exit or full enumeration.

Correctness: The is type check correctly handles subclasses of RetrievableValuedEntryHashSet<T>. _properties is readonly and set in every constructor, so it can never be null at runtime.

Documentation: The inline comment is accurate and complete — it names the allocation it avoids, identifies why the fast path is safe to assume, and names the concrete types that reach the else branch.

Generated by Expert Code Review (command) for #14272 · 1.4K AIC · ⊞ 31.9K
_{Comment /review to run again}