S3 Async GetObject `toBytes`: 3x memory reduction, less array-copying

[rtyley]

Motivation and Context

This fixes https://github.com/aws/aws-sdk-java-v2/issues/4392 (duplicate of https://github.com/aws/aws-sdk-java-v2/issues/3193): loading an S3 object into memory using AsyncResponseTransformer.toBytes() has these problems:

• Memory: Excessive peak requirements - peak memory requirement is 4x the actual size of the S3 object - loading a 258MB object into memory takes 1070MB of RAM.
• CPU: Unnecessary byte[] allocations, copies & GC

Modifications

Change A : Improve byte[] storage by using the known Content Length

When reading an S3 GetObject response, we have the S3 Content Length available to us, meaning we can create a AsyncS3ResponseTransformer which has these enhancements:

• A1 Use the Content Length to initialise the ByteArrayOutputStream with a byte array of the right size.
  • Array copies removed : $ceil(log_2 (N/32))$ (eg a 40MB object takes 21 array copies - graph)
  • Memory: 2N → N
• A2 Use a simple fixed-size byte array in preference to a ByteArrayOutputStream
  • Storing a chunk of X bytes (where X <= N - ie may equal N)
    • Array copies removed: 1 per chunk
    • Memory: X → 0
  • Retrieving the final N bytes
    • Array copies removed: 1
    • Memory: 2N → N

Change B : Use ResponseBytes.fromByteArrayUnsafe()

• Avoid performing a byte array copy to create the ResponseBytes instance
  • Array copies removed: 1
  • Memory: 2N → N

Testing

In https://github.com/rtyley/aws-sdk-async-response-bytes I've added automated memory testing of the various enhancements above. These are the resulting memory requirements, in MB, for all possible permutations of the fixes:

	Exclude A	A1	A1+A2
Exclude B	1070	934	644
Include B	787	657	357

From the results, it's clear that we need all 3 changes (A1, A2, & B) in order to get the lowest memory usage.

Screenshots (if appropriate)

Types of changes

• [x] Bug fix (non-breaking change which fixes https://github.com/aws/aws-sdk-java-v2/issues/4392)
• [x] New feature (non-breaking change which introduces AsyncS3ResponseTransformer, optimised for handling GetObjectResponse)

Checklist

• [x] I have read the CONTRIBUTING document
• [ ] Local run of mvn install succeeds
• [x] My code follows the code style of this project
• [x] My change requires a change to the Javadoc documentation
• [x] I have updated the Javadoc documentation accordingly
• [ ] I have added tests to cover my changes
• [ ] All new and existing tests passed
• [ ] I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
• [ ] My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

• [x] I confirm that this pull request can be released under the Apache 2 license

[debora-ito]

A quick update on this PR: we want to improve the memory usage, but we're not sure that a new AsyncS3ResponseTransformer is the right solution. At this moment, we don't know what the right solution looks like, we have a task in our backlog to design a solution.

I'll move this PR to a draft state to keep the feature request in our radar. We might do the change ourselves later on, depending on the solution complexity.

[debora-ito]

@rtyley I'm closing this PR as stale, we didn't have the chance to go back to this issue. We still have the task in our backlog to research how to improve memory usage.

[rtyley]

@rtyley I'm closing this PR as stale, we didn't have the chance to go back to this issue. We still have the task in our backlog to research how to improve memory usage.

Oh, that's disappointing - I did try hard to make this PR clear and show that it would fix the issue. If you'd like individual explanations of the 3 changes (A1, A2 & B), they're available in these 3 PRs:

• https://github.com/rtyley/aws-sdk-async-response-bytes/pull/8
• https://github.com/rtyley/aws-sdk-async-response-bytes/pull/10
• https://github.com/rtyley/aws-sdk-async-response-bytes/pull/12 - includes a justification of why it's ok to call ResponseBytes.fromByteArrayUnsafe()

we want to improve the memory usage, but we're not sure that a new AsyncS3ResponseTransformer is the right solution. At this moment, we don't know what the right solution looks like [...] We might do the change ourselves later on, depending on the solution complexity.

Could you share the team's concerns around solving the problem by adding an AsyncS3ResponseTransformer? If I knew the constraints you're operating under I might be able to find a better place to make the fix.

[chibenwa]

Same.

That's not understandable from my standpoint that a driver used in performance critical applications do not even take the basic performance enhancements proposals that outsiders with simple tools like flame graphs reports and fix.

[javafanboy]

Fully agree - this needs to be fixed. I need a way to stream large S3 objects with predictable and minimal CPU and in particular memory overhead and thought this method would be the right one for this - finding that it in fact is not was very disappointing...