Code Review Request, 8133070 Hot lock on BulkCipher.isAvailable

[Sean Mullan]

The updated webrev looks good.

--Sean

On 12/19/2015 11:06 AM, Xuelei Fan wrote:
> 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
>>>>>
>>>
>