new webrev: http://cr.openjdk.java.net/~xuelei/8133070/webrev.01/
Updates to webrev.00: Re-org the get methods of SSLContextImpl so as to avoid repeated initialization of instance fields. On 12/18/2015 11:35 PM, Sean Mullan wrote: > Here are a few other other comments on the code: > > SSLContextImpl: > - I noticed that SSLContext.init does not specify how it handles empty > arrays, and you have changed the code so that an empty TrustManager > array is treated like they are null - is this change in behavior a > compatibility issue at all? Could you file a follow-on issue to clarify > the behavior of SSLContext.init as to how it should handle empty arrays? > It's a bug of my update. Empty means no trust manager, which is different from using the default trust manager. I removed this update. > - are there any race conditions that you need to worry about now that > you have removed the synchronized blocks from > getSupportedCipherSuiteList and getDefaultCipherSuite? > May be initialized repeatedly. Should be OK as there is only one instance for the to-be-assigned value. The code can be more straightforward and effective. I moved the impls into sub-classes. > - on lines 781 and 789, an empty array is created and discarded if > there is not an exception thrown. It would be better to only create the > empty array when an exception is thrown (move it into the catch block). > Updated. Thanks, Xuelei > --Sean > > > On 12/14/2015 10:08 PM, Xuelei Fan wrote: >> On 12/15/2015 4:24 AM, Sean Mullan wrote: >>> Hi Xuelei, >>> >>> For JDK 9, the EC impl is defined to be in its own module >>> (jdk.crypto.ec). How does it affect this fix if that module is not >>> available/installed? >>> >> The SunJSSE provider would not support dynamically loading of crypto >> providers/modules. At present, there are two providers that supports EC >> implementation, SunEC (jdk.crypto.ec) and SunPKCS11 (jdk.crypto.pkcs11). >> If no EC provider, EC cipher suites would not be available to use. >> That's to say, EC cipher suites would not be enabled by default, and >> cannot be used for handshaking. In the fix, >> JsseJce#EcAvailability.isAvailable is used to check the availability of >> EC crypto impl. >> >> Xuelei >> >>> --Sean >>> >>> On 12/01/2015 07:49 AM, Xuelei Fan wrote: >>>> Hi, >>>> >>>> Please review the fix for JDK-8133070: >>>> >>>> http://cr.openjdk.java.net/~xuelei/8133070/webrev.00/ >>>> >>>> In (Open)JDK 6, EC cipher suites get supported by Java. However, there >>>> is no default EC provider in JDK 6 at that time. In order to support >>>> third part's EC algorithm JCE provider dynamically, it is hard-coded to >>>> check the cipher suite availability dynamically for EC algorithms in >>>> SunJSSE provider. >>>> >>>> Here is an example update in CipherSuite.java for the checking: >>>> >>>> - static final boolean DYNAMIC_AVAILABILITY = false; >>>> + static final boolean DYNAMIC_AVAILABILITY = true; >>>> >>>> The dynamically checking impacts the performance significantly as: >>>> 1. the check of the availability is expensive as it involves crypto >>>> operations. >>>> 2. a cache is used to maintain the availability of bulk ciphers in >>>> order >>>> to mitigate the #1 performance impact. However, access and update to >>>> the cache need to be synchronized. >>>> 3. in order to support dynamically checking, the cache may be >>>> cleared if >>>> a particular cipher is not found or a new SSLContext is generated. As >>>> bring the performance impact of #1 back again. >>>> >>>> Later, AEAD algorithm is defined by Java. The same mechanism is >>>> used to >>>> support AEAD ciphers. >>>> >>>> Now, EC and GCM algorithms are supported in JDK crypto providers. The >>>> hard-coded checking can get improved. This fix updates: >>>> 1. remove the dynamically checking of cipher suite availability. >>>> 2. remove the cipher suite availability cache accordingly (need no >>>> synchronization accordingly) >>>> 3. other updates that impact by the availability checking. >>>> >>>> The performance improvement is tested with the following simple case. >>>> Run the following code 1000, 2000, 3000 times in single thread mode and >>>> measure the millisecond for each: >>>> >>>> --------- >>>> String[] cipherSuites = >>>> {"TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256"}; >>>> for (int i = 0; i < loops; i++) { // loops: 1000, 2000, 3000 >>>> SSLEngine engine = SSLContext.getDefault().createSSLEngine(); >>>> engine.getEnabledCipherSuites(); >>>> engine.getSupportedCipherSuites(); >>>> } >>>> --------- >>>> >>>> The milliseconds for each test case (less is better) look like: >>>> >>>> loops | old | fixed >>>> ---------+---------+---------- >>>> 1000 | 2736 | 735 >>>> ---------+---------+---------- >>>> 2000 | 3718 | 746 >>>> ---------+---------+---------- >>>> 3000 | 4750 | 765 >>>> ---------+---------+---------- >>>> >>>> This fix improves the performance. >>>> >>>> The existing regression testing get passed. No new regression test is >>>> planned as this is a performance enhancement fix. >>>> >>>> Thanks, >>>> Xuelei >>>> >>
