Optimize UrlEncode in Utils #3307

danielmarbach · 2024-04-28T18:59:22Z

Description

Optimizes UrlEncode for better memory efficiency

Motivation and Context


BenchmarkDotNet v0.13.12, macOS Sonoma 14.4.1 (23E224) [Darwin 23.4.0]
Apple M3 Max, 1 CPU, 14 logical and 14 physical cores
.NET SDK 8.0.202
  [Host]     : .NET 8.0.3 (8.0.324.11423), Arm64 RyuJIT AdvSIMD
  DefaultJob : .NET 8.0.3 (8.0.324.11423), Arm64 RyuJIT AdvSIMD

https://github.com/danielmarbach/aws-sdk-net-benchmarks/blob/main/UrlEncode.cs

Testing

Added tests to verify the code still does the same as before.

Screenshots (if appropriate)

Types of changes

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

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

I confirm that this pull request can be released under the Apache 2 license

sdk/src/Core/AWSSDK.Core.NetStandard.csproj

sdk/src/Core/Amazon.Runtime/Internal/Util/_netstandard/EncodingExtensions.cs

sdk/src/Core/Amazon.Runtime/Internal/_bcl+netstandard/SkipLocalsInitAttribute.cs

danielmarbach · 2024-04-28T19:08:02Z

sdk/src/Core/Amazon.Util/AWSSDKUtils.cs

@@ -75,12 +77,6 @@ public static partial class AWSSDKUtils
        // Default value of progress update interval for streaming is 100KB.
        public const long DefaultProgressUpdateInterval = 102400;

-        internal static Dictionary<int, string> RFCEncodingSchemes = new Dictionary<int, string>


I did check the entire SDK and couldn't find another reference so I got rid of it. Is it possible to entirely get rid of the different RFC implementations? If that is possible we could potentially even get rid of the custom code and replace it with the System.Net implementation?

https://learn.microsoft.com/en-us/dotnet/api/system.net.webutility.urlencode?view=net-8.0

sdk/test/NetStandard/UnitTests/AWSSDK.UnitTests.Custom.NetStandard.csproj

sdk/test/NetStandard/UnitTests/Core/AWSSDKUtilsTests.cs

danielmarbach · 2024-04-28T19:16:53Z

sdk/src/Core/Amazon.Util/AWSSDKUtils.cs

+                if (!TryGetRFCEncodingSchemes(rfcNumber, out var validUrlCharacters))
+                    validUrlCharacters = ValidUrlCharacters;
+
+                var unreservedChars = string.Concat(validUrlCharacters, path ? ValidPathCharacters : "");


Given the encoding schemes are only used here the string concat could also go by already have the concatenated string available as costs and returning them by TryGetRFCEncodingSchemes

sdk/src/Core/Amazon.Util/AWSSDKUtils.cs

danielmarbach · 2024-04-29T11:45:52Z

@normj @dscpinheiro I know this is still draft but would you mind having a look at it at some point? I have added a bunch of inline comments that might influence this PR.

danielmarbach · 2024-05-15T18:09:50Z

Ping @normj

sdk/src/Core/AWSSDK.Core.NetStandard.csproj

sdk/src/Core/Amazon.Runtime/Internal/_bcl+netstandard/SkipLocalsInitAttribute.cs

sdk/src/Core/Amazon.Util/AWSSDKUtils.cs

sdk/test/NetStandard/UnitTests/Core/AWSSDKUtilsTests.cs

sdk/src/Core/Amazon.Runtime/Internal/Util/_netstandard/EncodingExtensions.cs

danielmarbach · 2024-05-30T16:50:59Z

I raised #3326 because the constants are a bit messy and need an overhaul. I did my best to get things into compilable state for now.

danielmarbach · 2024-06-19T21:29:59Z

Ok I'll do a rebase

danielmarbach · 2024-06-20T18:40:52Z

@normj rebased

normj

Looks good. I'll get somebody else on the team to do a second pass.

danielmarbach · 2024-07-01T09:09:11Z

Rebased to solve the conflict

danielmarbach · 2024-07-02T20:13:19Z

Would be nice if this could be merged since I have a bunch of other PRs that introduce more tests to the test file that probably require a rebase then ;)

boblodgett · 2024-07-02T21:40:09Z

Would be nice if this could be merged since I have a bunch of other PRs that introduce more tests to the test file that probably require a rebase then ;)

