Various performance improvements. #3178

millems · 2022-05-05T19:57:03Z

Replace uses of String.replace with StringUtils.replace and StringUtils.replaceEach to avoid the overhead of regex parsing the search term.
Remove uses of SdkHttpRequest and SdkHttpResponse's headers and rawQueryParameters calls in favor of more specific methods that do not require data copying. Added spotbugs rules to prevent uses of these methods in the future.
Update SdkHttpRequest and SdkHttpResponse to not copy data in headers and rawQueryParameters when round-tripped to/from builder unless it is actually modified.
Reduce string copies performed during message signing.
Remove uses of String.format that were performance-impacting on profiler output.
Remove construction of log strings within the netty client unless the logging is enabled.
Move most user-agent construction to client creation time instead of at request time.
Initialize collection/map capacities to prevent resizes that will always occur.
Prevent creation of RateLimitingTokenBucket unless adaptive retries are enabled.

1. Replace uses of String.replace with StringUtils.replace and StringUtils.replaceEach to avoid the overhead of regex parsing the search term. 2. Remove uses of SdkHttpRequest and SdkHttpResponse's headers and rawQueryParameters calls in favor of more specific methods that do not require data copying. Added spotbugs rules to prevent uses of these methods in the future. 3. Update SdkHttpRequest and SdkHttpResponse to not copy data in headers and rawQueryParameters when round-tripped to/from builder unless it is actually modified. 4. Reduce string copies performed during message signing. 5. Remove uses of String.format that were performance-impacting on profiler output. 6. Remove construction of log strings within the netty client unless the logging is enabled. 7. Move most user-agent construction to client creation time instead of at request time. 8. Initialize collection/map capacities to prevent resizes that will always occur. 9. Prevent creation of RateLimitingTokenBucket unless adaptive retries are enabled.

millems · 2022-05-06T19:34:22Z

.../src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/RetryableStage.java

-        this.rateLimitingTokenBucket = new RateLimitingTokenBucket();
+        this.rateLimitingTokenBucket = null;


Since you're reviewing, @dagnir: Isn't the RetryableStage created with each request, so this rate limiting token bucket is never reused between requests, making it essentially do nothing?

Ah good catch!

I didn't fix that here. I can create a backlog item to fix it...

dagnir · 2022-05-10T16:24:31Z

.../src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/RetryableStage.java

-        this.rateLimitingTokenBucket = new RateLimitingTokenBucket();
+        this.rateLimitingTokenBucket = null;


Ah good catch!

dagnir · 2022-05-10T17:03:48Z

Awesome stuff!

sonarqubecloud · 2022-05-10T23:16:56Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
24 Code Smells

61.1% Coverage
11.3% Duplication

S3 team had been, partly on our reports [1] working on numerous performance enhancements [2] [3] that Apache James IMAP workload would definitely benefit from. [1] aws/aws-sdk-java-v2#3162 [2] aws/aws-sdk-java-v2#3175 [3] aws/aws-sdk-java-v2#3178 Upgrade

chibenwa · 2022-05-13T01:44:43Z

core/auth/src/main/java/software/amazon/awssdk/auth/signer/AsyncAws4Signer.java

@@ -69,7 +69,7 @@ public CompletableFuture<SdkHttpFullRequest> signWithBody(SdkHttpFullRequest req
            Aws4SignerRequestParams requestParams = new Aws4SignerRequestParams(signingParams);

            SdkHttpFullRequest.Builder builder = doSign(request, requestParams, signingParams,
-                    new ContentChecksum(digestHex, sdkChecksum));
+                                                        new ContentChecksum(digestHex, sdkChecksum));


Is the indent change here intended? Or an IDE fail?

Not intended, but more of an IDE success. The indent is an improvement, imo.

chibenwa · 2022-05-13T01:47:09Z

core/auth/src/main/java/software/amazon/awssdk/auth/signer/internal/AbstractAws4Signer.java

