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

[Xuelei Fan]

208  synchronized (JceSecurity.class) {
229  synchronized(verificationResults) {
I did not get the point to use two locks.  Using one lock, either 
JceSecurity.class or verificationResults should be fine.

229-232:
I did not get the point of these 4 lines.  Could line 231 move between 
line 226 and 267, and remove line 228-232?
  208   synchronized (JceSecurity.class) {
  209       o = verificationResults.get(pKey);
  210       if (o == null) {
               ...
  224          } finally {
  225             verifyingProviders.remove(p);
  226          }
+
+             verificationResults.put(pKey, o);
  227       }

Otherwise, looks fine to me.

Thanks,
Xuelei

On 5/10/2019 4:36 PM, Valerie Peng wrote:
> 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