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

Valerie Peng valerie.peng at oracle.com
Fri Apr 19 22:58:37 UTC 2019


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



More information about the security-dev mailing list