RFR: 8274308: Improve efficiency for HandshakeContext initialization.

[Xue-Lei Andrew Fan]

On Sat, 2 Oct 2021 05:45:47 GMT, Clive Verghese <cverghese at openjdk.org> wrote:

> Hi,
> 
> We have identified that the `HandshakeContext` initialization takes up a close to 50% of the flame graph for startHandshake. I have moved the computation of the `activeProtocols` and `activeCipherSuites` from the HandshakeContext constructor to SSLConfiguration class because the values used to compute the two list are available in the SSLConfiguration. 
> 
> In order to reuse this, I have initialized SSLConfiguration in the SSLSocketFactory and reused this when possible for Client Socket Constructors in the SSLSocketImpl. 
> 
> Sockets created from the SSLSocketFactory see this improvements. 
> 
> Old Benchmarks
> 
> Benchmark                                   Mode  Cnt  Score   Error   Units
> SSLStartHandshake.handshakeBenchmark       thrpt   25  0.247 ± 0.011  ops/ms
> SSLStartHandshake.handshakeBenchmark        avgt   25  4.210 ± 0.445   ms/op
> 
> New Benchmarks
> 
> SSLStartHandshake.handshakeBenchmark       thrpt   25  0.257 ± 0.017  ops/ms
> SSLStartHandshake.handshakeBenchmark        avgt   25  3.967 ± 0.208   ms/op
> 
> 
> 
> I have also attached the JFR profiles from before and after the change.
> Before
> <img width="2325" alt="SSLOverhead-old" src="https://user-images.githubusercontent.com/934461/135705010-a8502966-c6be-4cb5-b743-4a5b382c8e1f.png">
> 
> After 
> <img width="2310" alt="SSLOverhead-new" src="https://user-images.githubusercontent.com/934461/135705007-ea852b36-e10f-4741-a166-249270b34465.png">
>  
> We have been able to optimize the `TransportContext.kickstart` function by removing the `HandshakeContext.<init>`  initialization and reduce the time to start the handshake by reusing `activeProtocols` and `activeCipherSuites`
> 
> In addition to the SSL and http tests, I have run tier-1 and tier-2 tests and ensure that they pass. 
> 
> Looking for your feedback

Changes requested by xuelei (Reviewer).

It looks like a good idea to make less computation for each handshake context.  Thanks for that.  I have some concerns for the update, and see the comments above.

src/java.base/share/classes/sun/security/ssl/SSLConfiguration.java line 57:

> 55:     AlgorithmConstraints        userSpecifiedAlgorithmConstraints;
> 56:     List<ProtocolVersion>       enabledProtocols;
> 57:     List<CipherSuite>           enabledCipherSuites;

What's the motivation to change enabledProtocols/CipherSuites to private?

src/java.base/share/classes/sun/security/ssl/SSLConfiguration.java line 278:

> 276:                 this.enabledCipherSuites, this.algorithmConstraints);
> 277:         this.activeCipherSuites = getActiveCipherSuites(this.activeProtocols,
> 278:                 this.enabledCipherSuites, this.algorithmConstraints);

Maybe a lazy set of activeProtocols could be better as setSSLParameters() may be called multiple times in practice.

src/java.base/share/classes/sun/security/ssl/SSLSocketFactoryImpl.java line 47:

> 45: 
> 46:     private final SSLContextImpl context;
> 47:     private final SSLConfiguration configuration;

Share the mutable configuration within the instances created by the factory may be not a good idea.

src/java.base/share/classes/sun/security/ssl/SSLSocketFactoryImpl.java line 173:

> 171:     {
> 172:         return new SSLSocketImpl(context, address, port,
> 173:                 clientAddress, clientPort, configuration);

Note that the configuration is used and shared for among socket instances, which may be not the expected behavior.

src/java.base/share/classes/sun/security/ssl/SSLSocketImpl.java line 174:

> 172:         HandshakeHash handshakeHash = new HandshakeHash();
> 173:         this.conContext = new TransportContext(sslContext, this,
> 174:                 sslConfiguration,

Sharing the mutable sslConfiguration for individual sockets may be not a good idea.

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

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