Allow users to control the release of EVP context for AES-GCM #329

amirhosv · 2023-09-15T19:37:25Z

Description of changes:

This change introduces a system property that allows one to control when an EVP_CIPHER_CTX (native context for encryption) is released. For use cases when the same Cipher object is used for multiple encryption/decryption operations, this can result in improved performance since it avoids allocation and release of the native context while the Cipher object is not garbage collected.

There are three strategies for releasing the context: 1) Hybrid, 2) Eager, 3) Lazy.

Hybrid: this is the existing strategy and it's introduced so that existing customers would not be affected. In this strategy, the context is released eagerly, unless it is used with the same key. Benchmark CipherReuse.reuse when newKey is false shows that the existing optimizations are preserved for customers that use the default setting.
Eager: in this strategy, the context is released as soon as possible. Based on CipherReuse.newInstance benchmarks, customers that use a fresh instance of Cipher class for their operations, could experience 30% performance improvement when Eager strategy is used.
Lazy: the context object is preserved while the Cipher object is not garbage collected. Customers caching Cipher objects. Compared to default Hybrid strategy, could experience 16% performance improvement even if they use different keys for with the same Cipher class; however, using the same key, this strategy and Hybrid perform the same.

Here is benchmark results:

ACCP 2.3.2 (the latest release of ACCP on Maven)

#../gradlew -PaccpVersion="2.3.2" -PincludeBenchmark="CipherReuse" lib:jmh

Benchmark                (newKey)   Mode  Cnt       Score       Error  Units
CipherReuse.newInstance      true  thrpt    5  195961.858 ± 26482.726  ops/s
CipherReuse.newInstance     false  thrpt    5  194516.879 ± 31070.067  ops/s
CipherReuse.reuse            true  thrpt    5  305481.372 ± 13363.712  ops/s
CipherReuse.reuse           false  thrpt    5  440410.914 ± 18570.172  ops/s

HYBRID

#./gradlew -PaccpLocalJar="../../build/cmake/AmazonCorrettoCryptoProvider.jar" -PnativeContextReleaseStrategy="HYBRID" -PincludeBenchmark="CipherReuse" lib:jmh

Benchmark                (newKey)   Mode  Cnt       Score       Error  Units
CipherReuse.newInstance      true  thrpt    5  191908.943 ± 26892.090  ops/s
CipherReuse.newInstance     false  thrpt    5  197180.321 ± 29013.380  ops/s
CipherReuse.reuse            true  thrpt    5  323944.919 ± 11095.834  ops/s
CipherReuse.reuse           false  thrpt    5  444121.750 ±  9679.539  ops/s

EAGER

#./gradlew -PaccpLocalJar="../../build/cmake/AmazonCorrettoCryptoProvider.jar" -PnativeContextReleaseStrategy="EAGER" -PincludeBenchmark="CipherReuse" lib:jmh

Benchmark                (newKey)   Mode  Cnt       Score       Error  Units
CipherReuse.newInstance      true  thrpt    5  275440.808 ±  9990.183  ops/s
CipherReuse.newInstance     false  thrpt    5  275176.586 ± 10058.817  ops/s
CipherReuse.reuse            true  thrpt    5  327901.459 ± 12479.909  ops/s
CipherReuse.reuse           false  thrpt    5  324376.765 ± 18051.911  ops/s

LAZY

#./gradlew -PaccpLocalJar="../../build/cmake/AmazonCorrettoCryptoProvider.jar" -PnativeContextReleaseStrategy="LAZY" -PincludeBenchmark="CipherReuse" lib:jmh

Benchmark                (newKey)   Mode  Cnt       Score       Error  Units
CipherReuse.newInstance      true  thrpt    5  223333.903 ± 44229.461  ops/s
CipherReuse.newInstance     false  thrpt    5  221117.187 ± 34093.029  ops/s
CipherReuse.reuse            true  thrpt    5  382687.274 ± 18950.889  ops/s
CipherReuse.reuse           false  thrpt    5  440212.922 ± 11446.041  ops/s

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

geedo0 · 2023-09-18T15:06:13Z

README.md

-
+* `com.amazon.corretto.crypto.provider.freeNativeContextEagerly`
+  Takes in `true` or `false` (defaults to `true`). If `true`, the underlying `EVP_CIPHER_CTX`
+  of a GCM `Cipher` is freed as soon as possible; otherwise, the underlying `EVP_CIPHER_CTX`


