feat(lib-storage): improve performance by reducing expensive buffer concat operations

[monken]

Issue #1841

Description of changes:

The for await syntax reads chunks from a readable stream and is in my opinion more concise and less error prone as NodeJS will take care of event handlers, exception handling, etc.

In addition, Buffer.concat will only be called once per chunk which greatly improves performance (uploading a 1GB file would take 23 seconds previously, down to 10 seconds with this patch). The use of Buffer.slice is intentional as it does not copy or create a new buffer. Instead, it's basically a view into the buffer it slices over.

I ran a few profiling runs over the code and I think there is more room for improvement. V2 of the SDK still seems to be ~20 percent faster. For example, v3 calls update from internal/crypto/hash.js twice as many times as v2.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[samsullivan]

I don't have to dig deeper (yet), but replacing this with my last build (that I tested #1841 with) gave me the following error:

Invalid client type

Happened with both S3 and S3Client objects from @aws-sdk/client-s3. My last build was on 3.0.1, though.

[monken]

@samsullivan yeah make sure you update everything to 3.3.0. I had the same issue...

[samsullivan]

Ah, should've been able to figure that out on my own. Looks like it's working now. Thanks!

I didn't have any more robust performance testing; however, this seems to be more thought out of a fix than my "bolt it on" approach in #1841 so I'm fine with this one being merged instead. Bummer that v2 SDK still had better performance here, but...the current v3 implementation is unusable.

[monken]

I have prepared another PR that will improve upload speeds further, even beating v2. It's a little more invasive by allowing passing Buffer arrays all the way down to the http-client and signer. I'll submit it if this PR gets merged.

[codecov-io]

Codecov Report

Merging #1955 (5b0abc7) into master (f2a47e8) will increase coverage by 0.54%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1955      +/-   ##
==========================================
+ Coverage   79.30%   79.85%   +0.54%     
==========================================
  Files         368      368              
  Lines       15132    15544     +412     
  Branches     3222     3367     +145     
==========================================
+ Hits        12001    12412     +411     
- Misses       3131     3132       +1     

Impacted Files	Coverage Δ
packages/util-waiter/src/utils/validate.ts	81.81% <0.00%> (-18.19%)	⬇️
packages/s3-request-presigner/src/getSignedUrl.ts	92.85% <0.00%> (-2.98%)	⬇️
packages/util-dynamodb/src/convertToAttr.ts	98.70% <0.00%> (-1.30%)	⬇️
packages/util-waiter/src/poller.ts	100.00% <0.00%> (ø)
packages/util-waiter/src/waiter.ts	100.00% <0.00%> (ø)
protocol_tests/aws-ec2/endpoints.ts	81.48% <0.00%> (ø)
protocol_tests/aws-json/endpoints.ts	81.48% <0.00%> (ø)
protocol_tests/aws-query/endpoints.ts	81.48% <0.00%> (ø)
packages/util-dynamodb/src/marshall.ts	100.00% <0.00%> (ø)
protocol_tests/aws-ec2/runtimeConfig.ts	100.00% <0.00%> (ø)
... and 156 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63ad215...5b0abc7. Read the comment docs.

[AllanZhengYP]

@monken

👍👍 Huge thanks to this optimization and the test case. It looks good to me!

The formating of the code is a little off. The Prettier should've formatted it but for some reason it doesn't seem the case. Can you try format the code?

[monken]

@AllanZhengYP for some reason the pre-commit hook didn't run. Should be fixed now.

[aws-sdk-js-automation]

AWS CodeBuild CI Report

• CodeBuild project: sdk-staging-test
• Commit ID: e685a21
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[alexforsyth]

This PR #2039 to rewrite Upload should incorporate these ideas. @monken please let me know if something is missing there.

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.