Code Review Request, 8133070 Hot lock on BulkCipher.isAvailable

[Xuelei Fan]

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
>>>>
>>