JDK 8 RFR 6470700: Math.random() / Math.initRNG() uses "double checked locking"

[Brian Burkhalter]

JDK 8 Reviewers:

The proposed patch is here:

http://cr.openjdk.java.net/~bpb/6470700/

JTREG tests pass and JMH benchmarking shows a very slight performance improvement versus the extant code.

Thanks,

Brian

On Aug 21, 2013, at 7:23 PM, Mike Duigou wrote:

> On Aug 21 2013, at 17:01 , David M. Lloyd wrote:
> 
>> On 8/21/13 5:37 PM, Brian Burkhalter wrote:
>>> With respect to this issue
>>> 
>>> http://bugs.sun.com/view_bug.do?bug_id=6470700
>>> 
>>> the code of concern from java.lang.Math is
>>> 
>>> 701    private static Random randomNumberGenerator;
>>> 702
>>> 703    private static synchronized Random initRNG() {
>>> 704        Random rnd = randomNumberGenerator;
>>> 705        return (rnd == null) ? (randomNumberGenerator = new Random()) : rnd;
>>> 706     }
>>> 
>>> 731    public static double random() {
>>> 732        Random rnd = randomNumberGenerator;
>>> 733        if (rnd == null) rnd = initRNG();
>>> 734        return rnd.nextDouble();
>>> 735    }
>>> 
>>> where the class variable "randomNumberGenerator" is not used anywhere else in the class. The complaint is that this is an instance of the broken double-checked locking pattern. While at first glance this might appear to be the case, it does not seem so to me. It looks more like an attempt to avoid calling a synchronized method if "randomNumberGenerator" has already been initialized.
>>> 
>>> A more typical pattern would be
>>> 
>>>    private static Random randomNumberGenerator;
>>> 
>>>    private static synchronized Random getRNG() {
>>>        if (randomNumberGenerator == null) {
>>>            randomNumberGenerator = new Random();
>>>        }
>>>        return randomNumberGenerator;
>>>    }
>>> 
>>>    public static double random() {
>>>        return getRNG().nextDouble();
>>>    }
>>> 
>>> Comments, please.
>> 
>> I don't think you'd want to introduce the overhead of synchronization here.  It may be better in this case to use this kind of lazy init pattern:
>> 
>>   static final class Holder {
>>       static final Random RNG = new Random();
>>   }
>> 
>>   public static double random() {
>>       return Holder.RNG.nextDouble();
>>   }
>> 
>> -- 
>> - DML
> 
> 
> David's suggestion seems very reasonable.
> 
> 
> Mike