<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>Hi Xuelei,</p>
<p>Thanks for the comments.<br>
</p>
<div class="moz-cite-prefix">On 4/16/2019 8:58 AM, Xuelei Fan wrote:<br>
</div>
<blockquote type="cite"
cite="mid:69d32064-a129-9809-d861-7c00237ab000@oracle.com">213-217:
<br>
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.
<br>
synchronized (JceSecurity.class) {
<br>
+ // double check if the provider has been verified
<br>
...
<br>
}
<br>
<br>
</blockquote>
<p>Hmm, are you talking about the <span class="changed">verificationResults.get(pKey)
call (line 206) and move it inside the </span><span class="new">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 </span><span class="new"><span
class="new">time consuming/potentially complex </span></span><span
class="new"><span class="new"><span class="new">verifyProvider(...)
call</span> </span> </span><span class="new"></span><span
class="new">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.<br>
</span></p>
<blockquote type="cite"
cite="mid:69d32064-a129-9809-d861-7c00237ab000@oracle.com">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?
<br>
<br>
</blockquote>
<p>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.</p>
<blockquote type="cite"
cite="mid:69d32064-a129-9809-d861-7c00237ab000@oracle.com">
<br>
406-426:
<br>
I may add a blank line between two different blocks (methods,
field and methods).
<br>
<br>
</blockquote>
<p>Sure, added.</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:69d32064-a129-9809-d861-7c00237ab000@oracle.com">- 416
if (o instanceof IdentityWrapper == false) {
<br>
+ 416 if (!(o instanceof IdentityWrapper)) {
<br>
<br>
I prefer to use "!" operator.
<br>
</blockquote>
<p>Sure.</p>
<p>Webrev updated at: <a
href="http://cr.openjdk.java.net/~valeriep/7107615/webrev.01/">http://cr.openjdk.java.net/~valeriep/7107615/webrev.01/</a></p>
Thanks,<br>
Valerie<br>
<blockquote type="cite"
cite="mid:69d32064-a129-9809-d861-7c00237ab000@oracle.com">
<br>
Xuelei
<br>
<br>
On 4/15/2019 6:20 PM, Valerie Peng wrote:
<br>
<blockquote type="cite">
<br>
Anyone has time to review this performance rfe? The fix is based
on Sergey's suggested patch.
<br>
<br>
RFE: <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-7107615">https://bugs.openjdk.java.net/browse/JDK-7107615</a>
<br>
<br>
Webrev: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~valeriep/7107615/webrev.00/">http://cr.openjdk.java.net/~valeriep/7107615/webrev.00/</a>
<br>
<br>
Mach5 run is clean.
<br>
<br>
Thanks,
<br>
Valerie
<br>
</blockquote>
</blockquote>
</body>
</html>