Skip to content

Avoid allocations from LINQ.ToArray() #28387

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

Merged
merged 1 commit into from
Dec 10, 2020

Conversation

paulomorgado
Copy link
Contributor

@paulomorgado paulomorgado commented Dec 3, 2020

Enumerable.Array() enumerates and allocates items for no real purposes in this use case.

@ghost ghost added the area-hosting label Dec 3, 2020

Action<TContainerBuilder> pipeline = InvokeConfigureContainer;
for (int i = 0; i < filters.Length; i++)
foreach (var filter in filters.Reverse())
Copy link
Member

Choose a reason for hiding this comment

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

Why change this to use foreach + Reverse?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After removing the invocation of ToArray(), it made more sense to me to move the invocation of Reverse() to its usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry, but I don't get your point.

Copy link
Member

@JamesNK JamesNK Dec 6, 2020

Choose a reason for hiding this comment

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

The goal is to avoid allocations from the Reverse, right? Why not remove it and loop through the collection backwards?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry! It was a typo due to copy+paste.

In this case the goal is to avoid Enumerable.ToArray().

Copy link
Member

Choose a reason for hiding this comment

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

Why are you worried about ToArray in one scenario and Reverse in the other? Shouldn't these use the same approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not about one or the other. It's about both.

In one case, the size of the collection is checked, so I kept ToArray to check its size. Now I don't need to reverse it because I can iterate it in reverse order.

In the other case, the collection is only iterated, so Reverse serves the purpose.

Copy link
Contributor Author

@paulomorgado paulomorgado Dec 8, 2020

Choose a reason for hiding this comment

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

Now that you mention it, this is a pattern I often use:

var filters = HostingServiceProvider.GetRequiredService<IEnumerable<IStartupConfigureServicesFilter>>()
    .Reverse();
var enumerator = filters.GetEnumerator();
try
{
    // If there are no filters just run startup (makes IServiceProvider ConfigureServices(IServiceCollection services) work.
    if (!enumerator.Next())
    {
        return startup(services);
    }
    Action<IServiceCollection> pipeline = InvokeStartup;
    do
    {
        pipeline = enumerator.Current.ConfigureServices(pipeline);
    } while (enumerator.Next());
    pipeline(services);
    // We return null so that the host here builds the container (same result as void ConfigureServices(IServiceCollection services);
    return null;
}
finally
{
    enumerator.Dispose();
}

No ToArray!

(ducks and hides)

Copy link
Member

Choose a reason for hiding this comment

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

filters.Length == 0 could be an .Any() check instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NO! Don't do that!

If the type provides a Length/Count, use it. Otherwise, there will be interface implementation checks and an enumerator allocation.

Now, if you go with Reverse and use Any you'll pay that cost when you can avoid it with this code.

@paulomorgado paulomorgado force-pushed the paulomorgado/remove-reverse-2 branch from ca852f0 to bd348b9 Compare December 6, 2020 23:43
@paulomorgado paulomorgado changed the title Avoid allocations from LINQ.Reverse() Avoid allocations from LINQ.Array() Dec 7, 2020
@paulomorgado paulomorgado changed the title Avoid allocations from LINQ.Array() Avoid allocations from LINQ.ToArray() Dec 8, 2020
@Tratcher Tratcher merged commit a62e27e into dotnet:master Dec 10, 2020
@Tratcher
Copy link
Member

Thanks

@amcasey amcasey added the area-hosting Includes Hosting label Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-hosting Includes Hosting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants