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

[Clive Verghese]

On Sat, 6 Nov 2021 08:54:33 GMT, Daniel Jeliński <duke at openjdk.java.net> wrote:

>>> 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?
>> 
>> I know, there are applications setting protocols and cipher suites for themselves.  But it should be common that the TLS connections created from the same SSLContext uses the same cipher suites and protocols.  There are still exceptions, but may be not that common.  Using an instance cache in SSLContext rather than static cache could be a further improvement.
>> 
>> I would not use static cache unless there is no other option.  The troublesome of static cache is more than the performance impact.  A series of problems like memory leaking and security issues could be followed even we design the cache very carefully.
>> 
>> About the benchmark, did you have a chance to check the result for full handshakes?  The code in the benchmark checks the initialization of the handshake, the handshake may not be full/completed.  A benchmark for full handshake may be able to provide more evidence about the overall impact of the improvement.
>
> Can we extend the public API of `SSLContext` with methods for managing `activeProtocols`, `activeCipherSuites` and possibly `algorithmConstraints`? Without this API change we would need to check every time if the active protocols, ciphers and constraints match the cached ones.
> 
> Also, looking at the flame graphs provided, it appears that about 50% of the handshake time is spent in `SSLAlgorithmDecomposer#decompose`; given that there are only 3 instances of that class (and could be further reduced to 2), would it make sense to optimize the method by caching the results of algorithm decomposition? We could use a size-limited soft cache to avoid memory leaks, and I fail to see how the cached information could be exploitable.

Hi @djelinski @XueleiFan, 

Thank you for the feedback on this issue. 

`activeProtocols` and `activeCipherSuites` are currently derived from `enabledProtocols` and `enabledCipherSuites` and `algorithmConstraints`. And the [SSLContext is currently considered as immutable.](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/security/ssl/SSLContextImpl.java#L42). If we provide ways to configure in SSLContext, We would need to rewrite this assumption. That could have implications in other areas where this assumption is currently held(?). 

The other option of having an instance cache in SSLContext could be helpful as well, but then it would also affect the assumption above.  

I can look into optimizing the SSLAlgorithmDecomposer, by adding a size limited cache there to see if it would beneficial. This could be an instance variable as well, so @XueleiFan concerns of static cache can also be addressed. I'll check if the benchmarks look better with the new approach. 

Additionally, I looked into  the decompose method and found that there is scope for improvements there. We could eliminate the [repeated calls to decompose CipherSuite](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/security/ssl/SSLAlgorithmDecomposer.java#L269). This would need us to store the results of decomposing the CipherSuite, Both in the case of when onlyX509 is true and false. 

Other optimization in the decompose path are in SSLCipher, but this won't be necessary if we optimize the CipherSuite decompose mentioned above. 

Other times spent in the function is within the HashMap and Collections (`add` and `addAll`). 

<img width="2986" alt="Screen Shot 2021-11-09 at 1 29 57 PM" src="https://user-images.githubusercontent.com/934461/141008035-a6549701-2233-4d3e-ba61-7859d25b339c.png">

I can follow up on this based on the results of the SSLAlgorithmDecomposer optimizations, and if this is still an issue, I can open a seperate JBS issue and create a PR for this.

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

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