Improve v4 Signer.SignHTTP & Signer.PresignHTTP performance #2956
Conversation
|
I'm not sure I'm sold on the microsecond/byte-level performance boost for what I think is a pretty drastic hit in terms of readability. Will let others chime in. |
|
|
| t := signingTime.ShortTimeFormat() | ||
|
|
||
| var sb strings.Builder | ||
| sb.Grow(len(t) + 1 + len(region) + 1 + len(service) + 1 + len(suffix)) |
There was a problem hiding this comment.
In fact, is this actually an improvement? strings.Join uses strings.Builder internally.
There was a problem hiding this comment.
It's a minor improvement of around -0.45% in the Benchmark.
I agree that it's a little less readable but I also expect this code to not change any time soon and in our solution we sign a lot for requests and every little improvement helps.
│ 1.sha-62c09690f2.txt │ 2.sha-ccdf28e449.txt │
│ sec/op │ sec/op vs base │
PresignRequest-16 10.49µ ± 3% 10.45µ ± 3% ~ (p=0.837 n=10)
SignRequest-16 5.564µ ± 2% 5.534µ ± 2% ~ (p=0.210 n=10)
geomean 7.641µ 7.606µ -0.45%
However, I'm fine removing the related commit from the PR if it's a deal breaker.
There was a problem hiding this comment.
FYI not using strings.Join has some precedence in lookupKey and buildAuthorizationHeader
There was a problem hiding this comment.
Yes, that's older code and it's something we very much want to get away from. I made a point to do away with all of it in the rewrite in the aws-http-auth module.
|
Note that if you're using this code outside of the SDK to sign a lot of requests manually, you probably instead want https://pkg.go.dev/github.com/aws/smithy-go/aws-http-auth. That gives you sigv4 and sigv4a without an SDK dependency(although it doesn't have presigning yet). |
|
Thanks again for submitting this pull request. We've discussed this as a team and have decided to close this PR, as we prefer the readability of the existing implementation. The changes apart from the removal of strings.Join do not appear to convey a significant performance boost. |
|
Hey @lucix-aws , That's to bad. I would have hoped that at least P.S. use sha256.Sum256 to reduce allocations bench stats are below. |
Good day,
This PR includes the following (minor) performance improvements when using Signer.SignHTTP & Signer.PresignHTTP.
use sha256.Sum256 to reduce allocations
avoid []string allocation
avoid unicode.AppendRune by using WriteByte
allocate http.Header
These changes result in the following benchstats results. (
go test -bench=. -benchmem -count=10 -run=^$ ./aws/signer/v4/was used to generate the reports.)Thank you for having a look at this PR and please let me know if there is any way I can improve it.
Have a great day!