Taking a look / Dry-running the changes.

boblodgett · 2024-07-02T22:42:55Z

I don't visually see issues in the PR. However, this didn't build in our build system because a change is required to support the /unsafe switch. I will make the change so we can ensure this PR will build and test correctly before I approve the PR.

sdk/src/Core/AWSSDK.Core.NetStandard.csproj

boblodgett

#3307 (comment)

sdk/test/NetStandard/UnitTests/AWSSDK.UnitTests.Custom.NetStandard.csproj

normj

Approve with assumption @boblodgett gets a successful build through the internal system.

dscpinheiro · 2024-07-04T16:43:41Z

@danielmarbach I approved and merged #3363 and #3365, but we saw integration test failures when running this PR on our build system (they were all related to this CloudFormation section: https://github.com/aws/aws-sdk-net/blob/3.7.844.0/sdk/test/NetStandard/IntegrationTests/IntegrationTests/CloudFormation.cs#L20).

Essentially that value is passed to UrlEncode (via GetParametersAsString), but AWSSDKUtils.UrlEncode(TEMPLATE_TEXT, false) in your branch is not returning the same value as v4-development.

…that can be executed

danielmarbach · 2024-07-04T16:54:52Z

@dscpinheiro Ok I'll check

danielmarbach · 2024-07-05T14:56:01Z

@dscpinheiro I'm approaching my leave period so it might take a bit longer for me to respond. Apologies in advance

danielmarbach · 2024-07-06T13:53:19Z

@dscpinheiro I fixed the problem. It was between the chair and the keyboard 👯

danielmarbach · 2024-07-06T13:56:10Z

sdk/src/Core/Amazon.Util/AWSSDKUtils.cs

