Micro-optimization: Use Array.ConvertAll instead of LINQ .Select .ToArray

[Henr1k80]

Prerequisites

• I have added steps to test this contribution in the description below

Description

This is around 2x faster on .NET 9, and much faster on previous versions.

It is probably not in any insanely hot code paths, but I see it used in recursion.

I could not get benchmarks to run in the project, I had to use a separate project for it, so I haven't updated the numbers in the bench

[github-actions]

Hi there @Henr1k80, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

• It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
• The automated tests all pass (see "Checks" tab on this PR)
• The level of security for this contribution is the same or improved
• The level of performance for this contribution is the same or improved
• Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
• If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
• 💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[Henr1k80]

My numbers from a separate bench project

BenchmarkDotNet v0.15.4, Windows 11 (10.0.26100.6584/24H2/2024Update/HudsonValley)
Intel Core i7-10750H CPU 2.60GHz (Max: 2.59GHz), 1 CPU, 12 logical and 6 physical cores
[Host] : .NET Framework 4.8.1 (4.8.9310.0), X64 RyuJIT VectorSize=256
.NET 8.0 : .NET 8.0.20 (8.0.20, 8.0.2025.41914), X64 RyuJIT x86-64-v3
.NET 9.0 : .NET 9.0.9 (9.0.9, 9.0.925.41916), X64 RyuJIT x86-64-v3
.NET Framework 4.8.1 : .NET Framework 4.8.1 (4.8.9310.0), X64 RyuJIT VectorSize=256

Method	Job	Runtime	Mean	Error	StdDev	Gen0	Allocated
NativeArrayToArray	.NET 8.0	.NET 8.0	17.58 ns	0.725 ns	2.021 ns	0.0089	56 B
LinqOnArrayToArray	.NET 8.0	.NET 8.0	50.45 ns	1.446 ns	4.078 ns	0.0166	104 B
NativeArrayToArray	.NET 9.0	.NET 9.0	16.10 ns	0.390 ns	1.054 ns	0.0089	56 B
LinqOnArrayToArray	.NET 9.0	.NET 9.0	30.13 ns	0.668 ns	1.302 ns	0.0166	104 B
NativeArrayToArray	.NET Framework 4.8.1	.NET Framework 4.8.1	25.85 ns	0.459 ns	0.407 ns	0.0089	56 B
LinqOnArrayToArray	.NET Framework 4.8.1	.NET Framework 4.8.1	155.39 ns	3.146 ns	5.830 ns	0.0241	152 B

[JasonElkin]

Hey @Henr1k80, thanks for taking the time to look at this, and especially for benchmarking this - I love a benchmark and it makes our job a lot easier when reviewing PRs 🙌

I'll double-check with HQ, but I feel like Array.ConvertAll() is a bit of a double edged sword here...

Firstly, it's not used in the project (outside of some tests) at the moment so would be a new convention in an otherwise LINQ heavy codebase.

Secondly, it's faster because it saves an additional iteration and the overhead of creating an array... so we should really ask , why create the array in the first place?

ToArray() is used pretty liberally in Umbraco's codebase, and often right after LINQ has already made a new IEnumerable 🤦. Sometimes this is down to old-fashioned, or perhaps lazy, API design (I'm looking at you params), sometimes it doesn't really make sense use a ToArray() at all because it's going straight into an API that will accept an IEnumerable anyway, here's an example:

Umbraco-CMS/src/Umbraco.Core/Models/PublishedContent/ModelType.cs

Line 240 in 15da4fd

var args = Array.ConvertAll(type.GetGenericArguments(), x => MapToName(x, map, true));

So I suppose my question is this. In the ccode we control, should we be looking to remove the unnecessary ToArray() calls by refactoring out the array's altogether?

Then, where there's a method call that requires an array/params in a library we don't control, switch to using Array.ConvertAll() - e.g. here:

Umbraco-CMS/src/Umbraco.Core/Models/PublishedContent/ModelType.cs

Lines 130 to 131 in 15da4fd

	Type[] args = Array.ConvertAll(type.GetGenericArguments(), x => Map(x, modelTypes, true));
	return def.MakeGenericType(args);

[Henr1k80]

Hey @JasonElkin, thank you, much appreciated.

I must admit that I did not look at if was possible to do away with the array allocation entirely.
I just did 1:1.

I totally agree, arrays EVERYWHERE, I think it was mostly due to the initial version of CollectionBenchmarks.
Yes, arrays iterate faster than Lists.
Yes, it can be fast to .ToArray(), IF WE KNOW THE LENGTH of what we are copying to an array...
Otherwise a List is used to make the array, possibly a lot of allocations and List resizes, at least always more allocations.

And is the cost of allocating an array worth it, if we just enumerate once without the need for the length?

E.g. in your first example, a an array is allocated to call string.Join, that also accepts an IEnumerable.
If it is an array, Join can calculate total length & allocate the new string and write the joined values into it.
If it is an IEnumerable, length is not known and a StringBuilder is allocated, iterated values are written, and when iterations is done, the complete string can be allocated from the StringBuilder.

