Materialize trigger collections to eliminate ConcatIterator CPU waste#215
Materialize trigger collections to eliminate ConcatIterator CPU waste#215
Conversation
…r CPU waste Replace IEnumerable<T> fields with List<T> to avoid deeply nested ConcatIterator chains from repeated .Concat() calls. Each WithAdditionalTrigger call now uses List.Add() instead. Also cache the service provider hash code, use O(1) List.Count property instead of LINQ .Count(), and add count-based fast paths in ShouldUseSameServiceProvider to short-circuit before SequenceEqual. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> (cherry picked from commit 0cc6aa1)
There was a problem hiding this comment.
Pull request overview
This PR optimizes TriggersOptionExtension to avoid CPU overhead from repeatedly chaining IEnumerable.Concat() (creating deep ConcatIterator trees) by materializing trigger collections into List<T>, and adds a couple of small fast paths around service-provider caching comparisons.
Changes:
- Replace trigger registries from
IEnumerable<T>toList<T>and switchWithAdditional*methods toList.Add(). - Cache
GetServiceProviderHashCode()result and useList.Count/ count-based short-circuiting inShouldUseSameServiceProvider(). - Avoid LINQ
.Count()in debug info population.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/EntityFrameworkCore.Triggered/Infrastructure/Internal/TriggersOptionExtension.cs
Show resolved
Hide resolved
| private List<(object typeOrInstance, ServiceLifetime lifetime)>? _triggers; | ||
| private List<Type> _triggerTypes; |
There was a problem hiding this comment.
Changing _triggers to List<...> means the public Triggers property now returns a reference to a mutable internal collection (it’s exposed as IEnumerable, but consumers can still cast back to List and mutate). That can invalidate the new GetServiceProviderHashCode() cache and break the expectation that option extensions are effectively immutable. Consider returning a read-only wrapper (AsReadOnly()), an IReadOnlyList, or otherwise preventing external mutation.
| clone._triggerTypes = clone._triggerTypes.Concat(triggerTypesEnumerable); | ||
| } | ||
|
|
||
| clone._triggerTypes ??= new List<Type>(); |
There was a problem hiding this comment.
_triggerTypes is non-nullable and always initialized in constructors, so clone._triggerTypes ??= new List<Type>(); is redundant and suggests the field can be null. Either remove the null-coalescing assignment here, or (if null is a valid state) make _triggerTypes nullable consistently and handle that throughout.
| clone._triggerTypes ??= new List<Type>(); |
…r CPU waste
Replace IEnumerable fields with List to avoid deeply nested ConcatIterator chains from repeated .Concat() calls. Each WithAdditionalTrigger call now uses List.Add() instead. Also cache the service provider hash code, use O(1) List.Count property instead of LINQ .Count(), and add count-based fast paths in ShouldUseSameServiceProvider to short-circuit before SequenceEqual.
(cherry picked from commit 0cc6aa1)