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

[Valerie Peng]

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)? 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 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.

>
> 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.

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20190416/3ac0120c/attachment.htm>