This documentation is misleading. We are naming this property very generically, but it only impacts AES-GCM. It doesn't makes sense to expand the scope of this work to allow re-use for all algorithms. I suggest leaving the naming as-is, but calling out that this only applies to AES-GCM more clearly (This leaves the door open to more context reuse in the future).

geedo0 · 2023-09-18T15:07:35Z

benchmarks/README.md

@@ -6,6 +6,8 @@ The benchmarks can use locally built ACCP or published ACCP.
 The `lib:jmh` Gradle task runs the benchmarks and generates reports in JSON and HTML.
 The reports are saved under `lib/build/results/jmh`.

+* `-Pib="INLCUDE_BENCHMARK"` would only run specified benchmark.


geedo0 · 2023-09-18T15:09:03Z

benchmarks/lib/build.gradle.kts

@@ -1,6 +1,7 @@
 val accpVersion: String? by project
 val accpLocalJar: String by project
 val fips: Boolean by project
+val ib: String by project


nit: The name is ambiguous. Maybe give it a long name instead? I think Gradle is smart enough to shorten a long name automatically so we can have it both ways.

geedo0 · 2023-09-18T15:09:57Z

benchmarks/lib/src/jmh/java/com/amazon/corretto/crypto/provider/benchmarks/CipherReuse.java