+     * @return true if the character is white  space, false otherwise.
+     */
+    private boolean isWhiteSpace(final char ch) {
+        return ch == ' ' || ch == '\t' || ch == '\n' || ch == '\u000b' || ch == '\r' || ch == '\f';


Guava's charmatcher allows denser checks, with a binary search and binary sets, which might be handy with 6 + chars to check for.... Especially if in the future you decide to add more.

I considered doing something like a 256 size boolean array lookup table for chars being whitespace (with some special handling for unicode), but this method didn't show up very high on a profiler so it felt like premature optimization. It would be nice if we could have a Guava dependency, but since we have to implement it ourselves (or deal with importing it), I wasn't inclined.

chibenwa · 2022-05-13T01:48:37Z

core/auth/src/main/java/software/amazon/awssdk/auth/signer/internal/AbstractAws4Signer.java

        byte[] kSecret = ("AWS4" + credentials.secretAccessKey())
-            .getBytes(Charset.forName("UTF-8"));
+            .getBytes(StandardCharsets.UTF_8);


Does this needs to be recomputed all of the time? Can't the credentials holds kSecret?

That's a cool idea. We'd probably want to use a concurrent hash map with weak reference keys from credential to cached values so that we don't leak that information out of the signer, but that could definitely save some computation for reused credentials at the cost of some volatile reads.

I thought credentials was immutable when commenting. If mutability is in the game it would ruin potentially ruin gains...

Credentials is immutable, but they might be used for other signing algorithms, so we don't really want to pre-calculate it within the credentials when it's created.

chibenwa · 2022-05-13T01:50:31Z

...-core/src/main/java/software/amazon/awssdk/awscore/client/handler/AwsClientHandlerUtils.java

-                                                  .stream()
-                                                  .collect(Collectors.toMap(Map.Entry::getKey, e -> HeaderValue.fromString(
-                                                      firstIfPresent(e.getValue()))));
+        Map<String, HeaderValue> headers = new LinkedHashMap<>();


Anything preventing us to pre-size this hash map?

Nope, good catch!

S3 team had been, partly on our reports [1] working on numerous performance enhancements [2] [3] that Apache James IMAP workload would definitely benefit from. [1] aws/aws-sdk-java-v2#3162 [2] aws/aws-sdk-java-v2#3175 [3] aws/aws-sdk-java-v2#3178

millems requested a review from a team as a code owner May 5, 2022 19:57

millems force-pushed the millem/perf branch from 0b99003 to dcb6593 Compare May 5, 2022 20:31

millems force-pushed the millem/perf branch from dcb6593 to 7aca70b Compare May 5, 2022 20:33

millems commented May 6, 2022

View reviewed changes

dagnir approved these changes May 10, 2022

View reviewed changes

Merge branch 'master' into millem/perf

f40cd35

millems enabled auto-merge (squash) May 10, 2022 22:19

millems disabled auto-merge May 10, 2022 22:53

millems enabled auto-merge (squash) May 10, 2022 22:55

millems merged commit 1eaba50 into master May 10, 2022

debora-ito mentioned this pull request May 12, 2022

Collections of async profiler collected enhancements #3162

Open

chibenwa mentioned this pull request May 13, 2022

[UPGRADE] S3 2.17.170 -> 2.17.189 apache/james-project#999

Merged

chibenwa reviewed May 13, 2022

View reviewed changes

millems deleted the millem/perf branch October 19, 2022 19:35

joviegas mentioned this pull request Nov 30, 2023

fix ClientOverrideConfiguration.Builder empty headers throwing NPE #4726

Merged

		this.rateLimitingTokenBucket = new RateLimitingTokenBucket();
		this.rateLimitingTokenBucket = null;

Various performance improvements. #3178

Various performance improvements. #3178

Uh oh!

Conversation

millems commented May 5, 2022

Uh oh!

millems May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

dagnir commented May 10, 2022

Uh oh!

sonarqubecloud bot commented May 10, 2022

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!

millems May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

millems May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

millems May 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

millems May 6, 2022 •

edited

Loading

millems May 13, 2022 •

edited

Loading

millems May 13, 2022 •

edited

Loading

millems May 16, 2022 •

edited

Loading