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
