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

[Xuelei Fan]

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