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

[David Holmes]

On 31/12/2016 12:50 AM, Claes Redestad wrote:
> 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.

Not sure how others have done this but the read-into-a-local should be 
documented more clearly as it isn't just the volatile aspect that is 
critical (for memory ordering) but the read-once aspect!

The volatile write must also come after all other initialization actions 
- even when done inside a sync block. (It does, but it isn't obvious 
that it does, or that it has to.)

> Or do we prefer no comments at all? :-)

Not sure. I'll let you think about it so more. I'll be back in the 
office on Tuesday :)

Cheers,
David

> /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