Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Array.FindAll improvements #120336

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
eb7ecd7
Array.FindAll improvements
Henr1k80 Oct 2, 2025
5cb2e20
If all allocated are matched AND the match-array is not rented, we ca…
Henr1k80 Oct 2, 2025
c7e3678
Style changes, makes the method not fit within screen height.
Henr1k80 Oct 3, 2025
a0694b2
Use a small amount of stack allocated matches before optionally alloc…
Henr1k80 Oct 3, 2025
f6269d6
Style changes
Henr1k80 Oct 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 41 additions & 3 deletions src/libraries/System.Private.CoreLib/src/System/Array.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1548,15 +1548,53 @@ public static T[] FindAll<T>(T[] array, Predicate<T> match)
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.match);
}

List<T> list = new List<T>();
List<T>? heapMatches = null; // only allocate if needed
Copy link

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;
        }
    }

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Array.FindAll isn't a common method

It could also be a chicken & egg problem, it isn't commonly used, because it is inefficient.
With these changes, it will probably be more efficient than LINQ

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it isn't commonly used, because it is inefficient.

Not quite, because it's API shape that allocating a new array is inefficient in nature. LINQ is not efficient enough either.
Callers that care about the difference for small arrays are very likely to use alternative approach like manual for loop. In that case, the overhead of the delegate also matters.

Copy link
Contributor Author

@Henr1k80 Henr1k80 Oct 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In

runtime/src/libraries/Common/src/System/Runtime/InteropServices/ComEventsMethod.cs

Line 207 in b38de35

List<DelegateWrapper> tmp = new(wrappers);

There is this piece of code where there is also delegate overhead:

List<DelegateWrapper> tmp = new(wrappers);
tmp.RemoveAll(w => condition(w.Delegate));
newWrappers = tmp.ToArray();

A List with an internal array are heap allocated.
The List internal array is never overallocated. A little time could be spent on removing entries in the array.

Using Array.FindAll in its current form would probably overallocate its List or even worse, grow it.
A List with an internal array are heap (over)allocated. But no time spent on removing entries.

newWrappers = Array.FindAll(wrappers, w => !condition(w.Delegate));

If the new implementation was used, there would probably be used zero heap allocations, and no removals, to create the resulting array.

InlineArray16<T> stackAllocatedMatches = default;
const int InlineArrayLength = 16;
int stackAllocatedMatchesFound = 0;
for (int i = 0; i < array.Length; i++)
{
if (match(array[i]))
{
list.Add(array[i]);
if (stackAllocatedMatchesFound < InlineArrayLength)
{
stackAllocatedMatches[stackAllocatedMatchesFound++] = array[i];
}
else
{
// Revert to the old logic, allocating and growing a List
heapMatches ??= [];
heapMatches.Add(array[i]);
}
}
}
return list.ToArray();

if (stackAllocatedMatchesFound == 0)
{
return EmptyArray<T>.Value;
}

int resultLength = stackAllocatedMatchesFound;
if (heapMatches != null)
{
resultLength += heapMatches.Count;
}

T[] result = new T[resultLength];

int index = 0;
foreach (T stackAllocatedMatch in stackAllocatedMatches)
{
result[index++] = stackAllocatedMatch;
if (index >= stackAllocatedMatchesFound)
{
break;
}
}

heapMatches?.CopyTo(result.AsSpan(start: InlineArrayLength));

return result;
}

public static int FindIndex<T>(T[] array, Predicate<T> match)
Expand Down
Loading