Suggested by @LeaFrock.
Proposal
Replace the List<T> + CollectionsMarshal.AsSpan fast path in GenericsExtensions.ToFastArray(ICollection<T>) with plain IList<T> indexing:
public static TResult[] ToFastArray<TSource, TResult>(this ICollection<TSource>? source, Func<TSource, TResult> action)
{
if (source is not { Count: > 0 })
return [];
if (source is TSource[] sourceArray)
return sourceArray.ToFastArray(action);
if (source is IList<TSource> sourceList)
{
var listResult = new TResult[sourceList.Count];
for (var i = 0; i < listResult.Length; i++)
listResult[i] = action.Invoke(sourceList[i]);
return listResult;
}
var result = new TResult[source.Count];
var index = 0;
foreach (var item in source)
result[index++] = action.Invoke(item);
return result;
}
Rationale
The CollectionsMarshal.AsSpan branch was introduced in #659. Reviewing it against the actual call-sites, the case for replacing it is stronger than a pure style preference:
1. The List<T> branch is currently dead code. Every call-site of the ICollection<T> overload passes either an array (string[], T[], RedisValue[], EndPoint[] — caught by the array fast path above it) or a HashSet<string> (which is not IList<T> and falls through to the foreach in both the current and proposed versions). No List<T> is passed anywhere in the codebase, and GenericsExtensions is internal, so there are no external callers either. We are carrying an explicitly-unsafe API plus #if NET8_0_OR_GREATER conditional compilation for a path nothing reaches today.
2. CollectionsMarshal is documented as unsafe and implementation-dependent. The contract requires that the list is not structurally modified while the span is alive. Here action is a caller-supplied delegate, so that invariant is only guaranteed by convention. IList<T> indexing has no such caveat.
3. The performance difference is noise at this granularity. The per-element cost is dominated by action.Invoke() (a delegate call, typically serializer or cast work). Interface indexing versus span indexing is not measurable against that, especially with dynamic PGO devirtualizing the List<T> indexer on .NET 8+.
4. Broader type coverage. IList<T> also covers Collection<T>, ReadOnlyCollection<T>, ImmutableArray<T> and custom list wrappers, so future call-sites benefit without further changes.
The proposed version also collapses the null and empty checks into the single is not { Count: > 0 } pattern, which removes the currently duplicated early-return logic.
Acceptance criteria
Suggested by @LeaFrock.
Proposal
Replace the
List<T>+CollectionsMarshal.AsSpanfast path inGenericsExtensions.ToFastArray(ICollection<T>)with plainIList<T>indexing:Rationale
The
CollectionsMarshal.AsSpanbranch was introduced in #659. Reviewing it against the actual call-sites, the case for replacing it is stronger than a pure style preference:1. The
List<T>branch is currently dead code. Every call-site of theICollection<T>overload passes either an array (string[],T[],RedisValue[],EndPoint[]— caught by the array fast path above it) or aHashSet<string>(which is notIList<T>and falls through to theforeachin both the current and proposed versions). NoList<T>is passed anywhere in the codebase, andGenericsExtensionsisinternal, so there are no external callers either. We are carrying an explicitly-unsafe API plus#if NET8_0_OR_GREATERconditional compilation for a path nothing reaches today.2.
CollectionsMarshalis documented as unsafe and implementation-dependent. The contract requires that the list is not structurally modified while the span is alive. Hereactionis a caller-supplied delegate, so that invariant is only guaranteed by convention.IList<T>indexing has no such caveat.3. The performance difference is noise at this granularity. The per-element cost is dominated by
action.Invoke()(a delegate call, typically serializer or cast work). Interface indexing versus span indexing is not measurable against that, especially with dynamic PGO devirtualizing theList<T>indexer on .NET 8+.4. Broader type coverage.
IList<T>also coversCollection<T>,ReadOnlyCollection<T>,ImmutableArray<T>and custom list wrappers, so future call-sites benefit without further changes.The proposed version also collapses the null and empty checks into the single
is not { Count: > 0 }pattern, which removes the currently duplicated early-return logic.Acceptance criteria
CollectionsMarshalusage and the#if NET8_0_OR_GREATERblock removed fromGenericsExtensions.csSystem.Runtime.InteropServicesimport dropped if unused