[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