-                    encoded.Append(ToUpperHex(loNibble));
+                    if (unreservedChars.IndexOf((char)symbol) != -1)
+                    {
+                        dataBuffer[index++] = symbol;


Technically we could potentially remove bound checks here by doing something like

Unsafe.Add(ref MemoryMarshal.GetReference(dataBuffer), index++) = symbol;

but I decided to not go down that path since it increases the complexity of the code

danielmarbach · 2024-07-06T13:57:29Z

Give it another run through the benchmark

Still very good allocation reduction and the throughput hit is still in very acceptable range

boblodgett · 2024-07-08T19:06:29Z

All the new changes are being run through the backend build system now.

boblodgett · 2024-07-08T21:04:35Z

One test failure for netcoreapp3.1 target running tests.

error : Test failure in XUnitTestRunner: UnitTests.NetStandard.Core.AWSSDKUtilsTests.UrlEncodeWithoutPath(input: "{\r\n\t\"AWSTemplateFormatVersion\" : \"2010-09-09"..., expected: "%7B%0A%09%22AWSTemplateFormatVersion%22%20%3A%20%2"...)

Because of how our system runs I do not know if other targets also have problems yet.

danielmarbach · 2024-07-09T06:31:10Z

I ran against net8 and as far as I recall net4.x. I'm not installing a runtime that is out of support for almost two years and hasn't received security updates since them. Sorry.

So if you can provide me the entire test output with some information that makes it possible for me to guess what's going on I'm happy to give it another look.

Otherwise this PR has also edit rights. Feel free to push updates.

boblodgett · 2024-07-09T20:01:58Z

I ran against net8 and as far as I recall net4.x. I'm not installing a runtime that is out of support for almost two years and hasn't received security updates since them. Sorry.

So if you can provide me the entire test output with some information that makes it possible for me to guess what's going on I'm happy to give it another look.

Otherwise this PR has also edit rights. Feel free to push updates.

Thank you for working with us on this PR while we shake out the test issues. I ran the test in Visual Studio locally and it also fails in .NET 8.

The project containing the failed test is: .\aws-sdk-net\sdk\test\NetStandard\UnitTests\AWSSDK.UnitTests.Custom.NetStandard.csproj

The file containing the failed test within that project is: .\aws-sdk-net\sdk\test\NetStandard\UnitTests\Core\AWSSDKUtilsTests.cs

The test is: public void UrlEncodeWithoutPath(string input, string expected) which has three [InlineData] test cases. The case that is failing specifically is the third one which uses the TemplateText.

Here is a link to the test case: https://github.com/danielmarbach/aws-sdk-net/blob/f5fe55e95b98d07bb7cd2c2a8e8f4451f0b1b819/sdk/test/NetStandard/UnitTests/Core/AWSSDKUtilsTests.cs#L70

TemplateText can be found here: https://github.com/danielmarbach/aws-sdk-net/blob/f5fe55e95b98d07bb7cd2c2a8e8f4451f0b1b819/sdk/test/NetStandard/UnitTests/Core/AWSSDKUtilsTests.cs#L78

All other tests in this project pass.

boblodgett · 2024-07-09T20:12:44Z

From the look of it \r (0x0D) characters are being injected and encoded when the test case hits a \n (0x0A):

boblodgett · 2024-07-09T20:21:06Z

sdk/test/NetStandard/UnitTests/Core/AWSSDKUtilsTests.cs

+        [Theory]
+        [InlineData("value, with special chars!", "value%2C%20with%20special%20chars%21")]
+        [InlineData("value, with special chars and path {/+:}", "value%2C%20with%20special%20chars%20and%20path%20%7B%2F%2B%3A%7D")]
+        [InlineData(TemplateText, "%7B%0A%09%22AWSTemplateFormatVersion%22%20%3A%20%222010-09-09%22%2C%0A%0A%09%22Description%22%20%3A%20%22This%20is%20a%20sample%20template%22%2C%0A%0A%09%22Parameters%22%20%3A%20%7B%0A%09%09%22TopicName%22%20%3A%20%7B%0A%09%09%20%20%20%20%22Type%22%20%3A%20%22String%22%2C%0A%20%20%20%20%20%20%20%20%20%20%20%20%22Default%22%20%3A%20%22TheTopic%22%2C%0A%20%20%20%20%20%20%20%20%20%20%20%20%22Description%22%20%3A%20%22A%20topic.%22%0A%09%09%7D%2C%0A%20%20%20%20%20%20%20%20%22QueueName%22%20%3A%20%7B%0A%20%20%20%20%20%20%20%20%20%20%20%20%22Type%22%20%3A%20%22String%22%2C%0A%20%20%20%20%20%20%20%20%20%20%20%20%22Default%22%20%3A%20%22TheQueue%22%2C%0A%20%20%20%20%20%20%20%20%20%20%20%20%22Description%22%20%3A%20%22A%20queue.%22%0A%20%20%20%20%20%20%20%20%7D%0A%09%7D%2C%0A%0A%09%22Resources%22%20%3A%20%7B%0A%0A%09%09%22TheQueue%22%20%3A%20%7B%0A%09%09%20%20%20%20%22Type%22%20%3A%20%22AWS%3A%3ASQS%3A%3AQueue%22%2C%0A%09%09%20%20%20%20%22Properties%22%20%3A%20%7B%0A%09%09%09%09%22QueueName%22%20%3A%20%7B%20%22Ref%22%20%3A%20%22QueueName%22%20%7D%0A%09%09%20%20%20%20%7D%0A%09%09%7D%2C%0A%0A%20%20%20%20%20%20%20%20%22TheTopic%22%20%3A%20%7B%0A%20%20%20%20%20%20%20%20%20%20%20%20%22Type%22%20%3A%20%22AWS%3A%3ASNS%3A%3ATopic%22%2C%0A%20%20%20%20%20%20%20%20%20%20%20%20%22Properties%22%20%3A%20%7B%0A%09%09%09%09%22TopicName%22%20%3A%20%7B%20%22Ref%22%20%3A%20%22TopicName%22%20%7D%2C%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%22Subscription%22%20%3A%20%5B%0A%09%09%09%09%09%7B%22Protocol%22%20%3A%20%22sqs%22%2C%20%22Endpoint%22%20%3A%20%7B%22Fn%3A%3AGetAtt%22%20%3A%20%5B%20%22TheQueue%22%2C%20%22Arn%22%5D%7D%7D%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%5D%0A%20%20%20%20%20%20%20%20%20%20%20%20%7D%0A%20%20%20%20%20%20%20%20%7D%0A%09%7D%2C%0A%0A%09%22Outputs%22%20%3A%20%7B%0A%09%09%22TopicARN%22%20%3A%20%7B%0A%09%09%20%20%20%20%22Value%22%20%3A%20%7B%20%22Ref%22%20%3A%20%22TheTopic%22%20%7D%0A%09%09%7D%2C%0A%20%20%20%20%20%20%20%20%22QueueURL%22%20%3A%20%7B%0A%20%20%20%20%20%20%20%20%20%20%20%20%22Value%22%20%3A%20%7B%20%22Ref%22%20%3A%20%22TheQueue%22%20%7D%0A%20%20%20%20%20%20%20%20%7D%0A%09%7D%0A%7D%0A")]


This is the test case that is failing which I didn't remember was a new test case until reviewing the changes again. This likely passes on a Linux based machine but will fail on Windows because of how some of the methods automatically translate \n to \r\n in windows.

Ah yeah the input needs to be normalized. I can only do that after my vacation. Feel free to push it if you have time.

Does it happen on checkout on the build agent?

danielmarbach · 2024-07-09T20:47:56Z

Honestly looking at this again on my phone I'd argue the only thing that would have been needed to cover the bug in 618ef3e would have been a long enough string. Fighting line endings and going back and forth on this PR for trying to fit the template text into here seems a bit pointless and unnecessarily holding this up. How about I remove this test case for now?

I can send an update PR on after vacation (pinky finger promise)

sdk/test/NetStandard/UnitTests/Core/AWSSDKUtilsTests.cs

danielmarbach · 2024-07-09T21:08:51Z

@boblodgett based on the above comment I removed the template test for now. Having some tests in place for the method where there previously have been none is already a good start and the template was only a temporary thing anyway which helped me discover the buffer length problem. Ideally, there would be another test with a longer input but I cannot do this right now since I'm not around my laptop or workstation for a while

danielmarbach · 2024-07-09T21:11:13Z

And thanks for your support here and sorry for me being less helpful this time

boblodgett · 2024-07-10T16:50:41Z

Backend build was successful. Approved and am going to merge this in.

danielmarbach mentioned this pull request Apr 28, 2024

Optimize AWSSDKUtils.ToHex() for speed and memory #3293

Merged

10 tasks

danielmarbach commented Apr 28, 2024

View reviewed changes

sdk/src/Core/Amazon.Util/AWSSDKUtils.cs Show resolved Hide resolved

dscpinheiro added the v4 label Apr 29, 2024

normj requested changes May 26, 2024

View reviewed changes

danielmarbach marked this pull request as ready for review May 30, 2024 16:13

danielmarbach force-pushed the UrlEncode branch from bee9c12 to 6396310 Compare May 30, 2024 16:46

danielmarbach force-pushed the UrlEncode branch from 6396310 to 80d1ff4 Compare June 20, 2024 18:33

normj approved these changes Jun 26, 2024

View reviewed changes

dscpinheiro requested a review from boblodgett June 26, 2024 17:36

danielmarbach force-pushed the UrlEncode branch from 80d1ff4 to d2dc42b Compare July 1, 2024 09:08

normj requested changes Jul 2, 2024

View reviewed changes

sdk/src/Core/AWSSDK.Core.NetStandard.csproj Show resolved Hide resolved

boblodgett requested changes Jul 3, 2024

View reviewed changes

danielmarbach commented Jul 3, 2024

View reviewed changes

sdk/test/NetStandard/UnitTests/AWSSDK.UnitTests.Custom.NetStandard.csproj Outdated Show resolved Hide resolved

danielmarbach requested review from boblodgett and normj July 3, 2024 20:02

normj approved these changes Jul 4, 2024

View reviewed changes

danielmarbach added 2 commits July 4, 2024 18:50

Add unit tests

5ae6c49

Remove netstandard from unit tests since netstandard is not a target …

a7d48d4

…that can be executed

danielmarbach and others added 4 commits July 4, 2024 18:51

Fix ifdef

0003d63

Unsafe for Netframework

a3c4646

Move extensions to BCL and netstandard folder

721ac7d

Bring back netstandard

0b34f45

danielmarbach force-pushed the UrlEncode branch from e0bd211 to 0b34f45 Compare July 4, 2024 16:52

danielmarbach added 3 commits July 6, 2024 14:58

Failing test for template

c4d330f

Well that was an easy fix

618ef3e

Span indexof

f5fe55e

danielmarbach commented Jul 6, 2024

View reviewed changes

boblodgett requested changes Jul 9, 2024

View reviewed changes

danielmarbach commented Jul 9, 2024

View reviewed changes

sdk/test/NetStandard/UnitTests/Core/AWSSDKUtilsTests.cs Outdated Show resolved Hide resolved

Remove extra test case for now

4df6816

boblodgett approved these changes Jul 10, 2024

View reviewed changes

boblodgett merged commit 7bd0df1 into aws:v4-development Jul 10, 2024

boblodgett mentioned this pull request Jul 10, 2024

V4 Development Tracker #3362

Open

danielmarbach deleted the UrlEncode branch July 10, 2024 20:08

peterrsongg pushed a commit that referenced this pull request Jul 29, 2024

Optimize UrlEncode in Utils (#3307)

6fcfd73

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize UrlEncode in Utils #3307

Optimize UrlEncode in Utils #3307

danielmarbach commented Apr 28, 2024 •

edited

Loading

danielmarbach Apr 28, 2024

danielmarbach Apr 28, 2024

danielmarbach commented Apr 29, 2024

danielmarbach commented May 15, 2024

danielmarbach commented May 30, 2024

danielmarbach commented Jun 19, 2024

danielmarbach commented Jun 20, 2024

normj left a comment

danielmarbach commented Jul 1, 2024

danielmarbach commented Jul 2, 2024

boblodgett commented Jul 2, 2024 •

edited

Loading

boblodgett commented Jul 2, 2024

boblodgett left a comment

normj left a comment

dscpinheiro commented Jul 4, 2024

danielmarbach commented Jul 4, 2024

danielmarbach commented Jul 5, 2024

danielmarbach commented Jul 6, 2024

danielmarbach Jul 6, 2024

danielmarbach commented Jul 6, 2024

boblodgett commented Jul 8, 2024

boblodgett commented Jul 8, 2024 •

edited

Loading

danielmarbach commented Jul 9, 2024

boblodgett commented Jul 9, 2024

boblodgett commented Jul 9, 2024

boblodgett Jul 9, 2024

danielmarbach Jul 9, 2024

danielmarbach Jul 9, 2024

danielmarbach commented Jul 9, 2024 •

edited

Loading

danielmarbach commented Jul 9, 2024

danielmarbach commented Jul 9, 2024

boblodgett commented Jul 10, 2024

Optimize UrlEncode in Utils #3307

Optimize UrlEncode in Utils #3307

Conversation

danielmarbach commented Apr 28, 2024 • edited Loading

Description

Motivation and Context

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

danielmarbach Apr 28, 2024

Choose a reason for hiding this comment

danielmarbach Apr 28, 2024

Choose a reason for hiding this comment

danielmarbach commented Apr 29, 2024

danielmarbach commented May 15, 2024

danielmarbach commented May 30, 2024

danielmarbach commented Jun 19, 2024

danielmarbach commented Jun 20, 2024

normj left a comment

Choose a reason for hiding this comment

danielmarbach commented Jul 1, 2024

danielmarbach commented Jul 2, 2024

boblodgett commented Jul 2, 2024 • edited Loading

boblodgett commented Jul 2, 2024

boblodgett left a comment

Choose a reason for hiding this comment

normj left a comment

Choose a reason for hiding this comment

dscpinheiro commented Jul 4, 2024

danielmarbach commented Jul 4, 2024

danielmarbach commented Jul 5, 2024

danielmarbach commented Jul 6, 2024

danielmarbach Jul 6, 2024

Choose a reason for hiding this comment

danielmarbach commented Jul 6, 2024

boblodgett commented Jul 8, 2024

boblodgett commented Jul 8, 2024 • edited Loading

danielmarbach commented Jul 9, 2024

boblodgett commented Jul 9, 2024

boblodgett commented Jul 9, 2024

boblodgett Jul 9, 2024

Choose a reason for hiding this comment

danielmarbach Jul 9, 2024

Choose a reason for hiding this comment

danielmarbach Jul 9, 2024

Choose a reason for hiding this comment

danielmarbach commented Jul 9, 2024 • edited Loading

danielmarbach commented Jul 9, 2024

danielmarbach commented Jul 9, 2024

boblodgett commented Jul 10, 2024

danielmarbach commented Apr 28, 2024 •

edited

Loading

boblodgett commented Jul 2, 2024 •

edited

Loading

boblodgett commented Jul 8, 2024 •

edited

Loading

danielmarbach commented Jul 9, 2024 •

edited

Loading