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

[Xue-Lei Andrew Fan]

On Fri, 29 Oct 2021 23:58:06 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
>
> 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