[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-security-dev
Subject:    Re: RFR: 8274308: Improve efficiency for HandshakeContext initialization. [v2]
From:       Xue-Lei Andrew Fan <xuelei () openjdk ! java ! net>
Date:       2021-10-30 5:02:10
Message-ID: ux2qOe79gPq5OJDfKyIj14chiobLYbHRk4r1VCtgbsM=.60ec55a5-9f11-468f-a30b-1cf25f8ae820 () github ! com
[Download RAW message or body]

On Fri, 29 Oct 2021 23:58:06 GMT, Clive Verghese <cverghese@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
> 
> 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.

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

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


[prev in list] [next in list] [prev in thread] [next in thread]