Re: RFR: 8274308: Improve efficiency for HandshakeContext initialization.

[Xue-Lei Andrew Fan]

On Sat, 2 Oct 2021 05:45:47 GMT, Clive Verghese <cvergh...@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