To be certain which mehod is the best, a benchmark has to be made for the used context.
What is the break even for number of values? What value lengths are used. What is the total joined string length?
And this code might be so old that the IEnumerable overload did not even exist.
Either the code is old, or they did not bother do make a benchmark.

Can the evaluation of the need for arrays be another PR? 🙂
Evaluation of the need for arrays require more analysis, and should start in the hottest of paths.

[JasonElkin]

💯 to so much of what you've said here @Henr1k80

Something that's been on my radar is to try and make it easier to contribute micro-optimizations into the codebase - here seems as good a place to start as anywhere 😁

I've had some guidance from HQ to help guide us here as well.

Cleaning up the ToArray() calls

Can the evaluation of the need for arrays be another PR?

Yes! Well, not one, probably many - focussing on hot paths first, and with proper analysis, as you point out.

All contributed code gets tested hands-on by one of us core collabs, then reviewed by HQ before making it into a release. For micro-optimizations we need a way that both core collabs and the devs at HQ can verify that it's an meaningful improvement.

That means that we basically need benchmarks for every optimization just like you've done here. And in cases like you've pointed out above that means benchmarks that cover that specific scenario so we can know if an Array is better than an IEnumerable in a given context.

Doing that one PR at a time helps us to turn contentious changes into a discussion, without blocking the obvious wins that we can merge quickly.

In summary: Multiple smaller PRs with relevant benchmarks, focussing on hot paths where arrays should be replaced. Does that make sense?

This PR

Can we narrow it down to only places where we're calling non-Umbraco methods that only accept an array?

Are you able to check which would be faster for string.Join() in Model.cs and pick the best option.

Then we'll leave the other ToArray() calls here be looked at on their own merit.

[Henr1k80]

(edited to actually use Array.ConvertAll)

Ok @JasonElkin, I had time to benchmark string.Join for array & IEnumerable overloads
It is faster to allocate a new array, IF you have an array in the first place.
Up to a certain length of course, but you would hopefully use a StringBuilder in that case anyway.

It should NOT be mistaken with doing .ToArray() on a e.g. .Where() enumerable, where the length is not known.

So the existing code in the PR is the fastest.

BenchmarkDotNet v0.15.4, Windows 11 (10.0.26200.6584)
Intel Core i7-10750H CPU 2.60GHz (Max: 2.59GHz), 1 CPU, 12 logical and 6 physical cores
.NET SDK 9.0.305
  [Host]   : .NET 9.0.9 (9.0.9, 9.0.925.41916), X64 RyuJIT x86-64-v3
  .NET 9.0 : .NET 9.0.9 (9.0.9, 9.0.925.41916), X64 RyuJIT x86-64-v3

Job=.NET 9.0  Runtime=.NET 9.0  

Method	ArraySize	Mean	Error	StdDev	Gen0	Allocated
AllocateArray	1	10.41 ns	0.269 ns	0.320 ns	0.0051	32 B
EnumerateDirectly	1	16.43 ns	0.381 ns	0.495 ns	0.0089	56 B
AllocateArray	4	46.00 ns	0.982 ns	1.694 ns	0.0166	104 B
EnumerateDirectly	4	47.09 ns	0.873 ns	1.006 ns	0.0166	104 B
AllocateArray	8	79.87 ns	1.649 ns	2.144 ns	0.0254	160 B
EnumerateDirectly	8	71.77 ns	1.487 ns	1.653 ns	0.0204	128 B
AllocateArray	16	149.67 ns	2.864 ns	2.679 ns	0.0446	280 B
EnumerateDirectly	16	135.54 ns	2.715 ns	3.434 ns	0.0293	184 B

Bench code:

Summary? summary = BenchmarkRunner.Run<StringJoinBenchmarks>();

[MemoryDiagnoser]
//[ShortRunJob(RuntimeMoniker.Net90)]
[SimpleJob(RuntimeMoniker.Net90)]
public class StringJoinBenchmarks
{
    private const string? Separator = ", ";
    private string[] _array = [];

    [Params(1, 4, 8, 16)]
    public int ArraySize
    {
        get => _array.Length;
        set => _array = Enumerable.Range(0, value).Select(i => i.ToString()).ToArray();
    }

    [Benchmark]
    public string AllocateArray()
    {
        string[] array = Array.ConvertAll(_array, s => s);
        return string.Join(Separator, array);
    }

    private IEnumerable<string> EnumerateArray()
    {
        foreach (string s in _array)
            yield return s;
    }

    [Benchmark]
    public string EnumerateDirectly()
    {
        IEnumerable<string> enumerable = EnumerateArray();
        return string.Join(Separator, enumerable);
    }
}

[JasonElkin]

Sorry @Henr1k80, that one got stuck in Limbo for a bit there, but it's merged now!
Thanks for your work on this!