Set collection capacity where information is available or for empty receives return unchangeable list

[danielmarbach]

Alternative to #20550

All SDK Contribution checklist:

This checklist is used to make sure that common guidelines for a pull request are followed.

• Please open PR in Draft mode if it is:
  • Work in progress or not intended to be merged.
  • Encountering multiple pipeline failures and working on fixes.
• If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
• I have read the contribution guidelines.
• The pull request does not introduce breaking changes.

General Guidelines and Best Practices

• Title of the pull request is clear and informative.
• There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

• Pull request includes test coverage for the included changes.

SDK Generation Guidelines

• The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as the commitid of your swagger spec or link to the swagger spec, used to generate the code. (Track 2 only)
• The *.csproj and AssemblyInfo.cs files have been updated with the new version of the SDK. Please double check nuget.org current release version.

Additional management plane SDK specific contribution checklist:

Note: Only applies to Microsoft.Azure.Management.[RP] or Azure.ResourceManager.[RP]

• Include updated management metadata.
• Update AzureRP.props to add/remove version info to maintain up to date API versions.

Management plane SDK Troubleshooting

• If this is very first SDK for a services and you are adding new service folders directly under /SDK, please add new service label and/or contact assigned reviewer.

• If the check fails at the Verify Code Generation step, please ensure:

  • Do not modify any code in generated folders.
  • Do not selectively include/remove generated files in the PR.
  • Do use generate.ps1/cmd to generate this PR instead of calling autorest directly.
    Please pay attention to the @microsoft.csharp version output after running generate.ps1. If it is lower than current released version (2.3.82), please run it again as it should pull down the latest version.

  Note: We have recently updated the PSH module called by generate.ps1 to emit additional data. This would help reduce/eliminate the Code Verification check error. Please run following command:

    `dotnet msbuild eng/mgmt.proj /t:Util /p:UtilityName=InstallPsModules`
  

Old outstanding PR cleanup

Please note:
If PRs (including draft) has been out for more than 60 days and there are no responses from our query or followups, they will be closed to maintain a concise list for our reviewers.

[ghost]

Thank you for your contribution @danielmarbach! We will review the pull request and get back to you soon.

[jsquire]

I wonder, do we save enough with this change that the intermediary list instantiation is worth it to get the count? We could do a yield return here and use an out param for the length, I think....

[danielmarbach]

We actually we would save something by changing the return type to the list and then there would be no enumerator allocation anymore. I initially wanted to go with that but then I remembered your enumerable all the things rule so I dismissed it.

But yeah yield return and counter would work too but still allocate the enumerator right?

[jsquire]

I think it's safe to return this as a list-type here, as it's an internal class. We try to keep the public API as generic as we can to avoid boxing ourselves in with too-specific a contract. Nothing breaks if we try to change this later.

@JoshLove-msft: Any objections to potentially changing the return type here?

[danielmarbach]

We could do a yield return here and use an out param for the length, I think....

iterators cannot have out parameters unfortunately

[danielmarbach]

Did a quick benchmark

   [Config(typeof(Config))]
    public class EnumerableCount
    {
        private List<object> data;
        private Consumer consumer;

        private class Config : ManualConfig
        {
            public Config()
            {
                AddExporter(MarkdownExporter.GitHub);
                AddDiagnoser(MemoryDiagnoser.Default);
                AddJob(Job.Default.WithInvocationCount(1000000));
            }
        }
        [IterationSetup]
        public void Setup()
        {
            data = Enumerable.Repeat(new SomeClass(), NumberOfItems).Cast<object>().ToList();
            consumer = new Consumer();
        }

        [Params(0, 1, 5)]
        public int NumberOfItems { get; set; }

        class SomeClass {}

        [Benchmark(Baseline = true)]
        public void Original()
        {
            GetValue<SomeClass>().Consume(consumer);
        }

        IEnumerable<TValue> GetValue<TValue>()
        {
            return data.Cast<TValue>();
        }

        [Benchmark]
        public void List()
        {
            GetValueList<SomeClass>().Consume(consumer);
        }

        IEnumerable<TValue> GetValueList<TValue>()
        {
            List<TValue> values = null;
            foreach (var item in data)
            {
                values ??= new List<TValue>(data.Count);
                values.Add((TValue)item);
            }
            return values ?? Enumerable.Empty<TValue>();
        }

        [Benchmark]
        public void ListReturnList()
        {
            GetValueListReturnList<SomeClass>().Consume(consumer);
        }

        List<TValue> GetValueListReturnList<TValue>()
        {
            var values = new List<TValue>(data.Count);
            foreach (var item in data)
            {
                values.Add((TValue)item);
            }
            return values;
        }
    }

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19042
AMD Ryzen 9 3950X, 1 CPU, 32 logical and 16 physical cores
.NET Core SDK=5.0.202
  [Host]     : .NET Core 3.1.14 (CoreCLR 4.700.21.16201, CoreFX 4.700.21.16208), X64 RyuJIT
  Job-UZLRLE : .NET Core 3.1.14 (CoreCLR 4.700.21.16201, CoreFX 4.700.21.16208), X64 RyuJIT

InvocationCount=1000000  

