-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Array.FindAll improvements #120336
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Array.FindAll improvements #120336
Conversation
…n just return the already allocated array
Thanks you for your contribution. The code is not performing well with best practices. Instead, just replace the |
Tagging subscribers to this area: @dotnet/area-system-runtime |
There may be more unintentional affect after rethinking. Initializing the array pool for each |
|
} | ||
|
||
List<T> list = new List<T>(); | ||
List<T>? heapMatches = null; // only allocate if needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is possible to extract the verbose code into a struct collection. I mean code like this:
public static T[] FindAll<T>(T[] array, Predicate<T> match)
{
if (array == null)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.array);
}
if (match == null)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.match);
}
OptimizedArrayBuilder<T> arrayBuilder = new OptimizedArrayBuilder<T>();
for (int i = 0; i < array.Length; i++)
{
if (match(array[i]))
{
arrayBuilder.Add(array[i]);
}
}
if (arrayBuilder.Count == 0)
{
return [];
}
return arrayBuilder.Build();
}
private ref struct OptimizedArrayBuilder<T>
{
const int InlineArrayLength = 16;
private List<T>? heapMatches;
private InlineArray16<T> stackAllocatedMatches;
private int stackAllocatedMatchesFound;
public SmallArrayBuilder()
{
this.heapMatches = null; // only allocate if needed
this.stackAllocatedMatches = default;
this.stackAllocatedMatchesFound = 0;
}
public readonly int Count => this.stackAllocatedMatchesFound + (heapMatches?.Count ?? 0);
public void Add(T item)
{
if (stackAllocatedMatchesFound < InlineArrayLength)
{
stackAllocatedMatches[stackAllocatedMatchesFound++] = item;
}
else
{
// Revert to the old logic, allocating and growing a List
heapMatches ??= [];
heapMatches.Add(item);
}
}
public readonly T[] Build()
{
T[] result = new T[this.Count];
int index = 0;
foreach (T stackAllocatedMatch in stackAllocatedMatches)
{
result[index++] = stackAllocatedMatch;
if (index >= stackAllocatedMatchesFound)
{
break;
}
}
heapMatches?.CopyTo(result.AsSpan(start: InlineArrayLength));
return result;
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was also thinking if it could be used instead of a regular List, in some places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is ArrayBuilder<T>
for non-pooling usage, and ValueListBuilder<T>
for pooling usage. Array.FindAll
isn't a common method and we aren't interested to introduce a new construct for it. Instead, it should use one of existing builder, optimizations should be done in the builder.
I found that there were room for improvement in
Array.FindAll
Now a small amount of matches are stack allocated and the List is only optionally allocated.
It is always faster with less allocations.
The stack allocated size can be made bigger, but it is fixed size, so it will have a price for no-match schenarios.
InlineArray16<T>
is only in .NET 10, but if this should be backported to .NET 8, we can create our local InlineMatchArrayX