Optimize sorting by leveraging ArrayPool for reduced allocations in ParameterCollection.

[danielmarbach]

Alternative to #4330

Commit 37a9afe optimized GetParametersAsString to use GetParametersEnumerable() directly instead of GetSortedParametersList().ToList(). While this avoided the intermediate List<KeyValuePair<string,string>> allocation, it exposed the pre-existing mutation bug where the original lists were being sorted in-place on every call.

Use ArrayPool<T>.Shared.Rent() to obtain temporary arrays for sorting, then return them to the pool. This approach:

• Zero heap allocations for the sorting operation (just rents from the pool)
• Prevents mutation of the original lists
• Maintains performance comparable to the buggy in-place sort

Benchmark Results

BenchmarkDotNet v0.15.0 shows the ArrayPool.Rent approach is competitive with alternatives:

• ArrayPool.Rent has identical performance to new List<T>(copy) but with ~40% less memory allocation
• ArrayPool.Rent is ~25% faster than LINQ OrderBy with ~3x less memory allocation
• The fix maintains the originally intended v4 optimization benefits while preventing the mutation bug

Although the span approach doesn't work due to the iterator. It is just here for comparison

Benchmark

Details

```csharp using System; using System.Buffers; using System.Collections.Generic; using System.Globalization; using System.Linq; using System.Runtime.CompilerServices;

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Columns;
using BenchmarkDotNet.Configs;
using BenchmarkDotNet.Diagnosers;
using BenchmarkDotNet.Exporters;
using BenchmarkDotNet.Jobs;
using BenchmarkDotNet.Reports;
using BenchmarkDotNet.Running;

namespace MutationBenchmark
{
[MemoryDiagnoser]
[SimpleJob]
[Config(typeof(Config))]
public class ParameterSortingBenchmarks
{
private List _stringList;
private List _doubleList;

    [Params(3, 10, 50)]
    public int ListSize { get; set; }

    [GlobalSetup]
    public void GlobalSetup()
    {
        _stringList = GenerateStringList(ListSize);
        _doubleList = GenerateDoubleList(ListSize);
    }

    [Benchmark(Baseline = true, Description = "ArrayPool.Rent (FIX)")]
    public int ArrayPoolApproach()
    {
        int count = 0;

        var stringArray = ArrayPool<string>.Shared.Rent(_stringList.Count);
        try
        {
            _stringList.CopyTo(stringArray);
            Array.Sort(stringArray, 0, _stringList.Count, StringComparer.Ordinal);
            for (int i = 0; i < _stringList.Count; i++)
            {
                count += stringArray[i].Length;
            }
        }
        finally
        {
            ArrayPool<string>.Shared.Return(stringArray);
        }

        var doubleArray = ArrayPool<double>.Shared.Rent(_doubleList.Count);
        try
        {
            _doubleList.CopyTo(doubleArray);
            Array.Sort(doubleArray, 0, _doubleList.Count);
            for (int i = 0; i < _doubleList.Count; i++)
            {
                count += doubleArray[i].ToString(CultureInfo.InvariantCulture).Length;
            }
        }
        finally
        {
            ArrayPool<double>.Shared.Return(doubleArray);
        }

        return count;
    }

    [Benchmark(Description = "new List<T>(copy)")]
    public int ListCopyApproach()
    {
        int count = 0;

        var sortedStringList = new List<string>(_stringList);
        sortedStringList.Sort(StringComparer.Ordinal);
        foreach (var item in sortedStringList)
        {
            count += item.Length;
        }

        var sortedDoubleList = new List<double>(_doubleList);
        sortedDoubleList.Sort();
        foreach (var item in sortedDoubleList)
        {
            count += item.ToString(CultureInfo.InvariantCulture).Length;
        }

        return count;
    }

    [Benchmark(Description = "LINQ OrderBy")]
    public int LinqOrderByApproach()
    {
        int count = 0;

        foreach (var item in _stringList.OrderBy(x => x, StringComparer.Ordinal))
        {
            count += item.Length;
        }

        foreach (var item in _doubleList.OrderBy(x => x))
        {
            count += item.ToString(CultureInfo.InvariantCulture).Length;
        }

        return count;
    }

    [Benchmark(Description = "Span (stackalloc doubles)")]
    public int SpanApproach()
    {
        int count = 0;

        var stringArray = ArrayPool<string>.Shared.Rent(_stringList.Count);
        try
        {
            _stringList.CopyTo(stringArray);
            Array.Sort(stringArray, 0, _stringList.Count, StringComparer.Ordinal);
            for (int i = 0; i < _stringList.Count; i++)
            {
                count += stringArray[i].Length;
            }
        }
        finally
        {
            ArrayPool<string>.Shared.Return(stringArray);
        }

        if (_doubleList.Count <= 256)
        {
            Span<double> doubleSpan = stackalloc double[_doubleList.Count];
            for (int i = 0; i < _doubleList.Count; i++)
            {
                doubleSpan[i] = _doubleList[i];
            }

            doubleSpan.Sort();
            foreach (var item in doubleSpan)
            {
                count += item.ToString(CultureInfo.InvariantCulture).Length;
            }
        }
        else
        {
            var doubleArray = ArrayPool<double>.Shared.Rent(_doubleList.Count);
            try
            {
                _doubleList.CopyTo(doubleArray);
                Array.Sort(doubleArray, 0, _doubleList.Count);
                for (int i = 0; i < _doubleList.Count; i++)
                {
                    count += doubleArray[i].ToString(CultureInfo.InvariantCulture).Length;
                }
            }
            finally
            {
                ArrayPool<double>.Shared.Return(doubleArray);
            }
        }

        return count;
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static List<string> GenerateStringList(int size)
    {
        var list = new List<string>(size);
        var random = new Random(42);
        var chars = "abcdefghijklmnopqrstuvwxyz";

        for (int i = 0; i < size; i++)
        {
            var value = new char[random.Next(5, 15)];
            for (int j = 0; j < value.Length; j++)
            {
                value[j] = chars[random.Next(chars.Length)];
            }

            list.Add(new string(value));
        }

        return list;
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static List<double> GenerateDoubleList(int size)
    {
        var list = new List<double>(size);
        var random = new Random(42);

        for (int i = 0; i < size; i++)
        {
            list.Add(random.NextDouble() * 1000);
        }

        return list;
    }

    private class Config : ManualConfig
    {
        public Config()
        {
            AddExporter(MarkdownExporter.GitHub);
        }
    }
}

public static class Program
{
    public static void Main(string[] args)
    {
        BenchmarkRunner.Run<ParameterSortingBenchmarks>(
            DefaultConfig.Instance
                .WithOption(ConfigOptions.JoinSummary, true)
                .WithOption(ConfigOptions.DisableOptimizationsValidator, true));
    }
}

}

</details>

## Description
<!--- Describe your changes in detail -->

## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open [issue][issues], please link to the issue here -->

## Testing
<!--- Please describe in detail how you tested your changes -->
<!--- Include details of your testing environment, and the tests you ran to -->
<!--- see how your change affects other areas of the code, etc. -->

## Breaking Changes Assessment

1. Identify all breaking changes including the following details:
    * What functionality was changed?
    * How will this impact customers?
    * Why does this need to be a breaking change and what are the most notable non-breaking alternatives?
    * Are best practices being followed?
    * How have you tested this breaking change?
2. Has a senior/+ engineer been assigned to review this PR?

## Screenshots (if appropriate)

## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Checklist
<!--- Go over all the following points, and put an `x` in all the boxes that apply -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
- [ ] My code follows the code style of this project
- [ ] My change requires a change to the documentation
- [ ] I have updated the documentation accordingly
- [ ] I have read the **README** document
- [ ] I have added tests to cover my changes
- [ ] All new and existing tests passed

## License
<!--- The SDK is released under the [Apache 2.0 license][license], so any code you submit will be released under that license -->
<!--- For substantial contributions, we may ask you to sign a [Contributor License Agreement (CLA)][cla] -->
<!--- Put an `x` in the below box if you confirm that this request can be released under the Apache 2 license -->
- [ ] I confirm that this pull request can be released under the Apache 2 license

[issues]: https://github.com/aws/aws-sdk-net/issues
[license]: http://aws.amazon.com/apache2.0/
[cla]: http://en.wikipedia.org/wiki/Contributor_License_Agreement

[danielmarbach]

Safer to clear?

[danielmarbach]

Even with clearing it is still reasonably fast

[danielmarbach]

Using the pool here seems safe since all generated enumerator paths need to dispose of the enumerator, which triggers the finally. If someone were to acquire the enumerator manually and not dispose of it, then the finally would not be called. Given this is a wrong usage of the enumerator API and the method is internal, I would not be concerned about this. Additionally, there is also this hint

Failure to return a rented buffer is not a fatal error. However, it may lead to decreased application performance, as the pool may need to create a new buffer to replace the lost one.

Which indicates it is not the end of the world. But again, if someone manually acquires the enumerator and doesn't follow the disposal guidelines, I guess that abuse of the internal method deserves to be punished with a small perf hit ;)

[danielmarbach]

@peterrsongg This is ready for review

[peterrsongg]

@peterrsongg This is ready for review

Just doing our due-diligence on this. Someone else on the team is actively looking into this and has this assigned to them as I got pulled into something else. But I will be the second reviewer

[muhammad-othman]

I checked the values of the parameters dictionary for the reproduction code provided in #1922 and none of them are StringListParameterValue or DoubleListParameterValue

All of them are StringParameterValue, which means this change wont affect the issue and we wont make any improvements for the memory allocation.
This change looks useful to me and it would improve the allocations for cases where there are ListParameterValues, but I wouldn't recommend merging it until we figure out the root cause of the issue as we might need to rework how GetParametersEnumerable work.
Maybe revert to using GetSortedParametersList for now? What do you think @danielmarbach ?

[danielmarbach]

Overall when I did the small improvements here in the past I deliberately left out any redesigns. The entire parameter valua collection being mutable would be one thing to look at besides others.

Judging by the benchmark I ran this seems still giving good allocation gains that benefit's all SDK clients that run through this path I'd say it is worth and safe to keep.

[muhammad-othman]

Hi @danielmarbach,
Sorry for the late review, I got distracted by other tasks. Still planning on getting back to the allocations xml issue but I wanted to focus on the json issue first as it got worse after we moved to v4.

[danielmarbach]

No worries. There is no rush

[Copilot]

Pull request overview

This PR fixes a mutation bug in ParameterCollection.GetParametersEnumerable() where calling the method would sort the original StringListParameterValue and DoubleListParameterValue lists in place, modifying the underlying data on every call. The fix uses ArrayPool<T>.Shared.Rent() to obtain temporary arrays for sorting, avoiding both the mutation bug and heap allocations.

Changes:

• Replaced in-place sorting of original lists with ArrayPool-backed temporary arrays for both StringListParameterValue and DoubleListParameterValue cases in GetParametersEnumerable()
• Added using System.Buffers import to support ArrayPool<T> usage

---

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

This PR modifies Core SDK code but does not include a DevConfig file, which is required per CONTRIBUTING.md for all code changes. Since this is a bug fix (preventing in-place mutation of the original lists), a DevConfig file with a "core" section and "type": "patch" should be added under generator/.DevConfigs/.

[Copilot]

The key motivation for this change is to fix a mutation bug where calling GetParametersEnumerable() (or GetSortedParametersList()) would sort the original lists in place. However, there are no tests verifying this non-mutation behavior. Consider adding a test that calls GetSortedParametersList() (or GetParametersAsString) on a ParameterCollection containing unsorted string/double lists, and then asserts that the original lists remain in their original unsorted order. The existing test in AWSSDKUtilsTests.cs already tests GetParametersAsString with sorted data, so a complementary test with unsorted data would also verify correct sorted output while confirming non-mutation.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[danielmarbach]

The commit will no longer be co-authored?

[muhammad-othman]

@danielmarbach I rewrote your commit using the same commands that I usually use for rewriting my commits which added me as the author, then it took me a while to figure out how to add you back as the author, didn't mean to take the credit for your change 😅

I'm currently running a dry run for this PR in our internal build system, will approve it when it passes.

[danielmarbach]

@muhammad-othman no worries. I'm not easily offended 😜 I just figured I'll ask. Happy coding!