Refactor s3-benchmarks and include v1 transfermanager tests #2575

zoewangg · 2021-07-03T00:20:26Z

Description

Refactor s3-benchmarks and include v1 transfermanager tests

Motivation and Context

Testing

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Checklist

License

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

sonarqubecloud · 2021-07-07T20:47:28Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

3.2% Coverage
3.5% Duplication

zoewangg · 2021-07-07T23:26:45Z

...nchmarks/src/main/java/software/amazon/awssdk/s3benchmarks/BaseTransferManagerBenchmark.java

-        transferManager = S3TransferManager.builder()
-                                           .s3ClientConfiguration(b -> b.targetThroughputInGbps(config.targetThroughput())
-                                           .minimumPartSizeInBytes(partSizeInMb))
-                                           .build();


I removed S3TransferManager and used the internal S3CrtClient for testing because S3TransferManager doesn't allow customizing S3 client , and we can't reuse the same S3 crt client.

Bennett-Lynch · 2021-07-08T00:24:06Z

...nchmarks/src/main/java/software/amazon/awssdk/s3benchmarks/BaseTransferManagerBenchmark.java

    }

    private void warmUp() throws InterruptedException {
        logger.info(() -> "Starting to warm up");

-        for (int i = 0; i < WARMUP_ITERATIONS; i++) {
+        for (int i = 0; i < PRE_WARMUP_ITERATIONS; i++) {


"warmup" or "pre-warmup"? The latter seems to imply:

pre-warmup

warmup

actual run

This is actually pre-warmup. I'll extract this logic out to make it more clear. Pre-warmup is just jvm/sdk warming up, sending downloading and uploading requests for smaller object. Warmup is to send the same request as the actual run. Warmup is needed for crt based s3 client because it takes sometime to resolve the IP addresses and create new connections.

I think for the sake of simplicity, I would still consider merging all of this into the pure "warmup" phase, and running more iterations if needed.

Bennett-Lynch · 2021-07-08T00:32:41Z

test/s3-benchmarks/src/main/java/software/amazon/awssdk/s3benchmarks/BenchmarkUtils.java

+import java.util.List;
+import software.amazon.awssdk.utils.Logger;
+
+public class BenchmarkUtils {


Bennett-Lynch · 2021-07-08T00:37:20Z

test/s3-benchmarks/src/main/java/software/amazon/awssdk/s3benchmarks/BenchmarkUtils.java

+                                       .average()
+                                       .orElse(0.0);
+
+        double lowestLatency = metrics.stream()


Percentiles might be interesting here as well, rather than just min/max.

https://guava.dev/releases/23.0/api/docs/com/google/common/math/Quantiles.html

Ah yeah, good idea

Bennett-Lynch · 2021-07-08T00:45:49Z

...arks/src/main/java/software/amazon/awssdk/s3benchmarks/TransferManagerDownloadBenchmark.java


    public TransferManagerDownloadBenchmark(TransferManagerBenchmarkConfig config) {
        super(config);
+        this.contentLength = s3Sync.headObject(b -> b.bucket(bucket).key(key)).contentLength();


I think we should parameterize (i.e., @Param) the file/object size. WDYT?

Currently, the CLI is designed to only download or upload a single object based on the bucket and key in the input params. I think it would make more sense to parameterize it if we decide to run this as part of the release pipeline, and we can create a workflow for it, something like uploading objects of size 512MB, 1GB, xxx and then downloading them.

I think different object sizes may have very different performance profiles, and it would be helpful to be able to use this benchmark to determine at what thresholds the CRT/v1 behave differently, or when TransferManager becomes faster than a standard getObject request (TransferManager may be slower for very small objects, for example). The CRT input params should not be a constraint here. We can do something like this:

// 1KB 1MB 10MB @Param({"1024", "1048576", "10485760"}) public long objectSize; @Setup(Level.Invocation) public void setUp() { // put object with objectSize } @Benchmark public void benchmark() { // get object }

Bennett-Lynch · 2021-07-08T00:49:10Z

...arks/src/main/java/software/amazon/awssdk/s3benchmarks/TransferManagerDownloadBenchmark.java

-                                           .destination(downloadPath));
-        download.completionFuture().join();
+
+        s3.getObject(b -> b.bucket(bucket).key(key), AsyncResponseTransformer.toFile(downloadPath)).join();


Are we, or should we, delete this file as part of clean up?

Yeah, the file gets deleted on line 94. I'll add try-finally block and move it there

Bennett-Lynch · 2021-07-08T00:51:38Z

...hmarks/src/main/java/software/amazon/awssdk/s3benchmarks/V1BaseTransferManagerBenchmark.java

+        s3Client.shutdown();
+    }
+
+    private void warmUp() {


Why are we not using JMH and its native @Warmup support here?

Yeah, because I feel we need more customizations in s3 perf tests, for example, we send different requests (small object download/upload) for warmup here and we also have additional warmup steps for other tests.

Bennett-Lynch · 2021-07-08T00:56:05Z

...ks/src/main/java/software/amazon/awssdk/s3benchmarks/V1TransferManagerDownloadBenchmark.java

+
+    private void downloadOnceToFile(List<Double> latencies) {
+        Path downloadPath = new File(this.sourcePath).toPath();
+        long start = System.currentTimeMillis();


Nit: Consider Instant.now() and Duration instead.

We are mainly measuring the elapsed time here. Is there any benefit of using Instant or Duration? I guess we could use System.nanoTime(), which provides nanosecond precision, but seems a bit overkill as well to me since this is not a microbenchmark.

zoewangg · 2022-05-09T19:25:02Z

Closing this PR since it's not relevant anymore

…f6f0fc458 Pull request: release <- staging/7b421d5b-857a-4a15-b619-9eaf6f0fc458

zoewangg force-pushed the zoewang/tm-benchmark-update branch from b7c297f to 59f1b88 Compare July 3, 2021 00:22

Refactor s3-benchmarks and include v1 transfermanager tests

a7e4a94

zoewangg force-pushed the zoewang/tm-benchmark-update branch from 59f1b88 to a7e4a94 Compare July 7, 2021 20:05

zoewangg commented Jul 7, 2021

View reviewed changes

Bennett-Lynch reviewed Jul 8, 2021

View reviewed changes

zoewangg closed this May 9, 2022

aws-sdk-java-automation added a commit that referenced this pull request May 26, 2023

Merge pull request #2575 from aws/staging/7b421d5b-857a-4a15-b619-9ea…

4024130

…f6f0fc458 Pull request: release <- staging/7b421d5b-857a-4a15-b619-9eaf6f0fc458

Refactor s3-benchmarks and include v1 transfermanager tests #2575

Refactor s3-benchmarks and include v1 transfermanager tests #2575

Uh oh!

Conversation

zoewangg commented Jul 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

Uh oh!

sonarqubecloud bot commented Jul 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoewangg commented May 9, 2022

Uh oh!

Uh oh!

zoewangg commented Jul 3, 2021 •

edited

Loading