If anyone is still reviewing this and need more time, please let me know
soon. Otherwise, I will proceed with integration this afternoon.
Thanks,
Valerie
On 12/6/2018 7:27 PM, Valerie Peng wrote:
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
