RFR: 8172048: Re-examine use of AtomicReference in java.security.Policy

[Claes Redestad]

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