Conversation
|
@normj I would appreciate your input ;) |
sdk/src/Core/Amazon.Runtime/Internal/Util/ValueStringBuilder.cs
Outdated
Show resolved
Hide resolved
…ed in the majority of the cases and benefits from .NET improvements
6d9d80a to
984f278
Compare
|
@normj I have to update the description of the PR but I'm running out of my fun coding time allotted budget for now. I have added some benchmark results that should speak though themselves. Of course ideally it would be better if we could bring the ValueStringBuilder ref struct into the helper methods but since I did not get feedback on the usage of those protected static methods I went ahead and used an approach that doesn't require signature changes which I think is in your favor anyway for now. @Dreamescaper I also addressed one of your feedbacks |
| } | ||
|
|
||
| return builder.ToString(); | ||
| return string.Join(";", sortedHeaders.Select(x => x.Key.ToLowerInvariant())); |
peterrsongg
left a comment
There was a problem hiding this comment.
I ran the core and custom unit tests, mainly was concerned about the existing Signer tests which pass. Change looks good to me.

This PR heavily optimizes the HTTP header path on the AWS4Signer to reduce the number of allocations.
Description
See screenshots
Motivation and Context
Making sure the request path is less allocation heavy will drastically improve the throughput of the AWS SDK.
Testing
Screenshots (if appropriate)
CanonicalHeaderNames
https://github.com/danielmarbach/aws-sdk-net-benchmarks/blob/main/CanonicalHeaderNames.cs
CanonicalHeaders
https://github.com/danielmarbach/aws-sdk-net-benchmarks/blob/main/CanonicalHeaderNames.cs
CompressSpace
https://github.com/danielmarbach/aws-sdk-net-benchmarks/blob/main/CompressSpace.cs
CanonicalHeaders with CompressSpace
https://github.com/danielmarbach/aws-sdk-net-benchmarks/blob/main/CanonicalHeaderNames.cs
Attention this was just a short run because things take ages otherwise
Types of changes
Checklist
License