Array will allocate less and little faster when creating RequestDelegateFactoryOptions in CreateRDFOptions

[marafiq]

It uses List, which gets assigned to RouteParameterNames as IEnumerable.

aspnetcore/src/Http/Routing/src/RouteEndpointDataSource.cs

Line 260 in 432e6a0

var routeParamNames = new List<string>(pattern.Parameters.Count);

Ran a benchmark with 10 strings of length 6. The results are below.

Another run:

The benchmark code used:

public class Benchmarks
    {
        private const int Length = 10;
        private readonly IReadOnlyCollection<string> _readOnlyCollection = new List<string>() { "param1","param2", "param1","param2", "param1","param2", "param1","param2", "param1","param2"}.AsReadOnly();
        [Benchmark]
        public void Add10StringsToList()
        {
            List<string> parameters = new(Length);
            foreach (var s in _readOnlyCollection)
            {
                parameters.Add(s);
            }

            
        }

        [Benchmark]
        public void Add10StringsToArray()
        {
            var parameters = new string[Length];
            var i = 0;
            foreach (var s in _readOnlyCollection)
            {
                parameters[i] = s;
                i++;
            }
            
        }
    }

[marafiq]

After spending a little bit more time with it, it seems like even array allocation is not required since parameter names are sourced from IReadOnlyCollection, and RequestDelegateFactoryOptions only requires IEnumerable<string> so all the allocation can go away. Either using the Linq Select or with the local function that yields.

Benchmark results

Benchmark code

[MemoryDiagnoser]
    public class Benchmarks
    {
        class RoutePatternPart
        {
            public RoutePatternPart(bool isOptional, string name)
            {
                IsOptional = isOptional;
                Name = name;
            }

            public bool IsOptional { get; }
            public string Name { get; }
        }

        private const int Length = 10;

        private readonly IReadOnlyCollection<RoutePatternPart> _readOnlyCollection = new List<RoutePatternPart>()
        {
            new(true, "param1"),
            new(true, "param1"),
            new(true, "param1"),
            new(true, "param1"),
            new(true, "param1"),
            new(true, "param1"),
            new(true, "param1"),
            new(true, "param1"),
            new(true, "param1"),
            new(true, "param1")
        };
        private readonly Consumer _consumer = new Consumer();
        [Benchmark]
        public void Add10StringsToList()
        {
            List<string> parameters = new(Length);
            foreach (var s in _readOnlyCollection)
            {
                parameters.Add(s.Name);
            }
            _consumer.Consume(parameters);
        }

        [Benchmark]
        public void Add10StringsToArray()
        {
            var parameters = new string[Length];
            var i = 0;
            foreach (var s in _readOnlyCollection)
            {
                parameters[i] = s.Name;
                i++;
            }
            _consumer.Consume(parameters);
        }
        
        [Benchmark]
        public void UseEnumerableWithLinqSelect()
        {
            var parameters = _readOnlyCollection.Select(x => x.Name);
            _consumer.Consume(parameters);
        }
        [Benchmark]
        public void UseEnumerableWithLocalFuncYield()
        {
            var parameters = LocalFuncEnumerable();

            _consumer.Consume(parameters);

            IEnumerable<string> LocalFuncEnumerable()
            {
                foreach (var routePatternPart in _readOnlyCollection)
                {
                    yield return routePatternPart.Name;
                }
            }
        }
    }

[mitchdenny]

How much impact would you expect this optimization to have over the lifetime of an app? CreateRDFOptions(...) isn't called typically once an application is up and running. Trying to save a few bytes per route over the whole lifetime of the app probably isn't worth it unless I'm missing something here.

Was there a particular performance issue you were experiencing where this was the solution?

[marafiq]

You meant only called once during startup.

Impact: It’s visible in the benchmarks which is saving few bytes in case of array. But allocating none of the parameter name strings if enumerating works and it’s marginally faster in case of array, while enumerating little bit more.
In case of enumerating, I would say it can save more given how many endpoints parameters and how long the names of those are.

It’s called only once isn’t really important in this context, my impression is .NET don’t like to allocated more than needed. In this context allocating less even few bytes is better since its startup. Also, scope of change seems limited.

Was there a particular performance issue you were experiencing where this was the solution?

No. I was reading an article and stumbled on it. I like to read code of this repo from time to time.

Having said that I understand it’s not high impact like breaking the bank. Though these can add up quickly.

“.. probably is not worth it..”

I certainly thought it’s worth filing the issue.

[captainsafia]

Triage: we don't think this is particularly high value given that it only occurs during startup but would welcome a PR to examine what the proposed change would look like.

[marafiq]

Thanks, I understand. I opened the draft PR nervously for you to examine if it will make sense.