Skip to content
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

Your example extension Replace method may be rather slow and allocate huge amounts of memory. #1

Open
Rhywden opened this issue May 23, 2022 · 2 comments

Comments

@Rhywden
Copy link

Rhywden commented May 23, 2022

Basically, I wondered what the best method was to replace one item in an immutable list. I first used the included Replace<t>(T oldItem, T newItem) "native" method (got the oldItem through Find() or FirstOrDefault)), then also tried the overload with the IComparer and then tried your extension method which has to call .toImmutableList() at the end as Select() only returns an IEnumerable.

Created a basic ImmutableList with 10,000 entries (each entry being public record TestRecord(string Id, string Content)) and ran it through a Benchmark when trying to replace the 10,000th entry (i.e. the last one).

This is the result:

image

As you can see, it's not only considerably slower, it also has a bit of an allocation problem.

@kjeske
Copy link
Owner

kjeske commented May 23, 2022

Good benchmark. Definitely my Replace method can be improved. I will check it more detailed later, but even the following implementation should be faster:

public static IEnumerable<T> Replace<T>(this IEnumerable<T> enumerable, Func<T, bool> predicate, Func<T, T> newItem) =>
    enumerable.Select(item => predicate(item) ? newItem(item) : item);

because it enumerates enumerable only once.

@Rhywden
Copy link
Author

Rhywden commented May 23, 2022

That won't help much, I'm afraid. It's the final .toImmutableList() which yields the huge allocation and the larger execution time. Without that conversion, your replace method is actually faster (but only yields an IEnumerable) and has comparable allocation.
That's how I stumbled across this in the first place ;)
Without the call to .toImmutableList() the benchmark looks like this:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants