Hi Sean,
Thanks for your review, I have removed the stale comment from the
VerificationProvider.
As for your concern on race condition, legacyChanged and the
String/String legacy mapping are still synchronized on the provider
object. Only the service mapping is managed by the ConcurrentHashMap. I
have also changed serviceChanged to be volatile. For those methods which
no longer have synchronized keywords, they still have synchronized
blocks around the legacy mapping related code. In general, the provider
mappings are populated during construction time and rarely updated
afterwards. In addition, most if not all, they either use all legacy or
all service. The mixing of them makes the code un-necessarily
complicated, oh-well.
Latest webrev: http://cr.openjdk.java.net/~valeriep/7092821/webrev.01
Valerie
On 12/4/2018 2:29 PM, Sean Mullan wrote:
I haven't reviewed all of this, but had a couple of comments so far:
- VerificationProvider.java
77 // the provider. Otherwise, create a temporary map and use a
This comment is now stale so it needs to be removed/updated.
- Provider.java
You removed synchronized from several of the methods, but some of
those methods modified fields such as servicesChanged and
legacyChanged. Are there any concurrency issues or race conditions now
that those methods are not synchronized?
--Sean
On 11/21/18 1:05 PM, Valerie Peng wrote:
Hi,
Can someone help reviewing this fix?
Besides changing the Provider class to use ConcurrentHashMap in order
to reduce the lock contention on Provider.getService() calls, I also
changed the security providers in java.base module to register
through putService(...) calls. Performance is manually verified and
mach5 run is clean.
Bug: https://bugs.openjdk.java.net/browse/JDK-7092821
Webrev: http://cr.openjdk.java.net/~valeriep/7092821/webrev.00/
Thanks,
Valerie