Method	NumberOfItems	Mean	Error	StdDev	Ratio	RatioSD	Gen 0	Gen 1	Gen 2	Allocated
Original	0	117.15 ns	0.907 ns	0.757 ns	1.00	0.00	0.0110	-	-	96 B
List	0	17.74 ns	0.226 ns	0.278 ns	0.15	0.00	-	-	-	-
ListReturnList	0	32.90 ns	0.561 ns	0.623 ns	0.28	0.01	0.0080	-	-	72 B
Original	1	150.39 ns	1.639 ns	1.533 ns	1.00	0.00	0.0110	-	-	96 B
List	1	56.27 ns	1.150 ns	1.412 ns	0.38	0.01	0.0120	-	-	104 B
ListReturnList	1	51.97 ns	0.755 ns	0.706 ns	0.35	0.00	0.0120	-	-	104 B
Original	5	241.15 ns	1.653 ns	1.466 ns	1.00	0.00	0.0110	-	-	96 B
List	5	140.63 ns	1.980 ns	1.852 ns	0.58	0.01	0.0160	-	-	136 B
ListReturnList	5	141.84 ns	2.861 ns	5.443 ns	0.56	0.02	0.0160	-	-	136 B

[danielmarbach]

This blows my mind every time

the for loop squeezes out even more

[danielmarbach]

Boah with outer loop for

    [Config(typeof(Config))]
    public class EnumerableCount
    {
        private List<object> data;

        private class Config : ManualConfig
        {
            public Config()
            {
                AddExporter(MarkdownExporter.GitHub);
                AddDiagnoser(MemoryDiagnoser.Default);
                AddJob(Job.Default.WithInvocationCount(1600));
            }
        }
        [IterationSetup]
        public void Setup()
        {
            data = Enumerable.Repeat(new SomeClass(), NumberOfItems).Cast<object>().ToList();
        }

        [Params(0, 1, 10, 32, 50, 100, 1000)]
        public int NumberOfItems { get; set; }

        public class SomeClass {}

        [Benchmark(Baseline = true)]
        public List<SomeClass> Original()
        {
            var copyList = new List<SomeClass>();
            foreach (var value in GetValue<SomeClass>())
            {
                copyList.Add(value);
            }

            return copyList;
        }

        IEnumerable<TValue> GetValue<TValue>()
        {
            return data.Cast<TValue>();
        }

        [Benchmark]
        public List<SomeClass> List()
        {
            List<SomeClass> copyList = null;
            var enumerable = GetValueList<SomeClass>();
            foreach (var value in enumerable)
            {
                copyList ??= enumerable is IReadOnlyCollection<SomeClass> readOnlyList
                    ? new List<SomeClass>(readOnlyList.Count)
                    : new List<SomeClass>();

                copyList.Add(value);
            }

            return copyList;
        }

        IEnumerable<TValue> GetValueList<TValue>()
        {
            List<TValue> values = null;
            foreach (var item in data)
            {
                values ??= new List<TValue>(data.Count);
                values.Add((TValue)item);
            }
            return values ?? Enumerable.Empty<TValue>();
        }

        [Benchmark]
        public List<SomeClass> ListReturnList()
        {
            List<SomeClass> copyList = null;
            var enumerable = GetValueListReturnList<SomeClass>();
            foreach (var value in enumerable)
            {
                copyList ??= enumerable is IReadOnlyCollection<SomeClass> readOnlyList
                    ? new List<SomeClass>(readOnlyList.Count)
                    : new List<SomeClass>();

                copyList.Add(value);
            }

            return copyList;
        }

        List<TValue> GetValueListReturnList<TValue>()
        {
            var values = new List<TValue>(data.Count);
            foreach (var item in data)
            {
                values.Add((TValue)item);
            }
            return values;
        }

        [Benchmark]
        public List<SomeClass> ListReturnListFor()
        {
            List<SomeClass> copyList = null;
            var enumerable = GetValueListReturnListFor<SomeClass>();
            foreach (var value in enumerable)
            {
                copyList ??= enumerable is IReadOnlyCollection<SomeClass> readOnlyList
                    ? new List<SomeClass>(readOnlyList.Count)
                    : new List<SomeClass>();

                copyList.Add(value);
            }

            return copyList;
        }

        [Benchmark]
        public List<SomeClass> ListForReturnListFor()
        {
            List<SomeClass> copyList = null;
            var enumerable = GetValueListReturnListFor<SomeClass>();
            for (var index = 0; index < enumerable.Count; index++)
            {
                var value = enumerable[index];
                copyList ??= enumerable is IReadOnlyCollection<SomeClass> readOnlyList
                    ? new List<SomeClass>(readOnlyList.Count)
                    : new List<SomeClass>();

                copyList.Add(value);
            }

            return copyList;
        }

        List<TValue> GetValueListReturnListFor<TValue>()
        {
            var values = new List<TValue>(data.Count);
            for (var index = 0; index < data.Count; index++)
            {
                var item = data[index];
                values.Add((TValue) item);
            }

            return values;
        }
    }

[JoshLove-msft]

I'm somewhat ambivalent on the foreach -> for change. It is nice to squeeze out more perf but I feel like we should be able to rely on language constructs and count on them to improve in perf over time. I am fine with either way here, but in general I don't think we should replace all of our foreach usages.

[danielmarbach]

For is a language construct ;) I'm aware that similar changes have been made in ASP.NET Core and the runtime as well in some places. Anyway I don't want to argue here I'm happy to go with the flow whatever you and the team prefer. I'm signing off now, so you can always edit the PR before merging

[JoshLove-msft]

Yeah I mean to say that it would be unfortunate if we couldn't use constructs as fundamental as foreach because of perf issues. I'm okay with the for loop here but mostly just wanted to call this out as a potential point of discussion. As always, thank you so much for the awesome contribution @danielmarbach! We greatly appreciate the time and effort you have put into improving the SDK.

[JoshLove-msft]

/azp run net - servicebus - tests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[danielmarbach]

Hmmm why would this fail suddenly?

[JoshLove-msft]

Hmmm why would this fail suddenly?

Looks like it was just a transient failure in one of the tests.

[danielmarbach]

🎆