+    byte[] encrypt(Cipher cipher) throws Exception {
+        random.nextBytes(iv);
+        keyb[0]++;
+        final String name = random.nextBoolean() ? "AES" : "aes";


Seems unecessary for this benchmark.

src/com/amazon/corretto/crypto/provider/AesGcmSpi.java

README.md

benchmarks/lib/build.gradle.kts

SalusaSecondus · 2023-09-26T15:59:03Z

src/com/amazon/corretto/crypto/provider/AesGcmSpi.java


-    byte[] encodedKey = null;
-    if (jceKey != key) {


NOT A CONTRIBUTION

By removing the straight object equality check you are incurring additional allocation and computation even when the key doesn't change.

NOT A CONTRIBUTION

This check only saves time if the Cipher object is reused AND the same AES key is use AND the key is the same Java object; in all other cases, keeping jceKey around not beneficial. From security perspective, jceKey is a cryptographic material and it's not a good practice to keep a reference to a secret object for longer than it is needed.

NOT A CONTRIBUTION

Consider the common case of a single Cipher object being used to encrypt/decrypt a lot of data with the same key but different parameters (iv or gcm). In this case you can absolutely expect to see the exact same Java key object provided each time. What's more, I would expect that the majority of the time that a cipher object is reused, it is reused with the same Java key object.

You're already keeping the cryptographic materials around in private byte[] key, so minimizing it here doesn't help. What's more, as you're calling getEncoded() each time for the comparison, you're resulting in more copies of the cryptographic material spilling to memory than before this change. (In other words, your change will result in more copies of the key in memory rather than fewer.)

NOT A CONTRIBUTION

Despite not having any impact on performance, I brought back the logic. I'm not sure about how "common" this pattern is, since we have many internal customers that prefer not reuse a Cipher object so that they don't have to deal with thread-safety issues.

SalusaSecondus · 2023-09-26T15:59:55Z

src/com/amazon/corretto/crypto/provider/AesGcmSpi.java

-   * The number a times a key must be reused prior to keeping it in native memory rather than
-   * freeing it each time.
-   */
-  private static final int KEY_REUSE_THRESHOLD = 1;


NOT A CONTRIBUTION

By removing KEY_REUSE_THRESHOLD you are removing all context reuse even when the key doesn't change. This is likely to degrade performance for the majority of uses (unless they explicitly opt into your new logic). Have you checked for regression under existing benchmarks?

NOT A CONTRIBUTION

The HYBRID mode, which is the default, preserves the existing behavior and our benchmarks show that there is no regression. When threshold is one, the same effect can be achieved by using a single boolean variable (sameKey).

NOT A CONTRIBUTION

My apologies. I missed the introduction of HYBRID mode.

However, have you checked existing benchmarks or only the new ones that you introduced in this change?

NOT A CONTRIBUTION

Yes, I did the run the existing benchmarks and as expected, there is no regression. The existing benchmarks utilize the same object so other patterns of usage are not measured. Here are the results of the existing benchmarks:

./gradlew -PaccpVersion="2.3.2" -PincludeBenchmark="AesGcm" lib:jmh Benchmark (chunkSize) (keyBits) Mode Cnt Score Error Units AesGcmOneShot.oneShot1MiBDecrypt N/A 128 thrpt 5 2600.963 ± 88.948 ops/s AesGcmOneShot.oneShot1MiBDecrypt N/A 256 thrpt 5 2179.269 ± 31.994 ops/s AesGcmOneShot.oneShot1MiBEncrypt N/A 128 thrpt 5 2606.161 ± 75.525 ops/s AesGcmOneShot.oneShot1MiBEncrypt N/A 256 thrpt 5 2190.634 ± 68.482 ops/s AesGcmUpdate.updateDecrypt 16 128 thrpt 5 734.160 ± 26.745 ops/s AesGcmUpdate.updateDecrypt 16 256 thrpt 5 745.288 ± 24.561 ops/s AesGcmUpdate.updateDecrypt 256 128 thrpt 5 1550.954 ± 47.719 ops/s AesGcmUpdate.updateDecrypt 256 256 thrpt 5 1704.490 ± 80.432 ops/s AesGcmUpdate.updateEncrypt 16 128 thrpt 5 67.282 ± 5.458 ops/s AesGcmUpdate.updateEncrypt 16 256 thrpt 5 66.709 ± 2.305 ops/s AesGcmUpdate.updateEncrypt 256 128 thrpt 5 789.663 ± 19.540 ops/s AesGcmUpdate.updateEncrypt 256 256 thrpt 5 737.160 ± 31.125 ops/s ./gradlew -PaccpLocalJar="../../build/cmake/AmazonCorrettoCryptoProvider.jar" -PnativeContextReleaseStrategy="HYBRID" -PincludeBenchmark="AesGcm" lib:jmh Benchmark (chunkSize) (keyBits) Mode Cnt Score Error Units AesGcmOneShot.oneShot1MiBDecrypt N/A 128 thrpt 5 2611.790 ± 141.192 ops/s AesGcmOneShot.oneShot1MiBDecrypt N/A 256 thrpt 5 2175.562 ± 68.551 ops/s AesGcmOneShot.oneShot1MiBEncrypt N/A 128 thrpt 5 2639.222 ± 63.212 ops/s AesGcmOneShot.oneShot1MiBEncrypt N/A 256 thrpt 5 2196.239 ± 317.460 ops/s AesGcmUpdate.updateDecrypt 16 128 thrpt 5 787.723 ± 28.777 ops/s AesGcmUpdate.updateDecrypt 16 256 thrpt 5 675.053 ± 28.231 ops/s AesGcmUpdate.updateDecrypt 256 128 thrpt 5 1887.667 ± 64.505 ops/s AesGcmUpdate.updateDecrypt 256 256 thrpt 5 1652.982 ± 95.543 ops/s AesGcmUpdate.updateEncrypt 16 128 thrpt 5 67.716 ± 1.974 ops/s AesGcmUpdate.updateEncrypt 16 256 thrpt 5 66.032 ± 4.465 ops/s AesGcmUpdate.updateEncrypt 256 128 thrpt 5 789.979 ± 38.398 ops/s AesGcmUpdate.updateEncrypt 256 256 thrpt 5 738.422 ± 28.252 ops/s

benchmarks/lib/build.gradle.kts

geedo0 · 2023-11-14T15:10:52Z

benchmarks/lib/src/jmh/java/com/amazon/corretto/crypto/provider/benchmarks/CipherReuse.java

+        blackhole.consume(encrypt(shared));
+    }
+
+    byte[] encrypt(Cipher cipher) throws Exception {


pedantic: the method name says encrypt, but you do both directions.

Renamed to encryptDecrypt

geedo0 · 2023-11-14T15:15:39Z

src/com/amazon/corretto/crypto/provider/Utils.java

+  }
+
+  static NativeContextReleaseStrategy getNativeContextReleaseStrategyProperty(
+      final String propertyName) {


This interface doesn't make sense. When would you ever need to call this with a different property name? Supporting that seems error-prone. Suggest referencing a constant within this method and removing the indirection inside AmazonCorrettoCryptoProvider.

super nit: I'd inline this function at this point.

WillChilds-Klein · 2023-11-15T14:43:28Z

benchmarks/lib/src/jmh/java/com/amazon/corretto/crypto/provider/benchmarks/CipherReuse.java

+    private Cipher getAesGcmCipherFromAccp() {
+        try {
+            return Cipher.getInstance(AES_GCM, AmazonCorrettoCryptoProvider.INSTANCE);
+        } catch (final Exception e) {


very minor nit: both callers of this function are themselves in functions that throw checked exceptions, so you don't need to wrap e in an unchecked exception here.

src/com/amazon/corretto/crypto/provider/AesGcmSpi.java

tst/com/amazon/corretto/crypto/provider/test/AesTest.java

amirhosv force-pushed the CipherContext branch from cd1a95a to 833ee8a Compare September 15, 2023 20:49

amirhosv marked this pull request as ready for review September 16, 2023 01:39

amirhosv requested a review from a team as a code owner September 16, 2023 01:39

amirhosv force-pushed the CipherContext branch from 833ee8a to b22bd39 Compare September 18, 2023 13:44

geedo0 reviewed Sep 18, 2023

View reviewed changes

amirhosv force-pushed the CipherContext branch 2 times, most recently from 2dc467b to 9ae3a60 Compare September 19, 2023 00:16

brian-jarvis-aws assigned brian-jarvis-aws and unassigned brian-jarvis-aws Sep 19, 2023

brian-jarvis-aws self-requested a review September 19, 2023 01:41

WillChilds-Klein reviewed Sep 19, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

benchmarks/lib/build.gradle.kts Outdated Show resolved Hide resolved

benchmarks/lib/build.gradle.kts Show resolved Hide resolved

amirhosv force-pushed the CipherContext branch 2 times, most recently from 6ea55e1 to 680e57c Compare September 20, 2023 16:30

WillChilds-Klein self-requested a review September 21, 2023 12:43

SalusaSecondus reviewed Sep 26, 2023

View reviewed changes

brian-jarvis-aws removed their request for review November 6, 2023 15:05

amirhosv force-pushed the CipherContext branch from 680e57c to 6275528 Compare November 7, 2023 18:18

amirhosv changed the title ~~Lazy release of EVP context~~ Allow users to control the release of EVP context for AES-GCM Nov 7, 2023

amirhosv force-pushed the CipherContext branch 2 times, most recently from d6df464 to 5438240 Compare November 11, 2023 12:40

geedo0 previously approved these changes Nov 14, 2023

View reviewed changes

amirhosv dismissed geedo0’s stale review via 3819c95 November 14, 2023 17:15

amirhosv force-pushed the CipherContext branch from 5438240 to 3819c95 Compare November 14, 2023 17:15

geedo0 previously approved these changes Nov 14, 2023

View reviewed changes

WillChilds-Klein previously approved these changes Nov 15, 2023

View reviewed changes

Allow users to control the release of EVP context for AES-GCM

c72699c

amirhosv dismissed stale reviews from WillChilds-Klein and geedo0 via c72699c November 15, 2023 15:45

amirhosv force-pushed the CipherContext branch from 3819c95 to c72699c Compare November 15, 2023 15:45

WillChilds-Klein approved these changes Nov 15, 2023

View reviewed changes

geedo0 approved these changes Nov 15, 2023

View reviewed changes

amirhosv merged commit c72699c into corretto:main Nov 15, 2023
8 checks passed

amirhosv deleted the CipherContext branch November 15, 2023 18:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow users to control the release of EVP context for AES-GCM #329

Allow users to control the release of EVP context for AES-GCM #329

amirhosv commented Sep 15, 2023 •

edited

Loading

geedo0 Sep 18, 2023

geedo0 Sep 18, 2023

geedo0 Sep 18, 2023

geedo0 Sep 18, 2023

SalusaSecondus Sep 26, 2023

amirhosv Sep 26, 2023

SalusaSecondus Sep 26, 2023

amirhosv Nov 7, 2023 •

edited

Loading

SalusaSecondus Sep 26, 2023

amirhosv Sep 26, 2023

SalusaSecondus Sep 26, 2023 •

edited

Loading

amirhosv Nov 7, 2023

geedo0 Nov 14, 2023

amirhosv Nov 14, 2023

geedo0 Nov 14, 2023

amirhosv Nov 14, 2023

geedo0 Nov 14, 2023

WillChilds-Klein Nov 15, 2023

Allow users to control the release of EVP context for AES-GCM #329

Allow users to control the release of EVP context for AES-GCM #329

Conversation

amirhosv commented Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirhosv Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SalusaSecondus Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirhosv commented Sep 15, 2023 •

edited

Loading

amirhosv Nov 7, 2023 •

edited

Loading

SalusaSecondus Sep 26, 2023 •

edited

Loading