Skip to content

Optimize sorting by leveraging ArrayPool for reduced allocations in ParameterCollection. #4331

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.

Sign up for GitHub

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

Merged
muhammad-othman merged 5 commits into from
Mar 24, 2026
Merged

Optimize sorting by leveraging ArrayPool for reduced allocations in ParameterCollection. #4331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
af0e476
Optimize sorting by leveraging ArrayPool for reduced allocations in P…
danielmarbach Mar 16, 2026
7f37c49
Add additional test to ensure original list in string parameterlistva…
peterrsongg Mar 19, 2026
cab16ad
Apply suggestion from @peterrsongg
peterrsongg Mar 24, 2026
51fbccc
Apply suggestion from @peterrsongg
peterrsongg Mar 24, 2026
ccccd7e
Apply suggestion from @peterrsongg
peterrsongg Mar 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions generator/.DevConfigs/54585c37-96bf-45a0-92d2-ad77a62e718d.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@

{
"core": {
"changeLogMessages": [
"Optimize ParameterCollection sorting by leveraging ArrayPool for reduced allocations. "
],
"type": "patch",
"updateMinimum": false
}
}
43 changes: 33 additions & 10 deletions sdk/src/Core/Amazon.Runtime/ParameterCollection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License").
Expand All @@ -13,6 +13,7 @@
* permissions and limitations under the License.
*/
using System;
using System.Buffers;
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 generated this review using guidance from repository custom instructions.
using System.Collections.Generic;
using System.Globalization;
using System.IO;
Expand Down Expand Up @@ -90,21 +91,43 @@ internal IEnumerable<KeyValuePair<string, string>> GetParametersEnumerable()
yield return new KeyValuePair<string, string>(name, stringParameterValue.Value);
break;
case StringListParameterValue stringListParameterValue:
var sortedStringListParameterValue = stringListParameterValue.Value;
sortedStringListParameterValue.Sort(StringComparer.Ordinal);
foreach (var listValue in sortedStringListParameterValue)
yield return new KeyValuePair<string, string>(name, listValue);
{
Copy link
Copy Markdown
Member

@muhammad-othman muhammad-othman Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Image

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 ?

Copy link
Copy Markdown
Contributor Author

@danielmarbach danielmarbach Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

var stringList = stringListParameterValue.Value;
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++)
yield return new KeyValuePair<string, string>(name, stringArray[i]);
}
finally
{
ArrayPool<string>.Shared.Return(stringArray);
Copy link
Copy Markdown
Contributor Author

@danielmarbach danielmarbach Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Safer to clear?

Copy link
Copy Markdown
Contributor Author

@danielmarbach danielmarbach Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even with clearing it is still reasonably fast

image

}
Comment on lines +94 to +107
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 generated this review using guidance from repository custom instructions.
break;
}
case DoubleListParameterValue doubleListParameterValue:
var sortedDoubleListParameterValue = doubleListParameterValue.Value;
sortedDoubleListParameterValue.Sort();
foreach (var listValue in sortedDoubleListParameterValue)
yield return new KeyValuePair<string, string>(name, listValue.ToString(CultureInfo.InvariantCulture));
{
var doubleList = doubleListParameterValue.Value;
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++)
yield return new KeyValuePair<string, string>(name, doubleArray[i].ToString(CultureInfo.InvariantCulture));
}
finally
{
ArrayPool<double>.Shared.Return(doubleArray);
}
break;
}
default:
throw new AmazonClientException("Unsupported parameter value type '" + value.GetType().FullName + "'");
}
}
}
}
}
}
25 changes: 25 additions & 0 deletions sdk/test/NetStandard/UnitTests/Core/AWSSDKUtilsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,31 @@ public void GetParametersAsString()
Assert.Equal("key1=value1&key2=value2&key3=value3&key3=value4&key4=1.1&key4=2.1", parametersString);
}

[Fact]
public void GetParametersAsString_ProducesCorrectlySortedOutput()
{
var parameters = new ParameterCollection
{
{ "key3", "value3" },
{ "key1", "value1" },
{ "key2", "value2" },
};

var result = AWSSDKUtils.GetParametersAsString(parameters);
Assert.Equal("key1=value1&key2=value2&key3=value3", result);
}
Comment thread
peterrsongg marked this conversation as resolved.
[Fact]
public void GetParametersAsStringWithStringListParameterValueCorrectlySortsOutput()
Copy link
Copy Markdown
Member

@muhammad-othman muhammad-othman Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this test because it’s too tied to the current implementation and doesn’t check for the intended behavior.
Right now, the implementation doesn’t mutate the list, but if we ever find a more efficient approach that does, we should use it. Adding this test, though, makes it seem like we’re against mutating the list, which I don’t think is the case.

{
var parameters = new ParameterCollection
{
{"key4", new List<string> { "value5", "value4" } },
{"key1", new List<string>{"value1", "value2"} }
};
var result = AWSSDKUtils.GetParametersAsString(parameters);
Assert.Equal("key1=value1&key1=value2&key4=value4&key4=value5", result);
}

[Fact]
public void ToHexUppercase()
{
Expand Down
Loading