RFR: 8274308: Improve efficiency for HandshakeContext initialization. [v2]

[Clive Verghese]

On Sat, 30 Oct 2021 04:57:22 GMT, Xue-Lei Andrew Fan <xuelei at openjdk.org> wrote:

>> Clive Verghese has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains three new commits since the last revision:
>> 
>>  - 8274308: Improve efficiency for HandshakeContext initialization
>>  - Add SSLEngineBenchmark
>>  - Benchmark
>
> src/java.base/share/classes/sun/security/ssl/HandshakeContext.java line 74:
> 
>> 72:                     "sun.security.ssl.allowLegacyHelloMessages", true);
>> 73: 
>> 74:     static Cache<Double, HandshakeContextCacheItem> handshakeContextCache;
> 
> I think I understand the motivation, but a mutable cache in handshake context does not sound like the right direction.  A cache could impact the performance more and occupy additional memories , because we have to synchronizing the access to the cache.  Maybe, you could to benchmark the maximum operations allowed per seconds, and try to use more complex cases, and see what the result could be.
> 
> I did not yet try to find an improvement yet.  But at this moment, the idea to me is to thinking about the TLS user cases.  It is common that applications use one instance of SSLContext and default configurations.  From the SSLContext instance, with the default configuration, the connections get established.  Normally, we don't recommend to set protocols and cipher suites other than the default SSLContex configuration, because it is not algorithm aglity.  So, it may be a direction to have the default and final active protocols and cipher suites, parsed and cached in SSLContext.  Just for your reference if you want a further improvement.

Hi @XueleiFan,

Thank you for the feedback, though it’s not recommended to set the protocols and cipher suites, various frameworks often do set these values, [Some references below]. Would it still make sense to only optimize the default case?. With my first variation, I had tried to avoid using the cache. However, The particular implementations had it drawbacks.

I have run some benchmarks, by rerunning the SSLStartHandshake Benchmark by increasing the concurrency to Threads.MAX and increasing the number of iterations for warmup and measurement. This triggers the concurrency effects of the cache, and here are my findings.

Baseline measurement (The default at origin/master)

Benchmark                                   Mode  Cnt   Score   Error   Units
SSLStartHandshake.handshakeBenchmark       thrpt  150   1.056 ± 0.048  ops/ms
SSLStartHandshake.handshakeBenchmark        avgt  150  13.198 ± 3.688   ms/op

Result "org.openjdk.bench.java.security.SSLStartHandshake.handshakeBenchmark":
  1.056 ±(99.9%) 0.048 ops/ms [Average]
  (min, avg, max) = (0.254, 1.056, 1.194), stdev = 0.174
  CI (99.9%): [1.008, 1.103] (assumes normal distribution)

Result "org.openjdk.bench.java.security.SSLStartHandshake.handshakeBenchmark":
  13.198 ±(99.9%) 3.688 ms/op [Average]
  (min, avg, max) = (10.128, 13.198, 106.039), stdev = 13.454
  CI (99.9%): [9.510, 16.886] (assumes normal distribution)


Cache implementation according to this PR with synchronization.

Benchmark                                   Mode  Cnt   Score   Error   Units
SSLStartHandshake.handshakeBenchmark       thrpt  150   1.098 ± 0.079  ops/ms
SSLStartHandshake.handshakeBenchmark        avgt  150  16.640 ± 6.784   ms/op

Result "org.openjdk.bench.java.security.SSLStartHandshake.handshakeBenchmark":
  1.098 ±(99.9%) 0.079 ops/ms [Average]
  (min, avg, max) = (0.087, 1.098, 1.272), stdev = 0.288
  CI (99.9%): [1.019, 1.177] (assumes normal distribution)

Result "org.openjdk.bench.java.security.SSLStartHandshake.handshakeBenchmark":
  16.640 ±(99.9%) 6.784 ms/op [Average]
  (min, avg, max) = (9.608, 16.640, 166.450), stdev = 24.752
  CI (99.9%): [9.855, 23.424] (assumes normal distribution)


Concurrent HashMap as a Cache Implementation

Benchmark                                   Mode  Cnt   Score   Error   Units
SSLStartHandshake.handshakeBenchmark       thrpt  150   1.163 ± 0.052  ops/ms
SSLStartHandshake.handshakeBenchmark        avgt  150  12.570 ± 3.854   ms/op

Result "org.openjdk.bench.java.security.SSLStartHandshake.handshakeBenchmark":
  1.163 ±(99.9%) 0.052 ops/ms [Average]
  (min, avg, max) = (0.172, 1.163, 1.287), stdev = 0.191
  CI (99.9%): [1.110, 1.215] (assumes normal distribution)

Result "org.openjdk.bench.java.security.SSLStartHandshake.handshakeBenchmark":
  12.570 ±(99.9%) 3.854 ms/op [Average]
  (min, avg, max) = (9.261, 12.570, 116.059), stdev = 14.061
  CI (99.9%): [8.716, 16.424] (assumes normal distribution)


I was synchronizing based on the  `handshakeContextCache`.
ConcurrentHashMap does provide better results when compared to the baseline, However, I would need figure out the evicting the nodes based on the size.

Please do let me know what you think about the results and I can update the PR accordingly.

Examples of library code explicitly setting the enabledProtocols and enabledCipherSuites
1: https://github.com/apache/httpcomponents-client/blob/861c071ee4f74a287e7b94b0711045bb5a46d8cf/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/SSLConnectionSocketFactory.java#L279
2: https://github.com/apache/hadoop/blob/86729e130fb563d87917850a41bff3b0a886246f/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/SSLFactory.java#L266

-------------

PR: https://git.openjdk.java.net/jdk/pull/5793