Improve collection and clone performance - #138
Merged
Merged
Conversation
Co-authored-by: Copilot App <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request targets micro-optimizations in SharedCode’s shared helper libraries to reduce allocations and repeated work along hot paths (LINQ helpers, enumerable utilities, deep clone, and DI assembly dependency loading).
Changes:
- Replaces some LINQ-based iterations with explicit loops in collection helpers to reduce allocations.
- Updates enumerable helper implementations to avoid repeated enumeration/counting.
- Caches array rank metadata (lengths/lower bounds) in deep-clone array handling.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| SharedCode.DependencyInjection/TypeSourceSelector.cs | Attempts to pre-size dependency assembly list for fewer allocations during dependency scanning. |
| SharedCode.Core/Reflection/DeepCloneGenerator.cs | Avoids repeated Enumerable.Range(...).Select(...) allocations when cloning arrays by caching lengths/lower-bounds in arrays. |
| SharedCode.Core/Linq/EnumerableExtensions.cs | Reworks Aggregate/IsNotNullOrEmpty and tweaks sort-expression splitting to reduce overhead. |
| SharedCode.Core/Linq/CollectionExtensions.cs | Removes LINQ Where(...) usage in Find/FindAll and adds list fast-paths to reduce repeated enumeration. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
SummarySummary
CoverageSharedCode.Core - 27.3%
SharedCode.Core.Tests - 98.1%
SharedCode.Data - 8.3%
SharedCode.Data.Tests - 100%
|
…r, List capacity Co-authored-by: wforney <[email protected]>
Co-authored-by: wforney <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
SharedCode.Core/Linq/EnumerableExtensions.cs:268
TryGetNonEnumeratedCountis not available on thenetstandard2.0/netstandard2.1targets for this project, so this implementation will not compile for those TFMs. Consider switching toICollection<T>/IReadOnlyCollection<T>checks and falling back toAny()only for true enumerables.
@this is not null && (@this.TryGetNonEnumeratedCount(out var count) ? count > 0 : @this.Any());
SharedCode.DependencyInjection/TypeSourceSelector.cs:157
assembly.GetReferencedAssemblies()is now called before the surrounding try/catch. If that call throws (e.g., for a dynamic/reflection-only assembly), the method will now throw instead of falling back to returning a selector for just the providedassemblyas it did before. To keep the previous fault-tolerant behavior, guardGetReferencedAssemblies()and fall back to an empty array.
var referencedAssemblies = assembly.GetReferencedAssemblies();
SharedCode.Core/Linq/CollectionExtensions.cs:376
RemoveAllcurrently removes by value (ICollection<T>.Remove(item)), which can remove an earlier equal item when duplicates exist and can cause matches to be skipped (e.g.,[A,B,A,A]withmatch(A)may leave oneA). When the collection is indexable, remove by index to ensure the correct element is removed; for non-indexable collections, consider a two-pass approach (collect then remove) to ensure all matches are removed.
_ = @this.Remove(item);
SharedCode.Core/Linq/EnumerableExtensions.cs:292
- Changing
Split(' ')toSplit(' ', RemoveEmptyEntries)changes behavior for inputs with leading spaces (e.g., " Name" previously resulted in no sort becauseparts[0]was empty, but now it will sort byName). The PR description says behavior is kept intact; confirm this behavior change is acceptable, or preserve the previous semantics.
var parts = sortExpression.Split(' ', StringSplitOptions.RemoveEmptyEntries);
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
SharedCode.Core/Linq/EnumerableExtensions.cs:293
- Using
StringSplitOptions.RemoveEmptyEntrieschanges how leading spaces are handled (e.g." desc"previously resulted in an empty first token and returned the source unchanged; now it treats"desc"as the property name and will likely throw). If behavior is expected to stay intact, consider keeping the original split behavior.
/// <param name="this">The enumerable.</param>
/// <param name="sortExpression">The sort expression.</param>
/// <returns>The sorted enumerable.</returns>
SharedCode.Core/Linq/EnumerableExtensions.cs:63
Aggregate(..., defaultValue, aggregateFunction)now throws foraggregateFunction == nulleven when the source isnullor empty. Previously, the method returneddefaultValuewithout needing the function in those cases, so this changes behavior and can break callers that rely on the early default return path.
_ = aggregateFunction ?? throw new ArgumentNullException(nameof(aggregateFunction));
if (@this is null)
{
return defaultValue;
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
SharedCode.Core/Linq/EnumerableExtensions.cs:275
IsNotNullOrEmptynow callsTryGetNonEnumeratedCount, which is not available on thenetstandard2.0/netstandard2.1targets (SharedCode.Core multi-targets those frameworks). This will fail to compile for those TFMs unless guarded with conditional compilation or replaced with a portable approach.
public static bool IsNotNullOrEmpty<T>(this IEnumerable<T> @this) =>
@this is not null && (@this.TryGetNonEnumeratedCount(out var count) ? count > 0 : @this.Any());
SharedCode.Core/Linq/EnumerableExtensions.cs:58
Aggregate<T>(..., defaultValue, aggregateFunction)usesSystem.Linq.Enumerable.AggregateforICollection<T>/IReadOnlyCollection<T>but a manual aggregation loop for other enumerables. For value types, LINQ’sAggregaterequires a non-nullTreturn and will throw ifaggregateFunctionreturnsnull, while the manual path can returnnull(T?). This makes behavior depend on the runtime type of@this.
if (@this is ICollection<T> collection)
{
return collection.Count == 0
? defaultValue
: System.Linq.Enumerable.Aggregate(@this, (a, b) => aggregateFunction(a, b)!);
SharedCode.Core/Linq/CollectionExtensions.cs:184
- In
FindIndex(startIndex/count overload), thestartIndexrange check throwsArgumentNullException, which is incorrect for an out-of-range index and makes exception handling inconsistent for callers.
if (predicate(@this is IList<T> list ? list[i] : @this.ElementAt(i)))
Co-authored-by: wforney <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
SharedCode.Core/Linq/EnumerableExtensions.cs:275
IsNotNullOrEmptynow callsTryGetNonEnumeratedCount, which is not available for thenetstandard2.0/netstandard2.1targets of SharedCode.Core. This will fail to compile for those TFMs. Consider usingICollection<T>/IReadOnlyCollection<T>count checks (fast path) and only useTryGetNonEnumeratedCountunder#if NET6_0_OR_GREATER(or drop it entirely and fall back toAny()for netstandard).
public static bool IsNotNullOrEmpty<T>(this IEnumerable<T> @this) =>
@this is not null && (@this.TryGetNonEnumeratedCount(out var count) ? count > 0 : @this.Any());
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Improves a few hot paths in the shared helpers:\n\n- avoids LINQ allocations in collection find helpers\n- reduces repeated enumeration/counting in enumerable helpers\n- removes repeated rank/lower-bound lookups in deep clone array handling\n- pre-sizes the dependency assembly list\n\nThe change keeps behavior intact while trimming overhead in common utility paths.