Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve AesGcmSiv Aead performance by optimizing classpath check #5

Closed
wants to merge 2 commits into from

Conversation

ks-yim
Copy link
Contributor

@ks-yim ks-yim commented Mar 3, 2023

Motivation:

Aead implementation for AesGcmSiv checks if GCMParameterSpec class exists in classpath by invoking Class#forName method whenever data gets encrypted or decrypted.

A benchmark on AesGcmSiv claims that Class#forName accounts for 4-5% of the total CPU cycles consumed for data en/decryption and 12% or more throughput gain can be obtained by optimizing away the Class#forName method invocation.

Modification:

Cache the result of the classpath check in AesGcmSiv

BM Results:

  • BM Code

  • Run on:

    • MacBook Pro (16-inch, 2021) Apple M1 Pro 32GB, MacOS Monterey version 12.6.2
    • Used Conscrypt custom build based on 3051cda to utilize AES-NI on Apple Silicon
  • Before

    # JMH version: 1.35
    # VM version: JDK 11.0.17, OpenJDK 64-Bit Server VM, 11.0.17+8
    # VM invoker: /Users/ks-yim/.sdkman/candidates/java/11.0.17-tem/bin/java
    # VM options: -Dfile.encoding=UTF-8 -Djava.io.tmpdir=/Users/ks-yim/IdeaProjects/tink-bm/build/tmp/jmh - 
    Duser.country=KR -Duser.language=en -Duser.variant
    # Blackhole mode: full + dont-inline hint (auto-detected, use -Djmh.blackhole.autoDetect=false to disable)
    # Warmup: 1 iterations, 10 s each
    # Measurement: 1 iterations, 10 s each
    # Timeout: 10 min per iteration
    # Threads: 8 threads, will synchronize iterations
    # Benchmark mode: Throughput, ops/time
    # Benchmark: com.example.tink.bm.AesGcmSivBenchmark.conscrypt_encrypt
    
    Benchmark                                             Mode  Cnt       Score   Error  Units
    AesGcmSivBenchmark.bc_decrypt                        thrpt       140397.856          ops/s
    AesGcmSivBenchmark.bc_encrypt                        thrpt       161178.303          ops/s
    AesGcmSivBenchmark.conscrypt_decrypt                 thrpt       487751.715          ops/s
    AesGcmSivBenchmark.conscrypt_encrypt                 thrpt       462492.430          ops/s
    
  • After

    Benchmark                                             Mode  Cnt       Score   Error  Units
    AesGcmSivBenchmark.bc_decrypt                        thrpt       168118.773          ops/s
    AesGcmSivBenchmark.bc_encrypt                        thrpt       172307.366          ops/s
    AesGcmSivBenchmark.conscrypt_decrypt                 thrpt       546720.978          ops/s
    AesGcmSivBenchmark.conscrypt_encrypt                 thrpt       551282.859          ops/s
    

CPU Profiling Result:

  • Encryption

    • Before
      flame-graph-cpu

    • After
      flame-graph-cpu

  • Decryption

    • Before
      flame-graph-cpu

    • After
      flame-graph-cpu

@google-cla
Copy link

google-cla bot commented Mar 3, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Motivation:

`Aead` implementation for `AesGcmSiv` checks if `GCMParameterSpec` class
exists in classpath by invoking `Class#forName` method whenever data
gets encrypted or decrypted.

A benchmark on `AesGcmSiv` claims that `Class#forName` accounts for 4-5%
of the total CPU cycles consumed for data en/decryption and 12% or more
throughput gain can be obtained by optimizing away the `Class#forName`
method invocation.

Modification:

Cache the result of the classpath check in `AesGcmSiv`
@@ -58,6 +58,18 @@ protected Cipher initialValue() {
private static final int IV_SIZE_IN_BYTES = 12;
private static final int TAG_SIZE_IN_BYTES = 16;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is slightly easier to understand if you just do:

private static final boolean HAS_GCM_PARAMETER_SPEC_CLASS = checkForGCMParameterSpec();

private static final boolean checkForGCMParameterSpec() {
try {
Class.forName("javax.crypto.spec.GCMParameterSpec");
} catch (ClassNotFoundException e) {
return false;
}
return true;
}

WDYT?

Copy link
Contributor Author

@ks-yim ks-yim Mar 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late response.

No objection to it. I will send a new commit!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed it, PTAL 🙏.

@ks-yim ks-yim dismissed a stale review via bc2584e March 17, 2023 02:52
@tholenst
Copy link
Contributor

Thank you. I will try to merge this but still have to see if I figure out how we set this up. Please ping me if it didn't work.

@ks-yim
Copy link
Contributor Author

ks-yim commented Mar 17, 2023

@tholenst Thanks for reviewing this, but I didn't get you in the last sentence.
Should I do something and let you know the result if it doesn't work?

@tholenst
Copy link
Contributor

Sorry for being unclear.

I am working on merging this, but the process works in some predefined way interacting with the Google-internal repository. I do not have experience with the process, so it might happen that it doesn't work and I forget about it.

It should be merged on Monday, if it didn't work I made a mistake and I would welcome a reminder.

@tholenst
Copy link
Contributor

Hmm, it seems this was now merged, but it was not associated with this PR, see f7ba536

Nevertheless, it seems it's correctly associated with you -- at least the commit seems to show up on your stats. I guess like this it's no big deal? WDYT?

@ks-yim
Copy link
Contributor Author

ks-yim commented Mar 22, 2023

@tholenst thanks for working on this, I will close the PR, then.

@ks-yim ks-yim closed this Mar 22, 2023
@ks-yim ks-yim deleted the gcmsiv-perf branch March 22, 2023 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants