Hi David,
On 2016-12-30 02:10, David Holmes wrote:
Hi Claes,
On 28/12/2016 12:04 AM, Claes Redestad wrote:
Hi,
since java.util.concurrent.AtomicReference was changed to use a
VarHandle internally, using it from within the security libraries can
lead to hard to diagnose bootstrap cycles (since VarHandles has to do
doPrivileged calls during setup). The need to initialize VarHandles is
also cause for a small startup regression for any application run with
a security manager.
The use of AtomicReference in java.security.Policy is not really
motivated, though, since only the .get/.set methods are used, thus a
rather straight-forward fix is to convert the code to use a volatile
reference instead with identical semantics:
I agree, this was a good find! - AtomicReference use was unnecessary in
this class and certainly not worth the additional initialization
complexity.
thanks for reviewing!
Not sure about the "// volatile read" commentary when there is no
corresponding volatile-write commentary, and when not applied in all
locations.
yes, it seems I was lacking in dedication to this idea.
It seems customary to point out that one is intentionally doing a read
into a local variable as to avoid both performance and correctness
issues that'd result if someone tried to "simplify" things. As we're
safely publishing an object with only final fields a comment on the
writes isn't strictly necessary, but I don't mind adding comments
there too for consistency.
Or do we prefer no comments at all? :-)
/Claes
Thanks,
David
Bug: https://bugs.openjdk.java.net/browse/JDK-8172048
Webrev: http://cr.openjdk.java.net/~redestad/8172048/webrev.01/
While a rather insignificant startup improvement in and off itself,
this would help to unblock the attempted fix to resolve
https://bugs.openjdk.java.net/browse/JDK-8062389
Thanks!
/Claes
