[13] RFR JDK-7107615 "scalability bloker in javax.crypto.JceSecurity"

[Valerie Peng]

Hi Xuelei,

Based on our earlier discussions, I experimented with a few approaches 
and updated the changes accordingly:

http://cr.openjdk.java.net/~valeriep/7107615/webrev.02/

Please let me know if you have more comments.

Thanks,
Valerie
On 4/19/2019 3:58 PM, Valerie Peng wrote:
> Ok, thanks for the double checking suggestion.
>
> Given the somewhat limited number of providers, I am exploring other 
> data structures such as WeakHashMap.
>
> Will have to put this on hold during my upcoming week-long vacation 
> and resume on 4/29.
>
> Thanks!
> Valerie
> On 4/16/2019 3:43 PM, Xuelei Fan wrote:
>> On 4/16/2019 2:16 PM, Valerie Peng wrote:
>>> Hi Xuelei,
>>>
>>> Thanks for the comments.
>>>
>>> On 4/16/2019 8:58 AM, Xuelei Fan wrote:
>>>> 213-217:
>>>> Previously, the set/get of both verificationResults and 
>>>> verifyingProviders are synchronized.  With this update, the get of 
>>>> verificationResults is not out of the set synchronized block, so 
>>>> there is a competition introduced.  I think it would be nice to 
>>>> double check the verificationResults in the synchronized block.
>>>>   synchronized (JceSecurity.class) {
>>>> +     //  double check if the provider has been verified
>>>>       ...
>>>>   }
>>>>
>>> Hmm, are you talking about the verificationResults.get(pKey) call 
>>> (line 206) and move it inside the block of synchronized 
>>> (JceSecurity.class) (line 212)?
>> I meant to get/check twice.
>> ... getVerificationResult(Provider p) {
>>     ...
>> // get/check block
>>     Object o = verificationResults.get(pKey);
>>     if ( o == ...) {
>>         ...
>>     } else if (o != null) {
>>         ...
>>     }
>>
>>     synchronized (JceSecurity.class) {
>>        // double check if the provider has been verified.
>> // use the get/check block again
>>        Object o = verificationResults.get(pKey);
>>        if ( o == ...) {
>>            ...
>>        } else if (o != null) {
>>            ...
>>        }
>>
>>        // further operation is the provider has not been verified.
>>        ....
>>     }
>> }
>>
>>> If that's what you suggest, I think that'd take away the key idea of 
>>> this performance rfe fix. My understanding of the patch is to use a 
>>> ConcurrentHashMap for "verificationResult" so accesses to this 
>>> "verificationResult" cache are controlled by the finer-grained model 
>>> of ConcurrentHashMap. On the other hand, verifyingProviders map and 
>>> the time consuming/potentially complex verifyProvider(...) call are 
>>> still inside the synchronized (JceSecurity.class) block. Maybe it'd 
>>> be clearer if I had re-written the code and move the 
>>> verificationResults.put(...) calls outside of the synchronized 
>>> (JceSecurity.class) block. The way I look at it, this change 
>>> separates out the "verificationResult" cache from the rest of the 
>>> provider verification logic and uses ConcurrentHashMap instead of 
>>> JceSecurity.class in order to improve performance.
>>>
>> I think there is a competition.  If two threads try to get the lock, 
>> line 212-227 get executed in both thread.  The double checking scheme 
>> could avoid it as when the 2nd thread get the lock, it will use the 
>> result of the 1st thread.
>>
>>>> I did not get the idea to use verifyingProviders.  In line 219, a 
>>>> provider was inserted into the map, and in the final block, line 
>>>> 235, the provider was removed from the map.  There is no other 
>>>> update of the map, so the map should always be empty and not really 
>>>> used. Am I missing something?  Could the verifyingProviders field 
>>>> get removed?
>>>>
>>> The verifyingProviders field is for detecting potential recursions 
>>> during the provider verification. the verifyProvider(URL, Provider) 
>>> call will call ProviderVerifier.verify(). This will trigger provider 
>>> jar file verification. Depending on runtime provider verification, 
>>> this may trigger another provider being loaded/verified. As 
>>> getVerificationResult(Provider) is being called by the pkg private 
>>> static method JceSecurity.getInstance(...) which is used internally 
>>> by other JCA classes, it's possible for getVerificationResult(...) 
>>> to be recursively called and thus the verifyingProviders field will 
>>> help detect if somehow verifying the current provider will require 
>>> loading another JCA service from the current provider.
>>>
>> I see the point now.  Thanks!
>>
>>>>
>>>> 406-426:
>>>> I may add a blank line between two different blocks (methods, field 
>>>> and methods).
>>>>
>>> Sure, added.
>>>
>>>
>>>> - 416 if (o instanceof IdentityWrapper == false) {
>>>> + 416  if (!(o instanceof IdentityWrapper)) {
>>>>
>>>> I prefer to use "!" operator.
>>>
>>> Sure.
>>>
>> Thanks!
>>
>> Xuelei
>>
>>> Webrev updated at: 
>>> http://cr.openjdk.java.net/~valeriep/7107615/webrev.01/
>>>
>>> Thanks,
>>> Valerie
>>>>
>>>> Xuelei
>>>>
>>>> On 4/15/2019 6:20 PM, Valerie Peng wrote:
>>>>>
>>>>> Anyone has time to review this performance rfe? The fix is based 
>>>>> on Sergey's suggested patch.
>>>>>
>>>>> RFE: https://bugs.openjdk.java.net/browse/JDK-7107615
>>>>>
>>>>> Webrev: http://cr.openjdk.java.net/~valeriep/7107615/webrev.00/
>>>>>
>>>>> Mach5 run is clean.
>>>>>
>>>>> Thanks,
>>>>> Valerie