Re: RFR: 8274308: Improve efficiency for HandshakeContext initialization. [v2]
> 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.handshakeBenchmarkavgt 25 4.210 ± 0.445 ms/op > > New Benchmarks > > SSLStartHandshake.handshakeBenchmark thrpt 25 0.257 ± 0.017 ops/ms > SSLStartHandshake.handshakeBenchmarkavgt 25 3.967 ± 0.208 ms/op > > > > I have also attached the JFR profiles from before and after the change. > Before > src="https://user-images.githubusercontent.com/934461/135705010-a8502966-c6be-4cb5-b743-4a5b382c8e1f.png";> > > After > 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.` 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/5793/files - new: https://git.openjdk.java.net/jdk/pull/5793/files/46ae42bf..baf0a90f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5793&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5793&range=00-01 Stats: 882 lines in 9 files changed: 487 ins; 351 del; 44 mod Patch: https://git.openjdk.java.net/jdk/pull/5793.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5793/head:pull/5793 PR: https://git.openjdk.java.net/jdk/pull/5793
Re: RFR: 8274308: Improve efficiency for HandshakeContext initialization. [v2]
On Fri, 29 Oct 2021 23:58:06 GMT, Clive Verghese 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.handshakeBenchmarkavgt 25 4.210 ± 0.445 ms/op >> >> New Benchmarks >> >> SSLStartHandshake.handshakeBenchmark thrpt 25 0.257 ± 0.017 ops/ms >> SSLStartHandshake.handshakeBenchmarkavgt 25 3.967 ± 0.208 ms/op >> >> >> >> I have also attached the JFR profiles from before and after the change. >> Before >> > src="https://user-images.githubusercontent.com/934461/135705010-a8502966-c6be-4cb5-b743-4a5b382c8e1f.png";> >> >> After >> > 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.` 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 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
Re: RFR: 8274308: Improve efficiency for HandshakeContext initialization. [v2]
On Sat, 30 Oct 2021 04:57:22 GMT, Xue-Lei Andrew Fan wrote: >> 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 >> 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. 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?. With my first variation, I had tried to avoid using the cache. However, The particular implementations had it drawbacks. I have run some benchmarks, by rerunning the SSLStartHandshake Benchmark by increasing the concurrency to Threads.MAX and increasing the number of iterations for warmup and measurement. This triggers the concurrency effects of the cache, and here are my findings. Baseline measurement (The default at origin/master) Benchmark Mode Cnt Score Error Units SSLStartHandshake.handshakeBenchmark thrpt 150 1.056 ± 0.048 ops/ms SSLStartHandshake.handshakeBenchmarkavgt 150 13.198 ± 3.688 ms/op Result "org.openjdk.bench.java.security.SSLStartHandshake.handshakeBenchmark": 1.056 ±(99.9%) 0.048 ops/ms [Average] (min, avg, max) = (0.254, 1.056, 1.194), stdev = 0.174 CI (99.9%): [1.008, 1.103] (assumes normal distribution) Result "org.openjdk.bench.java.security.SSLStartHandshake.handshakeBenchmark": 13.198 ±(99.9%) 3.688 ms/op [Average] (min, avg, max) = (10.128, 13.198, 106.039), stdev = 13.454 CI (99.9%): [9.510, 16.886] (assumes normal distribution) Cache implementation according to this PR with synchronization. Benchmark Mode Cnt Score Error Units SSLStartHandshake.handshakeBenchmark thrpt 150 1.098 ± 0.079 ops/ms SSLStartHandshake.handshakeBenchmarkavgt 150 16.640 ± 6.784 ms/op Result "org.openjdk.bench.java.security.SSLStartHandshake.handshakeBenchmark": 1.098 ±(99.9%) 0.079 ops/ms [Average] (min, avg, max) = (0.087, 1.098, 1.272), stdev = 0.288 CI (99.9%): [1.019, 1.177] (assumes normal distribution) Result "org.openjdk.bench.java.security.SSLStartHandshake.handshakeBenchmark": 16.640 ±(99.9%) 6.784 ms/op [Average] (min, avg, max) = (9.608, 16.640, 166.450), stdev = 24.752 CI (99.9%): [9.855, 23.424] (assumes normal distribution) Concurrent HashMap as a Cache Implementation Benchmark Mode Cnt Score Error Units SSLStartHandshake.handshakeBenchmark thrpt 150 1.163 ± 0.052 ops/ms SSLStartHandshake.handshakeBenchmarkavgt 150 12.570 ± 3.854 ms/op Result "org.openjdk.bench.java.security.SSLStartHandshake.handshakeBenchmark": 1.163 ±(99.9%) 0.052 ops/ms [Average] (min, avg, max) = (0.172, 1.163, 1.287), stdev = 0.191 CI (99.9%): [1.110, 1.215] (assumes normal distribution) Result "org.openjdk.bench.java.security.SSLStartHandshake.handshakeBenchmark": 12.570 ±(99.9%) 3.854 ms/op [Average] (min, avg, max) = (9.261, 12.570, 116.059), stdev = 14.061 CI (99.9%): [8.716, 16.424] (assumes normal distribution) I was synchronizing based on the `handshakeContextCache`. ConcurrentHashMap does provide better results when compared to the baseline, However, I would need figure out the evicting the nodes based on the size. Please do let me know what you think about the results and I can update t
Re: RFR: 8274308: Improve efficiency for HandshakeContext initialization. [v2]
On Sat, 30 Oct 2021 04:57:22 GMT, Xue-Lei Andrew Fan wrote: >> 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. > > src/java.base/share/classes/sun/security/ssl/HandshakeContext.java line 74: > >> 72: "sun.security.ssl.allowLegacyHelloMessages", true); >> 73: >> 74: static Cache >> 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. > 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. - PR: https://git.openjdk.java.net/jdk/pull/5793
Re: RFR: 8274308: Improve efficiency for HandshakeContext initialization. [v2]
On Fri, 5 Nov 2021 22:57:24 GMT, Xue-Lei Andrew Fan wrote: >> src/java.base/share/classes/sun/security/ssl/HandshakeContext.java line 74: >> >>> 72: "sun.security.ssl.allowLegacyHelloMessages", true); >>> 73: >>> 74: static Cache >>> 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. > >> 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. - PR: https://git.openjdk.java.net/jdk/pull/5793
Re: RFR: 8274308: Improve efficiency for HandshakeContext initialization. [v2]
On Sat, 6 Nov 2021 08:54:33 GMT, Daniel Jeliński wrote: > 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. > Yes, it is one direction that we could consider to have more configuration in SSLContext, rather than configure individually in each socket. I think it may improve the performance a lot. > 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? If it is true, I think should optimize the decompose method, may be not limited to caching. - PR: https://git.openjdk.java.net/jdk/pull/5793
Re: RFR: 8274308: Improve efficiency for HandshakeContext initialization. [v2]
On Sat, 6 Nov 2021 08:54:33 GMT, Daniel Jeliński 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`). 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
Re: RFR: 8274308: Improve efficiency for HandshakeContext initialization. [v2]
On Sat, 6 Nov 2021 08:54:33 GMT, Daniel Jeliński 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(?). I don't think it is the direction to change the immunity of SSLContext. There are two proposals right now. One is to change the SSLContext public APIs, @djelinski, by adding cipher suites and protocols configuration (immutable). This proposal does not impact the current code, where enabled protocols and cipher suites are configured in each socket. But it could be useful for future code to configure these parameters in the context level. It may be easier to maintain and could have better performance. I did not have time to think over the potentials yet, but I did have the proposal to move from static cache to instance cache. SSLContext/impl is ideal because normally only one SSLContext instance is required, surely there are exceptions, for an application. We may also missed another issue that the underlying crypto provider could change, for example unplugging a token from the slot. Once the crypto provider is changed, the cached binding between protocol and cipher suites may not work any longer. So we may need to take care of this kind of issue for static cache. For an instance of SSLContext, the crypto provider changing issues are still there, but get mitigated as the context has already bound to specific trust manager and key manager. Yes, it does change the SSLContext implementation from immutability to mutability. But it is in an acceptable level, just as what we did for session cache in the context instance. Just for your consideration. This is a tough problem, and it may take time to find a suitable solution. Let's keep exchanging our thoughts about it. > > 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/cla
Re: RFR: 8274308: Improve efficiency for HandshakeContext initialization. [v2]
On Sat, 6 Nov 2021 08:54:33 GMT, Daniel Jeliński 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. >I don't think it is the direction to change the immunity of SSLContext. There >are two proposals right now. One is to change the SSLContext public APIs, >@djelinski, by adding cipher suites and protocols configuration (immutable). >This proposal does not impact the current code, where enabled protocols and >cipher suites are configured in each socket. But it could be useful for future >code to configure these parameters in the context level. It may be easier to >maintain and could have better performance. > > I did not have time to think over the potentials yet, but I did have the > proposal to move from static cache to instance cache. SSLContext/impl is > ideal because normally only one SSLContext instance is required, surely there > are exceptions, for an application. We may also missed another issue that the > underlying crypto provider could change, for example unplugging a token from > the slot. Once the crypto provider is changed, the cached binding between > protocol and cipher suites may not work any longer. So we may need to take > care of this kind of issue for static cache. For an instance of SSLContext, > the crypto provider changing issues are still there, but get mitigated as the > context has already bound to specific trust manager and key manager. Yes, it > does change the SSLContext implementation from immutability to mutability. > But it is in an acceptable level, just as what we did for session cache in > the context instance. > > Just for your consideration. This is a tough problem, and it may take time to > find a suitable solution. Let's keep exchanging our thoughts about it. Thank you @XueleiFan for the feedback, I'll take a look at it from that perspective as well and post my findings here. > Yes, please. Your found is impressive to me. I'll create a new JBS issue and PR along with benchmarks for this. Thank you. - PR: https://git.openjdk.java.net/jdk/pull/5793
